On Wed Aug 2, 2023 at 12:18 AM 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>
> ---
> drivers/char/tpm/tpm-chip.c | 80 +++++++------------------------------
> drivers/char/tpm/tpm_crb.c | 23 +++++++++++
> include/linux/tpm.h | 7 ++++
> 3 files changed, 44 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index cf5499e51999b..3154a5ef2611f 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -89,6 +89,14 @@ static void tpm_clk_disable(struct tpm_chip *chip)
> chip->ops->clk_enable(chip, false);
> }
>
> +static int tpm_check_flags(struct tpm_chip *chip)
> +{
> + if (!chip->ops->check_flags)
> + return 0;
> +
> + return chip->ops->check_flags(chip);
> +}
Instead of adding a new callback, you should probably do the following
in crb_acpi_add(), and get a much less complicated fix:
1. Call tpm_chip_bootstrap() (see tpm_tis_core_init()).
2. Set TPM_CHIP_FLAG_HWRNG_DISABLED if the conditions have been met.
3. Call tpm_chip_register().
> +
> /**
> * tpm_chip_start() - power on the TPM
> * @chip: a TPM chip to use
> @@ -510,70 +518,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 +532,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))
> + tpm_is_rng_disabled(chip))
> return 0;
>
> snprintf(chip->hwrng_name, sizeof(chip->hwrng_name),
> @@ -670,6 +614,10 @@ int tpm_chip_register(struct tpm_chip *chip)
> if (rc)
> return rc;
>
> + rc = tpm_check_flags(chip);
> + if (rc)
> + return rc;
> +
> tpm_sysfs_add_device(chip);
>
> tpm_bios_log_setup(chip);
> @@ -719,7 +667,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))
> + !tpm_is_rng_disabled(chip))
> 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..5614b68ef1905 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,
> @@ -476,6 +498,7 @@ static const struct tpm_class_ops tpm_crb = {
> .relinquish_locality = crb_relinquish_locality,
> .req_complete_mask = CRB_DRV_STS_COMPLETE,
> .req_complete_val = CRB_DRV_STS_COMPLETE,
> + .check_flags = crb_check_flags,
> };
>
> static int crb_check_resource(struct acpi_resource *ares, void *data)
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 6a1e8f1572551..aeb84944b0ca4 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -82,6 +82,7 @@ struct tpm_class_ops {
> int (*request_locality)(struct tpm_chip *chip, int loc);
> int (*relinquish_locality)(struct tpm_chip *chip, int loc);
> void (*clk_enable)(struct tpm_chip *chip, bool value);
> + int (*check_flags)(struct tpm_chip *chip);
> };
>
> #define TPM_NUM_EVENT_LOG_FILES 3
> @@ -283,6 +284,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)
> @@ -417,6 +419,11 @@ static inline u32 tpm2_rc_value(u32 rc)
> return (rc & BIT(7)) ? rc & 0xff : rc;
> }
>
> +static inline bool tpm_is_rng_disabled(struct tpm_chip *chip)
> +{
> + return chip->flags & TPM_CHIP_FLAG_HWRNG_DISABLED;
> +}
Please open code to the call sites.
Wrapping a flag check is not very useful and makes the bug fix
only more complex.
> +
> #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
>
> extern int tpm_is_tpm2(struct tpm_chip *chip);
> --
> 2.34.1
BR, Jarkko
