On Wed Aug 2, 2023 at 3:25 PM EEST, Mario Limonciello wrote:
> The TPM RNG functionality is not necessary for entropy when the CPU
> already supports the RDRAND instruction. The TPM RNG functionality
> was previously disabled on a subset of AMD fTPM series, but reports
> continue to show problems on some systems causing stutter root caused
> to TPM RNG functionality.
>
> Expand disabling TPM RNG use for all AMD fTPMs whether they have versions
> that claim to have fixed or not. To accomplish this, move the detection
> into part of the TPM CRB registration and add a flag indicating that
> the TPM should opt-out of registration to hwrng.
>
> Cc: stable@xxxxxxxxxxxxxxx # 6.1.y+
> Fixes: b006c439d58d ("hwrng: core - start hwrng kthread also for untrusted sources")
> Fixes: f1324bbc4011 ("tpm: disable hwrng for fTPM on some AMD designs")
> Reported-by: daniil.stas@xxxxxxxxxx
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217719
> Reported-by: bitlord0xff@xxxxxxxxx
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=217212
> Signed-off-by: Mario Limonciello <mario.limonciello@xxxxxxx>
> ---
> v1->v2:
> * switch from callback to everything in tpm_crb
> * switch to open coded flags check instead of new inline
> ---
> drivers/char/tpm/tpm-chip.c | 68 ++-----------------------------------
> drivers/char/tpm/tpm_crb.c | 30 ++++++++++++++++
> include/linux/tpm.h | 1 +
> 3 files changed, 33 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index cf5499e51999b..e904aae9771be 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -510,70 +510,6 @@ 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 +
> - */
> -#ifdef CONFIG_X86
> -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);
> -
> -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;
> - }
> -
> - dev_warn(&chip->dev,
> - "AMD fTPM version 0x%llx causes system stutter; hwrng disabled\n",
> - version);
> -
> - return true;
> -}
> -#else
> -static inline bool tpm_amd_is_rng_defective(struct tpm_chip *chip)
> -{
> - return false;
> -}
> -#endif /* CONFIG_X86 */
> -
> 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);
> @@ -588,7 +524,7 @@ static int tpm_hwrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
> static int tpm_add_hwrng(struct tpm_chip *chip)
> {
> if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM) || tpm_is_firmware_upgrade(chip) ||
> - tpm_amd_is_rng_defective(chip))
> + chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED)
> return 0;
>
> snprintf(chip->hwrng_name, sizeof(chip->hwrng_name),
> @@ -719,7 +655,7 @@ void tpm_chip_unregister(struct tpm_chip *chip)
> {
> tpm_del_legacy_sysfs(chip);
> if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !tpm_is_firmware_upgrade(chip) &&
> - !tpm_amd_is_rng_defective(chip))
> + !(chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED))
> hwrng_unregister(&chip->hwrng);
> tpm_bios_log_teardown(chip);
> if (chip->flags & TPM_CHIP_FLAG_TPM2 && !tpm_is_firmware_upgrade(chip))
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1a5d09b185134..9eb1a18590123 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -463,6 +463,28 @@ static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
> return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
> }
>
> +static int crb_check_flags(struct tpm_chip *chip)
> +{
> + u32 val;
> + int ret;
> +
> + ret = crb_request_locality(chip, 0);
> + if (ret)
> + return ret;
> +
> + ret = tpm2_get_tpm_pt(chip, TPM2_PT_MANUFACTURER, &val, NULL);
> + if (ret)
> + goto release;
> +
> + if (val == 0x414D4400U /* AMD */)
> + chip->flags |= TPM_CHIP_FLAG_HWRNG_DISABLED;
> +
> +release:
> + crb_relinquish_locality(chip, 0);
> +
> + return ret;
> +}
> +
> static const struct tpm_class_ops tpm_crb = {
> .flags = TPM_OPS_AUTO_STARTUP,
> .status = crb_status,
> @@ -800,6 +822,14 @@ static int crb_acpi_add(struct acpi_device *device)
> chip->acpi_dev_handle = device->handle;
> chip->flags = TPM_CHIP_FLAG_TPM2;
>
> + rc = tpm_chip_bootstrap(chip);
> + if (rc)
> + goto out;
> +
> + rc = crb_check_flags(chip);
> + if (rc)
> + goto out;
> +
> rc = tpm_chip_register(chip);
>
> out:
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 6a1e8f1572551..4ee9d13749adc 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -283,6 +283,7 @@ enum tpm_chip_flags {
> TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
> TPM_CHIP_FLAG_FIRMWARE_UPGRADE = BIT(7),
> TPM_CHIP_FLAG_SUSPENDED = BIT(8),
> + TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9),
> };
>
> #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
> --
> 2.34.1
Reviewed-by: Jarkko Sakkinen <jarkko@xxxxxxxxxx>
Thank you, great work.
I pushed the patch to my next branch:
https://git.kernel.org/pub/scm/linux/kernel/git/jarkko/linux-tpmdd.git/log/?h=next
I'll hold on for tested-by's from AMD users, and send a pull
request tomorrow afternoon (GMT+3).
BR, Jarkko
