tpm: Fix barriers to prevent hwrng from activating during resume

[PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Thomas Fourier 1 month, 4 weeks ago

The memory barrier is placed after clearing the suspened flag so
memory reordering between the wake up and clearing the flag is allowed.
The reader of the flag is missing the corresponding barrier.

Clear the flag after the memory barrier and add a memory barrier in
the reader.

Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/char/tpm/tpm-chip.c      | 3 +++
 drivers/char/tpm/tpm-interface.c | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 12b7394b34bd..7f500797b7a7 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -173,6 +173,9 @@ int tpm_try_get_ops(struct tpm_chip *chip)
 	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
 		goto out_lock;
 
+	/* Ensure that device is fully resumed */
+	rmb();
+
 	rc = tpm_chip_start(chip);
 	if (rc)
 		goto out_lock;
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f745a098908b..2de12b02f62b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -474,13 +474,12 @@ int tpm_pm_resume(struct device *dev)
 	if (chip == NULL)
 		return -ENODEV;
 
-	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
-
 	/*
 	 * Guarantee that SUSPENDED is written last, so that hwrng does not
 	 * activate before the chip has been fully resumed.
 	 */
 	wmb();
+	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Jarkko Sakkinen 1 month, 3 weeks ago

On Mon, Aug 03, 2026 at 11:13:24AM +0200, Thomas Fourier wrote:
> The memory barrier is placed after clearing the suspened flag so
> memory reordering between the wake up and clearing the flag is allowed.
> The reader of the flag is missing the corresponding barrier.
> 
> Clear the flag after the memory barrier and add a memory barrier in
> the reader.
> 
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---

I tagged this too quickly, sorry, let's revisit the patch a bit
just to make sure that we do right things right.

>  drivers/char/tpm/tpm-chip.c      | 3 +++
>  drivers/char/tpm/tpm-interface.c | 3 +--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 12b7394b34bd..7f500797b7a7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -173,6 +173,9 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
>  		goto out_lock;
>  
> +	/* Ensure that device is fully resumed */
> +	rmb();
> +
>  	rc = tpm_chip_start(chip);
>  	if (rc)
>  		goto out_lock;
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f745a098908b..2de12b02f62b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -474,13 +474,12 @@ int tpm_pm_resume(struct device *dev)
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
> -
>  	/*
>  	 * Guarantee that SUSPENDED is written last, so that hwrng does not
>  	 * activate before the chip has been fully resumed.
>  	 */
>  	wmb();
> +	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;

Can you rationalize this change?

>  
>  	return 0;
>  }
> -- 
> 2.43.0
> 

BR, Jarkko

Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Richard Lyu 1 month, 1 week ago

>> memory reordering between the wake up and clearing the flag is allowed.

When exactly can this reordering happen?

>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 12b7394b34bd..7f500797b7a7 100644 --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c @@ -173,6 +173,9 @@ int
>> tpm_try_get_ops(struct tpm_chip *chip) if (chip->flags &
>> TPM_CHIP_FLAG_SUSPENDED) goto out_lock;
>>
>> +	/* Ensure that device is fully resumed */
>> +	rmb();
>> +
>>  	rc = tpm_chip_start(chip);
>>  	if (rc)
>>  		goto out_lock;

Where inside tpm_chip_start do we actually need to avoid loading a 
stale state or flag?

>> diff --git a/drivers/char/tpm/tpm-interface.c
>> b/drivers/char/tpm/tpm-interface.c index f745a098908b..2de12b02f62b 100644
>> --- a/drivers/char/tpm/tpm-interface.c +++
>> b/drivers/char/tpm/tpm-interface.c @@ -474,13 +474,12 @@ int
>> tpm_pm_resume(struct device *dev) if (chip == NULL) return -ENODEV;
>>
>> -	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>> -
>>  	/*
>>  	 * Guarantee that SUSPENDED is written last, so that hwrng does not
>>  	 * activate before the chip has been fully resumed.
>>  	 */
>>  	wmb();
>> +	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>
>Can you rationalize this change?

I agree the clearing of the flag should be moved after wmb() to guarantee 
that SUSPENDED is written last, the flag has to be cleared after the barrier.
That part makes sense.
  
My remaining question is whether we actually need the wmb() barrier here?

Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Thomas Fourier 1 month, 1 week ago

Thank you all for your time and comments. Revisiting my patch after my 
vacation, I now think none of the barriers are actually necessary.

On 21/08/2026 11:40, Richard Lyu wrote:
>>> memory reordering between the wake up and clearing the flag is allowed.
> 
> When exactly can this reordering happen?
> 
>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>> index 12b7394b34bd..7f500797b7a7 100644 --- a/drivers/char/tpm/tpm- 
>>> chip.c
>>> +++ b/drivers/char/tpm/tpm-chip.c @@ -173,6 +173,9 @@ int
>>> tpm_try_get_ops(struct tpm_chip *chip) if (chip->flags &
>>> TPM_CHIP_FLAG_SUSPENDED) goto out_lock;
>>>
>>> +    /* Ensure that device is fully resumed */
>>> +    rmb();
>>> +
>>>      rc = tpm_chip_start(chip);
>>>      if (rc)
>>>          goto out_lock;
> 
> Where inside tpm_chip_start do we actually need to avoid loading a stale 
> state or flag?
tpm_chip_start() reads chip->locality for example.  I'm not sure it can 
be written to concurrently but some drivers write to it. It might not be 
a problem as it would just trigger a call to tpm_request_locality().

Since speculative writes are not possible, so rmb() is sufficient; in 
any case, no need for a read-to-write memory barrier.
> 
>>> diff --git a/drivers/char/tpm/tpm-interface.c
>>> b/drivers/char/tpm/tpm-interface.c index f745a098908b..2de12b02f62b 
>>> 100644
>>> --- a/drivers/char/tpm/tpm-interface.c +++
>>> b/drivers/char/tpm/tpm-interface.c @@ -474,13 +474,12 @@ int
>>> tpm_pm_resume(struct device *dev) if (chip == NULL) return -ENODEV;
>>>
>>> -    chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>>> -
>>>      /*
>>>       * Guarantee that SUSPENDED is written last, so that hwrng does not
>>>       * activate before the chip has been fully resumed.
>>>       */
>>>      wmb();
>>> +    chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>>
>> Can you rationalize this change?
This change is mostly based on your comment: tpm_pm_resume() is called 
after the hardware-specific operations have been performed, and the flag 
must be set after these operations are complete.

I assumed hwrng corresponds to tpm_hwrng_read() which calls 
tpm_get_random() which itself calls tpm_try_get_ops(). After 
tpm_chip_start() is called, tpm_try_get_ops() can return a valid 
pointer, and tpm_hwrng_read() can work.

Individual drivers implement the .resume() method by doing 
hardware-specific operations then calling tpm_pm_resume(), so ordering 
needs to happen between those hardware-specific operations and 
tpm_pm_resume().  This ensures that the order of operations is:
   - hardware-specific resume operations,
   - tpm_pm_resume(), including clearing the TPM_CHIP_FLAG_SUSPENDED flag,
   - check that TPM_CHIP_FLAG_SUSPENDED is clear,
   - run of tpm_chip_start().

If tpm_try_get_ops() succeeds, then tpm_chip_start() is run so the 
hardware-specific resume operations are complete.

> 
> I agree the clearing of the flag should be moved after wmb() to 
> guarantee that SUSPENDED is written last, the flag has to be cleared 
> after the barrier.
> That part makes sense.
> 
> My remaining question is whether we actually need the wmb() barrier here?
Revisiting the patch, I agree that this point is not fully clear. 
Looking at the implementations of tpm drivers, most use the 
tpm_pm_resume() function directly as the .resume() method.

Three drivers implement specific .resume() methods:
   - tpm_infineon.c: which uses writeb/outb that adds the necessary 
barriers,
   - st33zp24/st33zp24.c: which calls tpm_pm_resume() without 
driver-specific operation depending on the config,
   - tpm_tis_core.c: which makes a self-test before calling tpm_pm_resume().

None of those cases seem to require a barrier at all.

Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Jarkko Sakkinen 1 month, 3 weeks ago

On Mon, Aug 03, 2026 at 11:13:24AM +0200, Thomas Fourier wrote:
> The memory barrier is placed after clearing the suspened flag so
> memory reordering between the wake up and clearing the flag is allowed.
> The reader of the flag is missing the corresponding barrier.
> 
> Clear the flag after the memory barrier and add a memory barrier in
> the reader.
> 
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
> ---
>  drivers/char/tpm/tpm-chip.c      | 3 +++
>  drivers/char/tpm/tpm-interface.c | 3 +--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 12b7394b34bd..7f500797b7a7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -173,6 +173,9 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
>  		goto out_lock;
>  
> +	/* Ensure that device is fully resumed */
> +	rmb();
> +
>  	rc = tpm_chip_start(chip);
>  	if (rc)
>  		goto out_lock;
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f745a098908b..2de12b02f62b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -474,13 +474,12 @@ int tpm_pm_resume(struct device *dev)
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
> -
>  	/*
>  	 * Guarantee that SUSPENDED is written last, so that hwrng does not
>  	 * activate before the chip has been fully resumed.
>  	 */
>  	wmb();
> +	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>  
>  	return 0;
>  }
> -- 
> 2.43.0


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Applied.

BR, Jarkko

Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume

Posted by Jarkko Sakkinen 1 month, 4 weeks ago

On Mon, Aug 03, 2026 at 11:13:24AM +0200, Thomas Fourier wrote:
> The memory barrier is placed after clearing the suspened flag so
> memory reordering between the wake up and clearing the flag is allowed.
> The reader of the flag is missing the corresponding barrier.
> 
> Clear the flag after the memory barrier and add a memory barrier in
> the reader.
> 
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>

Thanks I investigate this properly before answering. It's my first day
after holiday so might take until Tue/Wed.

PS. And if someone noticed, sorry for copy-pasting the same response to
two bug reports. I just cherry-picked critical looking stuff from my
inbox :-)

BR, Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c      | 3 +++
>  drivers/char/tpm/tpm-interface.c | 3 +--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 12b7394b34bd..7f500797b7a7 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -173,6 +173,9 @@ int tpm_try_get_ops(struct tpm_chip *chip)
>  	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
>  		goto out_lock;
>  
> +	/* Ensure that device is fully resumed */
> +	rmb();
> +
>  	rc = tpm_chip_start(chip);
>  	if (rc)
>  		goto out_lock;
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f745a098908b..2de12b02f62b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -474,13 +474,12 @@ int tpm_pm_resume(struct device *dev)
>  	if (chip == NULL)
>  		return -ENODEV;
>  
> -	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
> -
>  	/*
>  	 * Guarantee that SUSPENDED is written last, so that hwrng does not
>  	 * activate before the chip has been fully resumed.
>  	 */
>  	wmb();
> +	chip->flags &= ~TPM_CHIP_FLAG_SUSPENDED;
>  
>  	return 0;
>  }
> -- 
> 2.43.0
>