* Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume
2026-08-03 9:13 [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume Thomas Fourier
@ 2026-08-03 12:00 ` Jarkko Sakkinen
2026-08-10 15:39 ` Jarkko Sakkinen
2026-08-11 0:03 ` Jarkko Sakkinen
2 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2026-08-03 12:00 UTC (permalink / raw)
To: Thomas Fourier
Cc: stable, Peter Huewe, Jason Gunthorpe, Jerry Snitselaar,
open list:TPM DEVICE DRIVER, open list
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume
2026-08-03 9:13 [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume Thomas Fourier
2026-08-03 12:00 ` Jarkko Sakkinen
@ 2026-08-10 15:39 ` Jarkko Sakkinen
2026-08-11 0:03 ` Jarkko Sakkinen
2 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2026-08-10 15:39 UTC (permalink / raw)
To: Thomas Fourier
Cc: stable, Peter Huewe, Jason Gunthorpe, Jerry Snitselaar,
open list:TPM DEVICE DRIVER, open list
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume
2026-08-03 9:13 [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume Thomas Fourier
2026-08-03 12:00 ` Jarkko Sakkinen
2026-08-10 15:39 ` Jarkko Sakkinen
@ 2026-08-11 0:03 ` Jarkko Sakkinen
2026-08-21 9:40 ` Richard Lyu
2 siblings, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2026-08-11 0:03 UTC (permalink / raw)
To: Thomas Fourier
Cc: stable, Peter Huewe, Jason Gunthorpe, Jerry Snitselaar,
open list:TPM DEVICE DRIVER, open list
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] tpm: Fix barriers to prevent hwrng from activating during resume
2026-08-11 0:03 ` Jarkko Sakkinen
@ 2026-08-21 9:40 ` Richard Lyu
0 siblings, 0 replies; 5+ messages in thread
From: Richard Lyu @ 2026-08-21 9:40 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: Thomas Fourier, stable, Peter Huewe, Jason Gunthorpe,
Jerry Snitselaar, open list:TPM DEVICE DRIVER, open list
>> 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?
^ permalink raw reply [flat|nested] 5+ messages in thread