Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v5 01/10] tpm, tpm_tis: Avoid cache incoherency in test
 for interrupts
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, l.sanfilippo@kunbus.com,
        lukas@wunner.de, p.rosenberger@kunbus.com
References: <20220610110846.8307-1-LinoSanfilippo@gmx.de>
 <20220610110846.8307-2-LinoSanfilippo@gmx.de> <YqntNDU5tcwgDdvG@kernel.org>
From:   Lino Sanfilippo <LinoSanfilippo@gmx.de>
Message-ID: <967ab8d9-2c57-508f-b009-586f560b2c57@gmx.de>
Date:   Thu, 16 Jun 2022 00:06:36 +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: <YqntNDU5tcwgDdvG@kernel.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


Hi,

On 15.06.22 at 16:32, Jarkko Sakkinen wrote:
> On Fri, Jun 10, 2022 at 01:08:37PM +0200, LinoSanfilippo@gmx.de wrote:
>> From: Lino Sanfilippo <l.sanfilippo@kunbus.com>
>>
>> The interrupt handler that sets the boolean variable irq_tested may run=
 on
>> another CPU as the thread that checks irq_tested as part of the irq tes=
t in
>> tmp_tis_send().
>>
>> Since nothing guarantees cache coherency between CPUs for unsynchronize=
d
>> accesses to boolean variables the testing thread might not perceive the
>> value change done in the interrupt handler.
>>
>> Avoid this issue by using a bitfield instead of a boolean variable and =
by
>> accessing this field with the bit manipulating functions that provide c=
ache
>> 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 dc56b976d816..6f2cf75add8b 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_IRQ_TESTED, &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_IRQ_TESTED, &priv->irqtest_flags))
>>  		tpm_msleep(1);
>> -	if (!priv->irq_tested)
>> +	if (!test_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags))
>>  		disable_interrupts(chip);
>> -	priv->irq_tested =3D true;
>> +	set_bit(TPM_TIS_IRQ_TESTED, &priv->irqtest_flags);
>>  	return rc;
>>  }
>>
>> @@ -693,7 +694,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_IRQ_TESTED, &priv->irqtest_flags);
>>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>>  		wake_up_interruptible(&priv->read_queue);
>>  	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
>> @@ -779,7 +780,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_IRQ_TESTED, &priv->irqtest_flags);
>>
>>  	/* Generate an interrupt by having the core call through to
>>  	 * tpm_tis_send
>> diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis=
_core.h
>> index 3be24f221e32..0f29d0b68c3e 100644
>> --- a/drivers/char/tpm/tpm_tis_core.h
>> +++ b/drivers/char/tpm/tpm_tis_core.h
>> @@ -88,11 +88,15 @@ enum tpm_tis_flags {
>>  	TPM_TIS_INVALID_STATUS		=3D BIT(1),
>>  };
>>
>> +enum tpm_tis_irqtest_flags {
>> +	TPM_TIS_IRQ_TESTED		=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.1
>>
>
> Otherwise looks fine, but please add TPM_TIS_IRQ_TESTED to 'flags', and
> convert existing sites to use set_bit() and and test_bit().
>
> BR, Jarkko
>

Ok, will do.

Regards,
Lino