linux-kernel - Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <D5B5D47GLWWS.119EDSKMMGFVF@kernel.org>
Date: Fri, 01 Nov 2024 23:07:15 +0200
From: "Jarkko Sakkinen" <jarkko@...nel.org>
To: "Jerry Snitselaar" <jsnitsel@...hat.com>
Cc: "Peter Huewe" <peterhuewe@....de>, "Jason Gunthorpe" <jgg@...pe.ca>,
 <stable@...r.kernel.org>, "Mike Seo" <mikeseohyungjin@...il.com>, "open
 list:TPM DEVICE DRIVER" <linux-integrity@...r.kernel.org>, "open list"
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] tpm: Lock TPM chip in tpm_pm_suspend() first

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@...r.kernel.org # v6.4+
> > Fixes: 99d464506255 ("tpm: Prevent hwrng from activating during resume")
> > Reported-by: Mike Seo <mikeseohyungjin@...il.com>
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219383
> > Signed-off-by: Jarkko Sakkinen <jarkko@...nel.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