Re: [PATCH v2] tpm: disable hwrng for fTPM on some AMD designs

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Feb 27, 2023 at 08:58:37AM -0600, Mario Limonciello wrote:
> On 2/27/23 08:55, Guenter Roeck wrote:
> > On Mon, Feb 20, 2023 at 12:07:28PM -0600, Mario Limonciello wrote:
> > > AMD has issued an advisory indicating that having fTPM enabled in
> > > BIOS can cause "stuttering" in the OS.  This issue has been fixed
> > > in newer versions of the fTPM firmware, but it's up to system
> > > designers to decide whether to distribute it.
> > > 
> > > This issue has existed for a while, but is more prevalent starting
> > > with kernel 6.1 because commit b006c439d58db ("hwrng: core - start
> > > hwrng kthread also for untrusted sources") started to use the fTPM
> > > for hwrng by default. However, all uses of /dev/hwrng result in
> > > unacceptable stuttering.
> > > 
> > > So, simply disable registration of the defective hwrng when detecting
> > > these faulty fTPM versions.  As this is caused by faulty firmware, it
> > > is plausible that such a problem could also be reproduced by other TPM
> > > interactions, but this hasn't been shown by any user's testing or reports.
> > > 
> > > It is hypothesized to be triggered more frequently by the use of the RNG
> > > because userspace software will fetch random numbers regularly.
> > > 
> > > Intentionally continue to register other TPM functionality so that users
> > > that rely upon PCR measurements or any storage of data will still have
> > > access to it.  If it's found later that another TPM functionality is
> > > exacerbating this problem a module parameter it can be turned off entirely
> > > and a module parameter can be introduced to allow users who rely upon
> > > fTPM functionality to turn it on even though this problem is present.
> > > 
> > > Link: https://www.amd.com/en/support/kb/faq/pa-410
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=216989
> > > Link: https://lore.kernel.org/all/20230209153120.261904-1-Jason@zx2c4.com/
> > > Fixes: b006c439d58d ("hwrng: core - start hwrng kthread also for untrusted sources")
> > > Cc: stable@vger.kernel.org
> > > Cc: Jarkko Sakkinen <jarkko@kernel.org>
> > > Cc: Thorsten Leemhuis <regressions@leemhuis.info>
> > > Cc: James Bottomley <James.Bottomley@hansenpartnership.com>
> > > Co-developed-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > > ---
> > > v1->v2:
> > >   * Minor style from Jarkko's feedback
> > >   * Move comment above function
> > >   * Explain further in commit message
> > > ---
> > >   drivers/char/tpm/tpm-chip.c | 61 ++++++++++++++++++++++++++++++-
> > >   drivers/char/tpm/tpm.h      | 73 +++++++++++++++++++++++++++++++++++++
> > >   2 files changed, 133 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 741d8f3e8fb3..1b066d7a6e21 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -512,6 +512,64 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip)
> > >   	return 0;
> > >   }
> > > +/*
> > > + * Some AMD fTPM versions may cause stutter
> > > + * https://www.amd.com/en/support/kb/faq/pa-410
> > > + *
> > > + * Fixes are available in two series of fTPM firmware:
> > > + * 6.x.y.z series: 6.0.18.6 +
> > > + * 3.x.y.z series: 3.57.y.5 +
> > > + */
> > > +static bool tpm_amd_is_rng_defective(struct tpm_chip *chip)
> > > +{
> > > +	u32 val1, val2;
> > > +	u64 version;
> > > +	int ret;
> > > +
> > > +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
> > > +		return false;
> > > +
> > > +	ret = tpm_request_locality(chip);
> > > +	if (ret)
> > > +		return false;
> > > +
> > > +	ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val1, NULL);
> > > +	if (ret)
> > > +		goto release;
> > > +	if (val1 != 0x414D4400U /* AMD */) {
> > > +		ret = -ENODEV;
> > > +		goto release;
> > > +	}
> > > +	ret = tpm2_get_tpm_pt(chip, TPM2_PT_FIRMWARE_VERSION_1, &val1, NULL);
> > > +	if (ret)
> > > +		goto release;
> > > +	ret = tpm2_get_tpm_pt(chip, TPM2_PT_FIRMWARE_VERSION_2, &val2, NULL);
> > > +	if (ret)
> > > +		goto release;
> > 
> > This goto is unnecessary.
> > 
> > > +
> > > +release:
> > > +	tpm_relinquish_locality(chip);
> > > +
> > > +	if (ret)
> > > +		return false;
> > > +
> > > +	version = ((u64)val1 << 32) | val2;
> > > +	if ((version >> 48) == 6) {
> > > +		if (version >= 0x0006000000180006ULL)
> > > +			return false;
> > > +	} else if ((version >> 48) == 3) {
> > > +		if (version >= 0x0003005700000005ULL)
> > > +			return false;
> > > +	} else
> > > +		return false;
> > 
> > checkpatch:
> > 
> > CHECK: braces {} should be used on all arms of this statement
> > #200: FILE: drivers/char/tpm/tpm-chip.c:557:
> > +	if ((version >> 48) == 6) {
> > [...]
> > +	} else if ((version >> 48) == 3) {
> > [...]
> > +	} else
> > [...]
> 
> It was requested by Jarko explicitly in v1 to do it this way.
> 
> https://lore.kernel.org/lkml/Y+%2F6G+UlTI7GpW6o@kernel.org/

OK, you're right, it is my bad, I'm sorry about that. Send v3 with that feedback reverted.

BR, Jarkko