1. Patchew
  2. linux
  3. View series

[PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first

Jarkko Sakkinen posted 1 patch 3 weeks, 2 days ago 
drivers/char/tpm/tpm-chip.c      |  4 ----
drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
2 files changed, 22 insertions(+), 14 deletions(-)
[PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jarkko Sakkinen 3 weeks, 2 days ago 
Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
according, as this leaves window for tpm_hwrng_read() to be called while
the operation is in progress. The recent bug report gives also evidence of
this behaviour.

Aadress this by locking the TPM chip before checking any chip->flags both
in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
check inside tpm_get_random() so that it will be always checked only when
the lock is reserved.

Cc: stable@vger.kernel.org # v6.4+
Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
v3:
- Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
  also done under the lock (suggested by Jerry Snitselaar).
v2:
- Addressed my own remark:
  https://lore.kernel.org/linux-integrity/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/
---
 drivers/char/tpm/tpm-chip.c      |  4 ----
 drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1ff99a7091bb..7df7abaf3e52 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 {
 	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
 
-	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
-	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
-		return 0;
-
 	return tpm_get_random(chip, data, max);
 }
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 8134f002b121..b1daa0d7b341 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
 	if (!chip)
 		return -ENODEV;
 
+	rc = tpm_try_get_ops(chip);
+	if (rc) {
+		/* Can be safely set out of locks, as no action cannot race: */
+		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
 		goto suspended;
 
@@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
 	    !pm_suspend_via_firmware())
 		goto suspended;
 
-	rc = tpm_try_get_ops(chip);
-	if (!rc) {
-		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-			tpm2_end_auth_session(chip);
-			tpm2_shutdown(chip, TPM2_SU_STATE);
-		} else {
-			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
-		}
-
-		tpm_put_ops(chip);
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		tpm2_end_auth_session(chip);
+		tpm2_shutdown(chip, TPM2_SU_STATE);
+		goto suspended;
 	}
 
+	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
+
 suspended:
 	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
+	tpm_put_ops(chip);
 
+out:
 	if (rc)
 		dev_err(dev, "Ignoring error %d while suspending\n", rc);
 	return 0;
@@ -440,11 +445,18 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	if (!chip)
 		return -ENODEV;
 
+	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
+	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED) {
+		rc = 0;
+		goto out;
+	}
+
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
 		rc = tpm2_get_random(chip, out, max);
 	else
 		rc = tpm1_get_random(chip, out, max);
 
+out:
 	tpm_put_ops(chip);
 	return rc;
 }
-- 
2.47.0
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jerry Snitselaar 3 weeks, 2 days ago 
On Fri, Nov 01, 2024 at 02:21:56AM +0200, Jarkko Sakkinen wrote:
> Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> according, as this leaves window for tpm_hwrng_read() to be called while
> the operation is in progress. The recent bug report gives also evidence of
> this behaviour.
> 
> Aadress this by locking the TPM chip before checking any chip->flags both
> in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> check inside tpm_get_random() so that it will be always checked only when
> the lock is reserved.
> 
> Cc: stable@vger.kernel.org # v6.4+
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> v3:
> - Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
>   also done under the lock (suggested by Jerry Snitselaar).
> v2:
> - Addressed my own remark:
>   https://lore.kernel.org/linux-integrity/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/
> ---
>  drivers/char/tpm/tpm-chip.c      |  4 ----
>  drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
>  2 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 1ff99a7091bb..7df7abaf3e52 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
>  {
>  	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
>  
> -	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
> -	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> -		return 0;
> -
>  	return tpm_get_random(chip, data, max);
>  }
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 8134f002b121..b1daa0d7b341 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
>  	if (!chip)
>  		return -ENODEV;
>  
> +	rc = tpm_try_get_ops(chip);
> +	if (rc) {
> +		/* Can be safely set out of locks, as no action cannot race: */
> +		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> +		goto out;
> +	}
> +
>  	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
>  		goto suspended;
>  
> @@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
>  	    !pm_suspend_via_firmware())
>  		goto suspended;
>  
> -	rc = tpm_try_get_ops(chip);
> -	if (!rc) {
> -		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -			tpm2_end_auth_session(chip);
> -			tpm2_shutdown(chip, TPM2_SU_STATE);
> -		} else {
> -			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> -		}
> -
> -		tpm_put_ops(chip);
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		tpm2_end_auth_session(chip);
> +		tpm2_shutdown(chip, TPM2_SU_STATE);
> +		goto suspended;
>  	}
>  
> +	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> +


I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2)
otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?

So:

	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
		tpm2_end_auth_session(chip);
		tpm2_shutdown(chip, TPM2_SU_STATE);
		goto suspended;
	} else {
		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
	}


Other than that I think it looks good.


>  suspended:
>  	chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> +	tpm_put_ops(chip);
>  
> +out:
>  	if (rc)
>  		dev_err(dev, "Ignoring error %d while suspending\n", rc);
>  	return 0;
> @@ -440,11 +445,18 @@ int tpm_get_random(struct tpm_chip *chip, u8 *out, size_t max)
>  	if (!chip)
>  		return -ENODEV;
>  
> +	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
> +	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED) {
> +		rc = 0;
> +		goto out;
> +	}
> +
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>  		rc = tpm2_get_random(chip, out, max);
>  	else
>  		rc = tpm1_get_random(chip, out, max);
>  
> +out:
>  	tpm_put_ops(chip);
>  	return rc;
>  }
> -- 
> 2.47.0
>
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jarkko Sakkinen 3 weeks, 2 days ago 
On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote:
> On Fri, Nov 01, 2024 at 02:21:56AM +0200, Jarkko Sakkinen wrote:
> > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> > according, as this leaves window for tpm_hwrng_read() to be called while
> > the operation is in progress. The recent bug report gives also evidence of
> > this behaviour.
> > 
> > Aadress this by locking the TPM chip before checking any chip->flags both
> > in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> > check inside tpm_get_random() so that it will be always checked only when
> > the lock is reserved.
> > 
> > Cc: stable@vger.kernel.org # v6.4+
> > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> > Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > v3:
> > - Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
> >   also done under the lock (suggested by Jerry Snitselaar).
> > v2:
> > - Addressed my own remark:
> >   https://lore.kernel.org/linux-integrity/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/
> > ---
> >  drivers/char/tpm/tpm-chip.c      |  4 ----
> >  drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
> >  2 files changed, 22 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > index 1ff99a7091bb..7df7abaf3e52 100644
> > --- a/drivers/char/tpm/tpm-chip.c
> > +++ b/drivers/char/tpm/tpm-chip.c
> > @@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> >  {
> >  	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
> >  
> > -	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
> > -	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> > -		return 0;
> > -
> >  	return tpm_get_random(chip, data, max);
> >  }
> >  
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 8134f002b121..b1daa0d7b341 100644
> > --- a/drivers/char/tpm/tpm-interface.c
> > +++ b/drivers/char/tpm/tpm-interface.c
> > @@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
> >  	if (!chip)
> >  		return -ENODEV;
> >  
> > +	rc = tpm_try_get_ops(chip);
> > +	if (rc) {
> > +		/* Can be safely set out of locks, as no action cannot race: */
> > +		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> > +		goto out;
> > +	}
> > +
> >  	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
> >  		goto suspended;
> >  
> > @@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
> >  	    !pm_suspend_via_firmware())
> >  		goto suspended;
> >  
> > -	rc = tpm_try_get_ops(chip);
> > -	if (!rc) {
> > -		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > -			tpm2_end_auth_session(chip);
> > -			tpm2_shutdown(chip, TPM2_SU_STATE);
> > -		} else {
> > -			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > -		}
> > -
> > -		tpm_put_ops(chip);
> > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > +		tpm2_end_auth_session(chip);
> > +		tpm2_shutdown(chip, TPM2_SU_STATE);
> > +		goto suspended;
> >  	}
> >  
> > +	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > +
>
>
> I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2)
> otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?
>
> So:
>
> 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> 		tpm2_end_auth_session(chip);
> 		tpm2_shutdown(chip, TPM2_SU_STATE);
> 		goto suspended;
> 	} else {
> 		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> 	}
>
>
> Other than that I think it looks good.

It should be fine because after tpm2_shutdown() is called there is "goto
suspended;". This is IMHO more readable as it matches the structure of
previous exits before it. In future if this needs to be improved it will
easier to move the logic to a helper function (e.g. __tpm_pm_suspend())
where gotos are substituted with return-statements.

BR, Jarkko
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jerry Snitselaar 3 weeks, 2 days ago 
On Fri, Nov 01, 2024 at 11:07:15PM +0200, Jarkko Sakkinen wrote:
> On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote:
> > On Fri, Nov 01, 2024 at 02:21:56AM +0200, Jarkko Sakkinen wrote:
> > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> > > according, as this leaves window for tpm_hwrng_read() to be called while
> > > the operation is in progress. The recent bug report gives also evidence of
> > > this behaviour.
> > > 
> > > Aadress this by locking the TPM chip before checking any chip->flags both
> > > in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> > > check inside tpm_get_random() so that it will be always checked only when
> > > the lock is reserved.
> > > 
> > > Cc: stable@vger.kernel.org # v6.4+
> > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
> > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > ---
> > > v3:
> > > - Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
> > >   also done under the lock (suggested by Jerry Snitselaar).
> > > v2:
> > > - Addressed my own remark:
> > >   https://lore.kernel.org/linux-integrity/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/
> > > ---
> > >  drivers/char/tpm/tpm-chip.c      |  4 ----
> > >  drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
> > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 1ff99a7091bb..7df7abaf3e52 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> > >  {
> > >  	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
> > >  
> > > -	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
> > > -	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> > > -		return 0;
> > > -
> > >  	return tpm_get_random(chip, data, max);
> > >  }
> > >  
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 8134f002b121..b1daa0d7b341 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
> > >  	if (!chip)
> > >  		return -ENODEV;
> > >  
> > > +	rc = tpm_try_get_ops(chip);
> > > +	if (rc) {
> > > +		/* Can be safely set out of locks, as no action cannot race: */
> > > +		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> > > +		goto out;
> > > +	}
> > > +
> > >  	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
> > >  		goto suspended;
> > >  
> > > @@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
> > >  	    !pm_suspend_via_firmware())
> > >  		goto suspended;
> > >  
> > > -	rc = tpm_try_get_ops(chip);
> > > -	if (!rc) {
> > > -		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > -			tpm2_end_auth_session(chip);
> > > -			tpm2_shutdown(chip, TPM2_SU_STATE);
> > > -		} else {
> > > -			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > > -		}
> > > -
> > > -		tpm_put_ops(chip);
> > > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > +		tpm2_end_auth_session(chip);
> > > +		tpm2_shutdown(chip, TPM2_SU_STATE);
> > > +		goto suspended;
> > >  	}
> > >  
> > > +	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > > +
> >
> >
> > I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?
> >
> > So:
> >
> > 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > 		tpm2_end_auth_session(chip);
> > 		tpm2_shutdown(chip, TPM2_SU_STATE);
> > 		goto suspended;
> > 	} else {
> > 		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > 	}
> >
> >
> > Other than that I think it looks good.
> 
> It should be fine because after tpm2_shutdown() is called there is "goto
> suspended;". This is IMHO more readable as it matches the structure of
> previous exits before it. In future if this needs to be improved it will
> easier to move the logic to a helper function (e.g. __tpm_pm_suspend())
> where gotos are substituted with return-statements.
> 
> BR, Jarkko
> 

Heh, yep.

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jarkko Sakkinen 3 weeks, 2 days ago 
On Fri Nov 1, 2024 at 11:09 PM EET, Jerry Snitselaar wrote:
> On Fri, Nov 01, 2024 at 11:07:15PM +0200, Jarkko Sakkinen wrote:
> > On Fri Nov 1, 2024 at 10:23 PM EET, Jerry Snitselaar wrote:
> > > On Fri, Nov 01, 2024 at 02:21:56AM +0200, Jarkko Sakkinen wrote:
> > > > Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> > > > according, as this leaves window for tpm_hwrng_read() to be called while
> > > > the operation is in progress. The recent bug report gives also evidence of
> > > > this behaviour.
> > > > 
> > > > Aadress this by locking the TPM chip before checking any chip->flags both
> > > > in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> > > > check inside tpm_get_random() so that it will be always checked only when
> > > > the lock is reserved.
> > > > 
> > > > Cc: stable@vger.kernel.org # v6.4+
> > > > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> > > > Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
> > > > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > > v3:
> > > > - Check TPM_CHIP_FLAG_SUSPENDED inside tpm_get_random() so that it is
> > > >   also done under the lock (suggested by Jerry Snitselaar).
> > > > v2:
> > > > - Addressed my own remark:
> > > >   https://lore.kernel.org/linux-integrity/D59JAI6RR2CD.G5E5T4ZCZ49W@kernel.org/
> > > > ---
> > > >  drivers/char/tpm/tpm-chip.c      |  4 ----
> > > >  drivers/char/tpm/tpm-interface.c | 32 ++++++++++++++++++++++----------
> > > >  2 files changed, 22 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > > index 1ff99a7091bb..7df7abaf3e52 100644
> > > > --- a/drivers/char/tpm/tpm-chip.c
> > > > +++ b/drivers/char/tpm/tpm-chip.c
> > > > @@ -525,10 +525,6 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> > > >  {
> > > >  	struct tpm_chip *chip = container_of(rng, struct tpm_chip, hwrng);
> > > >  
> > > > -	/* Give back zero bytes, as TPM chip has not yet fully resumed: */
> > > > -	if (chip->flags & TPM_CHIP_FLAG_SUSPENDED)
> > > > -		return 0;
> > > > -
> > > >  	return tpm_get_random(chip, data, max);
> > > >  }
> > > >  
> > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > index 8134f002b121..b1daa0d7b341 100644
> > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > @@ -370,6 +370,13 @@ int tpm_pm_suspend(struct device *dev)
> > > >  	if (!chip)
> > > >  		return -ENODEV;
> > > >  
> > > > +	rc = tpm_try_get_ops(chip);
> > > > +	if (rc) {
> > > > +		/* Can be safely set out of locks, as no action cannot race: */
> > > > +		chip->flags |= TPM_CHIP_FLAG_SUSPENDED;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > >  	if (chip->flags & TPM_CHIP_FLAG_ALWAYS_POWERED)
> > > >  		goto suspended;
> > > >  
> > > > @@ -377,21 +384,19 @@ int tpm_pm_suspend(struct device *dev)
> > > >  	    !pm_suspend_via_firmware())
> > > >  		goto suspended;
> > > >  
> > > > -	rc = tpm_try_get_ops(chip);
> > > > -	if (!rc) {
> > > > -		if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > > -			tpm2_end_auth_session(chip);
> > > > -			tpm2_shutdown(chip, TPM2_SU_STATE);
> > > > -		} else {
> > > > -			rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > > > -		}
> > > > -
> > > > -		tpm_put_ops(chip);
> > > > +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > > +		tpm2_end_auth_session(chip);
> > > > +		tpm2_shutdown(chip, TPM2_SU_STATE);
> > > > +		goto suspended;
> > > >  	}
> > > >  
> > > > +	rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > > > +
> > >
> > >
> > > I imagine the above still be wrapped in an else with the if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > > otherwise it will call tpm1_pm_suspend for both tpm1 and tpm2 devices, yes?
> > >
> > > So:
> > >
> > > 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > 		tpm2_end_auth_session(chip);
> > > 		tpm2_shutdown(chip, TPM2_SU_STATE);
> > > 		goto suspended;
> > > 	} else {
> > > 		rc = tpm1_pm_suspend(chip, tpm_suspend_pcr);
> > > 	}
> > >
> > >
> > > Other than that I think it looks good.
> > 
> > It should be fine because after tpm2_shutdown() is called there is "goto
> > suspended;". This is IMHO more readable as it matches the structure of
> > previous exits before it. In future if this needs to be improved it will
> > easier to move the logic to a helper function (e.g. __tpm_pm_suspend())
> > where gotos are substituted with return-statements.
> > 
> > BR, Jarkko
> > 
>
> Heh, yep.
>
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Thanks!

BR, Jarkko
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jarkko Sakkinen 3 weeks, 2 days ago 
On Fri Nov 1, 2024 at 2:21 AM EET, Jarkko Sakkinen wrote:
> Setting TPM_CHIP_FLAG_SUSPENDED in the end of tpm_pm_suspend() can be racy
> according, as this leaves window for tpm_hwrng_read() to be called while
> the operation is in progress. The recent bug report gives also evidence of
> this behaviour.
>
> Aadress this by locking the TPM chip before checking any chip->flags both
> in tpm_pm_suspend() and tpm_hwrng_read(). Move TPM_CHIP_FLAG_SUSPENDED
> check inside tpm_get_random() so that it will be always checked only when
> the lock is reserved.
>
> Cc: stable@vger.kernel.org # v6.4+
> Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> Reported-by: Mike Seo <mikeseohyungjin@gmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>

A basic smoke test in QEMU:

# rtcwake -m mem -s 15
rtcwake -m mem -s 15
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Fri Nov  1 02:21:06 2024
PM: suspend entry (deep)
Filesystems sync: 0.017 seconds
Freezing user space processes
Freezing user space processes completed (elapsed 0.004 seconds)
OOM killer disabled.
Freezing remaining freezable tasks
Freezing remaining freezable tasks completed (elapsed 0.004 seconds)
printk: Suspending console(s) (use no_console_suspend to debug)
ata2.00: Check power mode failed (err_mask=0x1)
ACPI: PM: Preparing to enter system sleep state S3
ACPI: PM: Saving platform NVS memory
Disabling non-boot CPUs ...
ACPI: PM: Low-level resume complete
ACPI: PM: Restoring platform NVS memory
ACPI: PM: Waking up from system sleep state S3
pci 0000:00:01.0: PIIX3: Enabling Passive Release
virtio_blk virtio1: 1/0/0 default/read/poll queues
OOM killer enabled.
Restarting tasks ... done.
random: crng reseeded on system resumption
PM: suspend exit
# ata2: found unknown device (class 0)

# dmesg|tail -20
dmesg|tail -20
[   28.199150] Freezing user space processes
[   28.205393] Freezing user space processes completed (elapsed 0.004
seconds)
[   28.206780] OOM killer disabled.
[   28.207858] Freezing remaining freezable tasks
[   28.213224] Freezing remaining freezable tasks completed (elapsed
0.004 seconds)
[   28.214591] printk: Suspending console(s) (use no_console_suspend to
debug)
[   28.222203] ata2.00: Check power mode failed (err_mask=0x1)
[   28.240808] ACPI: PM: Preparing to enter system sleep state S3
[   28.241218] ACPI: PM: Saving platform NVS memory
[   28.241390] Disabling non-boot CPUs ...
[   28.243011] ACPI: PM: Low-level resume complete
[   28.243273] ACPI: PM: Restoring platform NVS memory
[   28.246191] ACPI: PM: Waking up from system sleep state S3
[   28.250415] pci 0000:00:01.0: PIIX3: Enabling Passive Release
[   28.256539] virtio_blk virtio1: 1/0/0 default/read/poll queues
[   28.280715] OOM killer enabled.
[   28.281766] Restarting tasks ... done.
[   28.287096] random: crng reseeded on system resumption
[   28.288181] PM: suspend exit
[   28.410073] ata2: found unknown device (class 0)

Testing done with https://codeberg.org/jarkko/linux-tpmdd-test

cmake -Bbuild -Dbuildroot_defconfig=busybox_x86_64_defconfig && make -Cbuild buildroot-prepare
make -Cbuild/buildroot/build
pushd build/buildroot/build
images/run-qemu.sh &
socat - UNIX-CONNECT:images/serial.sock

BR, Jarkko
Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first
Posted by Jarkko Sakkinen 3 weeks, 2 days ago 
On Fri Nov 1, 2024 at 2:25 AM EET, Jarkko Sakkinen wrote:
> cmake -Bbuild -Dbuildroot_defconfig=busybox_x86_64_defconfig && make -Cbuild buildroot-prepare
> make -Cbuild/buildroot/build
> pushd build/buildroot/build
> images/run-qemu.sh &
> socat - UNIX-CONNECT:images/serial.sock

and export LINUX_OVERRIDE_SRCDIR=/home/jarkko/work/kernel.org/jarkko/linux-tpmdd

<offtopic>
I wondered what was the thing anyway with those "kernel patches for
Github/lab" discussed in LWN while ago. If you know how to propeerly
use BuildRoot, CI compatibility is and old thing. This has been fully
tested to run also inside CI (and has run-tests.sh based on TCL's
utility expect). How I understood that article was lack of knowledge
of the tools available. Hope none of those kernel patches never
landed tbh...
</offtopic>

BR, Jarkko

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.