Date:   Mon, 27 Feb 2023 08:58:37 -0600
From:   Mario Limonciello <mario.limonciello@....com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Peter Huewe <peterhuewe@....de>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Dominik Brodowski <linux@...inikbrodowski.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        stable@...r.kernel.org,
        Thorsten Leemhuis <regressions@...mhuis.info>,
        James Bottomley <James.Bottomley@...senpartnership.com>,
        "Jason A . Donenfeld" <Jason@...c4.com>,
        linux-integrity@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] tpm: disable hwrng for fTPM on some AMD designs

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@...r.kernel.org
>> Cc: Jarkko Sakkinen <jarkko@...nel.org>
>> Cc: Thorsten Leemhuis <regressions@...mhuis.info>
>> Cc: James Bottomley <James.Bottomley@...senpartnership.com>
>> Co-developed-by: Jason A. Donenfeld <Jason@...c4.com>
>> Signed-off-by: Jason A. Donenfeld <Jason@...c4.com>
>> Signed-off-by: Mario Limonciello <mario.limonciello@....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/

> 
>> +
>> +	dev_warn(&chip->dev,
>> +		 "AMD fTPM version 0x%llx causes system stutter; hwrng disabled\n",
>> +		 version);
>> +
>> +	return true;
>> +}
>> +
>>   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);
>> @@ -521,7 +579,8 @@ 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))
>> +	if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM) || tpm_is_firmware_upgrade(chip) ||
>> +	    tpm_amd_is_rng_defective(chip))
>>   		return 0;
>>   
>>   	snprintf(chip->hwrng_name, sizeof(chip->hwrng_name),
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 24ee4e1cc452..830014a26609 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -150,6 +150,79 @@ enum tpm_sub_capabilities {
>>   	TPM_CAP_PROP_TIS_DURATION = 0x120,
>>   };
>>   
>> +enum tpm2_pt_props {
>> +	TPM2_PT_NONE = 0x00000000,
>> +	TPM2_PT_GROUP = 0x00000100,
>> +	TPM2_PT_FIXED = TPM2_PT_GROUP * 1,
>> +	TPM2_PT_FAMILY_INDICATOR = TPM2_PT_FIXED + 0,
>> +	TPM2_PT_LEVEL = TPM2_PT_FIXED + 1,
>> +	TPM2_PT_REVISION = TPM2_PT_FIXED + 2,
>> +	TPM2_PT_DAY_OF_YEAR = TPM2_PT_FIXED + 3,
>> +	TPM2_PT_YEAR = TPM2_PT_FIXED + 4,
>> +	TPM2_PT_MANUFACTURER = TPM2_PT_FIXED + 5,
>> +	TPM2_PT_VENDOR_STRING_1 = TPM2_PT_FIXED + 6,
>> +	TPM2_PT_VENDOR_STRING_2 = TPM2_PT_FIXED + 7,
>> +	TPM2_PT_VENDOR_STRING_3 = TPM2_PT_FIXED + 8,
>> +	TPM2_PT_VENDOR_STRING_4 = TPM2_PT_FIXED + 9,
>> +	TPM2_PT_VENDOR_TPM_TYPE = TPM2_PT_FIXED + 10,
>> +	TPM2_PT_FIRMWARE_VERSION_1 = TPM2_PT_FIXED + 11,
>> +	TPM2_PT_FIRMWARE_VERSION_2 = TPM2_PT_FIXED + 12,
>> +	TPM2_PT_INPUT_BUFFER = TPM2_PT_FIXED + 13,
>> +	TPM2_PT_HR_TRANSIENT_MIN = TPM2_PT_FIXED + 14,
>> +	TPM2_PT_HR_PERSISTENT_MIN = TPM2_PT_FIXED + 15,
>> +	TPM2_PT_HR_LOADED_MIN = TPM2_PT_FIXED + 16,
>> +	TPM2_PT_ACTIVE_SESSIONS_MAX = TPM2_PT_FIXED + 17,
>> +	TPM2_PT_PCR_COUNT = TPM2_PT_FIXED + 18,
>> +	TPM2_PT_PCR_SELECT_MIN = TPM2_PT_FIXED + 19,
>> +	TPM2_PT_CONTEXT_GAP_MAX = TPM2_PT_FIXED + 20,
>> +	TPM2_PT_NV_COUNTERS_MAX = TPM2_PT_FIXED + 22,
>> +	TPM2_PT_NV_INDEX_MAX = TPM2_PT_FIXED + 23,
>> +	TPM2_PT_MEMORY = TPM2_PT_FIXED + 24,
>> +	TPM2_PT_CLOCK_UPDATE = TPM2_PT_FIXED + 25,
>> +	TPM2_PT_CONTEXT_HASH = TPM2_PT_FIXED + 26,
>> +	TPM2_PT_CONTEXT_SYM = TPM2_PT_FIXED + 27,
>> +	TPM2_PT_CONTEXT_SYM_SIZE = TPM2_PT_FIXED + 28,
>> +	TPM2_PT_ORDERLY_COUNT = TPM2_PT_FIXED + 29,
>> +	TPM2_PT_MAX_COMMAND_SIZE = TPM2_PT_FIXED + 30,
>> +	TPM2_PT_MAX_RESPONSE_SIZE = TPM2_PT_FIXED + 31,
>> +	TPM2_PT_MAX_DIGEST = TPM2_PT_FIXED + 32,
>> +	TPM2_PT_MAX_OBJECT_CONTEXT = TPM2_PT_FIXED + 33,
>> +	TPM2_PT_MAX_SESSION_CONTEXT = TPM2_PT_FIXED + 34,
>> +	TPM2_PT_PS_FAMILY_INDICATOR = TPM2_PT_FIXED + 35,
>> +	TPM2_PT_PS_LEVEL = TPM2_PT_FIXED + 36,
>> +	TPM2_PT_PS_REVISION = TPM2_PT_FIXED + 37,
>> +	TPM2_PT_PS_DAY_OF_YEAR = TPM2_PT_FIXED + 38,
>> +	TPM2_PT_PS_YEAR = TPM2_PT_FIXED + 39,
>> +	TPM2_PT_SPLIT_MAX = TPM2_PT_FIXED + 40,
>> +	TPM2_PT_TOTAL_COMMANDS = TPM2_PT_FIXED + 41,
>> +	TPM2_PT_LIBRARY_COMMANDS = TPM2_PT_FIXED + 42,
>> +	TPM2_PT_VENDOR_COMMANDS = TPM2_PT_FIXED + 43,
>> +	TPM2_PT_NV_BUFFER_MAX = TPM2_PT_FIXED + 44,
>> +	TPM2_PT_MODES = TPM2_PT_FIXED + 45,
>> +	TPM2_PT_MAX_CAP_BUFFER = TPM2_PT_FIXED + 46,
>> +	TPM2_PT_VAR = TPM2_PT_GROUP * 2,
>> +	TPM2_PT_PERMANENT = TPM2_PT_VAR + 0,
>> +	TPM2_PT_STARTUP_CLEAR = TPM2_PT_VAR + 1,
>> +	TPM2_PT_HR_NV_INDEX = TPM2_PT_VAR + 2,
>> +	TPM2_PT_HR_LOADED = TPM2_PT_VAR + 3,
>> +	TPM2_PT_HR_LOADED_AVAIL = TPM2_PT_VAR + 4,
>> +	TPM2_PT_HR_ACTIVE = TPM2_PT_VAR + 5,
>> +	TPM2_PT_HR_ACTIVE_AVAIL = TPM2_PT_VAR + 6,
>> +	TPM2_PT_HR_TRANSIENT_AVAIL = TPM2_PT_VAR + 7,
>> +	TPM2_PT_HR_PERSISTENT = TPM2_PT_VAR + 8,
>> +	TPM2_PT_HR_PERSISTENT_AVAIL = TPM2_PT_VAR + 9,
>> +	TPM2_PT_NV_COUNTERS = TPM2_PT_VAR + 10,
>> +	TPM2_PT_NV_COUNTERS_AVAIL = TPM2_PT_VAR + 11,
>> +	TPM2_PT_ALGORITHM_SET = TPM2_PT_VAR + 12,
>> +	TPM2_PT_LOADED_CURVES = TPM2_PT_VAR + 13,
>> +	TPM2_PT_LOCKOUT_COUNTER = TPM2_PT_VAR + 14,
>> +	TPM2_PT_MAX_AUTH_FAIL = TPM2_PT_VAR + 15,
>> +	TPM2_PT_LOCKOUT_INTERVAL = TPM2_PT_VAR + 16,
>> +	TPM2_PT_LOCKOUT_RECOVERY = TPM2_PT_VAR + 17,
>> +	TPM2_PT_NV_WRITE_RECOVERY = TPM2_PT_VAR + 18,
>> +	TPM2_PT_AUDIT_COUNTER_0 = TPM2_PT_VAR + 19,
>> +	TPM2_PT_AUDIT_COUNTER_1 = TPM2_PT_VAR + 20,
>> +};
>>   
>>   /* 128 bytes is an arbitrary cap. This could be as large as TPM_BUFSIZE - 18
>>    * bytes, but 128 is still a relatively large number of random bytes and
>> -- 
>> 2.34.1
>>

Powered by blists - more mailing lists