Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Subject: Re: [PATCH v4 4/6] tpm, tpm_tis: avoid CPU cache incoherency in irq
 test
To:     Jarkko Sakkinen <jarkko@kernel.org>
Cc:     peterhuewe@gmx.de, jgg@ziepe.ca, stefanb@linux.vnet.ibm.com,
        linux@mniewoehner.de, linux-integrity@vger.kernel.org,
        linux-kernel@vger.kernel.org, lukas@wunner.de,
        p.rosenberger@kunbus.com, Lino Sanfilippo <l.sanfilippo@kunbus.com>
References: <20220509080559.4381-1-LinoSanfilippo@gmx.de>
 <20220509080559.4381-5-LinoSanfilippo@gmx.de> <YnvRC3FI7bcXj7zL@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <b7885fcf-70ae-9d7d-dcfb-97d77251c71d@gmx.de>
Date:   Wed, 11 May 2022 21:35:44 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YnvRC3FI7bcXj7zL@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On 11.05.22 at 17:06, Jarkko Sakkinen wrote:
> On Mon, May 09, 2022 at 10:05:57AM +0200, Lino Sanfilippo wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> The interrupt handler that sets irq_tested to indicate that interrupts =
are
>> working may run on another CPU than the thread that checks this variabl=
e in
>> tmp_tis_send().
>>
>> Since no synchronization is used to access irq_tested, there is no
>> guarantee for cache coherency between the CPUs, so that the value set b=
y
>> the interrupt handler might not be visible to the testing thread.
>>
>> Avoid this issue by using a bitfield instead of a boolean variable and =
by
>> accessing this field with bit manipulating functions that guarantee cac=
he
>> coherency.
>>
>> Signed-off-by: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>> ---
>>  drivers/char/tpm/tpm_tis_core.c | 13 +++++++------
>>  drivers/char/tpm/tpm_tis_core.h |  6 +++++-
>>  2 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis=
_core.c
>> index 4f3b82c3f205..bdfde1cd71fe 100644
>> --- a/drivers/char/tpm/tpm_tis_core.c
>> +++ b/drivers/char/tpm/tpm_tis_core.c
>> @@ -470,7 +470,8 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *=
buf, size_t len)
>>  	int rc, irq;
>>  	struct tpm_tis_data *priv =3D dev_get_drvdata(&chip->dev);
>>
>> -	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) || priv->irq_tested)
>> +	if (!(chip->flags & TPM_CHIP_FLAG_IRQ) ||
>> +	     test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		return tpm_tis_send_main(chip, buf, len);
>>
>>  	/* Verify receipt of the expected IRQ */
>> @@ -480,11 +481,11 @@ static int tpm_tis_send(struct tpm_chip *chip, u8=
 *buf, size_t len)
>>  	rc =3D tpm_tis_send_main(chip, buf, len);
>>  	priv->irq =3D irq;
>>  	chip->flags |=3D TPM_CHIP_FLAG_IRQ;
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		tpm_msleep(1);
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags))
>>  		disable_interrupts(chip);
>> -	priv->irq_tested =3D true;
>> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	return rc;
>>  }
>>
>> @@ -689,7 +690,7 @@ static irqreturn_t tis_int_handler(int dummy, void =
*dev_id)
>>  	if (interrupt =3D=3D 0)
>>  		return IRQ_NONE;
>>
>> -	priv->irq_tested =3D true;
>> +	set_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>>  		wake_up_interruptible(&priv->read_queue);
>>  	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
>> @@ -780,7 +781,7 @@ static int tpm_tis_probe_irq_single(struct tpm_chip=
 *chip, u32 intmask,
>>  	if (rc < 0)
>>  		return rc;
>>
>> -	priv->irq_tested =3D false;
>> +	clear_bit(TPM_TIS_IRQTEST_OK, &priv->irqtest_flags);
>>  	chip->flags |=3D TPM_CHIP_FLAG_IRQ;
>>
>>  	/* Generate an interrupt by having the core call through to
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis=
_core.h
>> index 43b724e55192..c8972ea8e13e 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -89,11 +89,15 @@ enum tpm_tis_flags {
>>  	TPM_TIS_USE_THREADED_IRQ	=3D BIT(2),
>>  };
>>
>> +enum tpm_tis_irqtest_flags {
>> +	TPM_TIS_IRQTEST_OK		=3D BIT(0),
>> +};
>> +
>>  struct tpm_tis_data {
>>  	u16 manufacturer_id;
>>  	int locality;
>>  	int irq;
>> -	bool irq_tested;
>> +	unsigned long irqtest_flags;
>>  	unsigned long flags;
>>  	void __iomem *ilb_base_addr;
>>  	u16 clkrun_enabled;
>> --
>> 2.36.0
>>
>
> So, this would caused by changing to threaded IRQs?
>
> BR, Jarkko
>

No, this is caused by the fact that we access irq_tested from different pa=
ths of execution.
The writing and reading to/from irq_tested from different execution paths =
without any
synchronization is a plain bug in the existing code:
We simply cannot guarantee that the value we set for irq_tested in the int=
errupt handler
which may run on CPU 1 is visible in tpm_tis_send() which may run on CPU2.=
 This is because
there is no enforced data transfer between the CPUs and thus the written v=
alue may end up in
the CPU cache of CPU 1 and never (or at least not within the timout of 1 m=
s as used in tpm_tis_send())
be seen by CPU 2.
This kind of issues are in length described in memory-barriers.txt.

The patch is supposed to fix this bug.

Regards,
Lino