Re: [PATCH] tpm: Disable RNG for all AMD fTPMs

[Jarkko Sakkinen]

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