Date:   Wed, 8 Dec 2021 10:30:06 +0100
From:   Axel Jonsson <axel.jonsson@...s.com>
To:     <jarkko@...nel.org>
CC:     <sergio.ruiz@...s.com>, <borysmn@...s.com>, <jgg@...pe.ca>,
        <kernel@...s.com>, <linux-integrity@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <peterhuewe@....de>
Subject: [PATCH v6] tpm: Add Upgrade/Reduced mode support for TPM2 modules

Hello Jarkko!

I am Axel, a colleague to Borys, and will take over the integration of this
patch since Borys is too busy with other work.

I am sorry for the long delay since the last update.

>> If something went wrong during the TPM firmware upgrade, like power
>> failure or the firmware image file get corrupted, the TPM might end
>> up in Upgrade or Failure mode upon the next start. The state is
>> persistent between the TPM power cycle/restart.
>>=20
>> According to TPM specification:
>>  * If the TPM is in Upgrade mode, it will answer with TPM2_RC_UPGRADE
>>    to all commands except Field Upgrade related ones.
> 
> How many such command are there and what they are? This patch is missing
> the part that filters in only these commands.

The documentation states:

"If the proper authorization is given, the TPM will retain the signed
digest and enter the Field Upgrade mode (FUM). While in FUM, the TPM will
accept TPM2_FieldUpgradeData() commands. It may accept other commands if
it is able to complete them using the previously installed firmware.
Otherwise, it will return TPM_RC_UPGRADE."
www.trustedcomputinggroup.org/wp-content/uploads/
TPM-Rev-2.0-Part-3-Commands-01.38.pdf, page 307

and I do not not think that TPM2_FieldUpgradeData is a command which will/
should be given in a tpm kernel driver. So I think it is enough to handle 
all other commands as it is right now. Please tell me if you disagree.

I will update the commit message to clarify which commands there are.

>>  * If the TPM is in Failure mode, it will allow performing TPM
>>    initialization but will not provide any crypto operations.
>>    Will happily respond to Field Upgrade calls.
>>=20
>> Change the behavior of the tpm2_auto_startup(), so it detects the active
>> running mode of the TPM by adding the following checks.  If
>> tpm2_do_selftest() call returns TPM2_RC_UPGRADE, the TPM is in Upgrade
>> mode.
>> If the TPM is in Failure mode, it will successfully respond to both
>> tpm2_do_selftest() and tpm2_startup() calls. Although, will fail to
>> answer to tpm2_get_cc_attrs_tbl(). Use this fact to conclude that TPM is
>> in Failure mode.
>>=20
>> If detected that the TPM is in the Upgrade or Failure mode, the function
>> sets TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE flag.
> 
> I think we should shorten this to TPM_CHIP_FLAG_FIRMWARE_MODE. It's clear
> enough and just easier for eyes, really.

Done.

>> The limited mode flag is used later during driver
> 
> You probably have forgot to update commit message over here? Please try to
> align it with the code changes.

Done.

>> initialization/deinitialization to disable functionality which makes no
>> sense or will fail in the current TPM state. Following functionality is
>> affected:
>>  * do not register TPM as a hwrng
> 
> Nit: "do" -> "Do"
> 
>>  * do not register sysfs entries which provide information impossible to
>>    obtain in limited mode
>>  * do not register resource managed character device
> 
> Ditto.'

Done.

>>=20
>> Signed-off-by: Borys Movchan <borysmn@...s.com>
>> ---
>>=20
>> Notes:
>>     v2:
>>      * Commit message updated.
>>    =20
>>     v3:
>>      * Commit message reworked.
>>    =20
>>     v4:
>>      * Description of how tpm2_auto_startup() detects the mode added to
>>        commit message.
>>    =20
>>     v5:
>>      * Introduce global flag: TPM_CHIP_FLAG_LIMITED_MODE.
>>      * Add checks for the flag in places that will not work properly when=
> TPM
>>        functionality is limited.
>>      * Avoid registering sysfs and character device entries that have no =
> useful
>>        function in limited mode.
>>      * Do not register TPM as a hwrng.
>>      * Do not try to obtain any crypto-related properties from TPM as it =
> will fail
>>        in limited mode.
>>    =20
>>     v6:
>>      * Rename the TPM_CHIP_FLAG_LIMITED_MODE to TPM_CHIP_FLAG_FIRMWARE_UP=
> GRADE_MODE
>>        to reduce confusion. Update info messages.
>>=20
>>  drivers/char/tpm/tpm-chip.c  | 23 ++++++++++++++++-------
>>  drivers/char/tpm/tpm-sysfs.c |  3 +++
>>  drivers/char/tpm/tpm2-cmd.c  | 14 +++++++++++++-
>>  include/linux/tpm.h          |  2 ++
>>  4 files changed, 34 insertions(+), 8 deletions(-)
>>=20
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index ddaeceb7e109..3378bfe06006 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -444,7 +444,8 @@ static int tpm_add_char_device(struct tpm_chip *chip)
>>  		return rc;
>>  	}
>> =20
>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2 &&
>> +	    !(chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)) {
> 
> Please do this instead in tpm.h, give that the check is everywhere:
> 
> /*
>  * Check if TPM device is in the firmware upgrade mode.
>  */
> static inline bool tpm_is_firmware_upgrade(void)
> {
> 	return chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE;
> }
> 
> 
> Makes the code less stressing to read (and find the decision points
> later on with tools).
> 

Done.

>>  		rc =3D cdev_device_add(&chip->cdevs, &chip->devs);
>>  		if (rc) {
>>  			dev_err(&chip->devs,
>> @@ -488,7 +489,8 @@ static void tpm_del_legacy_sysfs(struct tpm_chip *chi=
>p)
>>  {
>>  	struct attribute **i;
>> =20
>> -	if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL))
>> +	if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL) ||
>> +	    chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
>>  		return;
>> =20
>>  	sysfs_remove_link(&chip->dev.parent->kobj, "ppi");
>> @@ -506,7 +508,8 @@ static int tpm_add_legacy_sysfs(struct tpm_chip *chip=
>)
>>  	struct attribute **i;
>>  	int rc;
>> =20
>> -	if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL))
>> +	if (chip->flags & (TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_VIRTUAL) ||
>> +		chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
>>  		return 0;
>> =20
>>  	rc =3D compat_only_sysfs_link_entry_to_kobj(
>> @@ -536,7 +539,8 @@ static int tpm_hwrng_read(struct hwrng *rng, void *da=
>ta, size_t max, bool wait)
>> =20
>>  static int tpm_add_hwrng(struct tpm_chip *chip)
>>  {
>> -	if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM))
>> +	if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM) ||
>> +	    chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
>>  		return 0;
>> =20
>>  	snprintf(chip->hwrng_name, sizeof(chip->hwrng_name),
>> @@ -550,6 +554,9 @@ static int tpm_get_pcr_allocation(struct tpm_chip *ch=
>ip)
>>  {
>>  	int rc;
>> =20
>> +	if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
>> +		return 0;
>> +
>>  	rc =3D (chip->flags & TPM_CHIP_FLAG_TPM2) ?
>>  	     tpm2_get_pcr_allocation(chip) :
>>  	     tpm1_get_pcr_allocation(chip);
>> @@ -612,7 +619,7 @@ int tpm_chip_register(struct tpm_chip *chip)
>>  	return 0;
>> =20
>>  out_hwrng:
>> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM))
>> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) && !(chip->flags & TPM_CHIP_FLAG_F=
>IRMWARE_UPGRADE_MODE))
>>  		hwrng_unregister(&chip->hwrng);
>>  out_ppi:
>>  	tpm_bios_log_teardown(chip);
>> @@ -637,10 +644,12 @@ EXPORT_SYMBOL_GPL(tpm_chip_register);
>>  void tpm_chip_unregister(struct tpm_chip *chip)
>>  {
>>  	tpm_del_legacy_sysfs(chip);
>> -	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM))
>> +	if (IS_ENABLED(CONFIG_HW_RANDOM_TPM) &&
>> +	    !(chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE))
>>  		hwrng_unregister(&chip->hwrng);
>>  	tpm_bios_log_teardown(chip);
>> -	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>> +	if (chip->flags & TPM_CHIP_FLAG_TPM2 &&
>> +	    !(chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE))
>>  		cdev_device_del(&chip->cdevs, &chip->devs);
>>  	tpm_del_char_device(chip);
>>  }
>> diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c
>> index 63f03cfb8e6a..2842f3c667a5 100644
>> --- a/drivers/char/tpm/tpm-sysfs.c
>> +++ b/drivers/char/tpm/tpm-sysfs.c
>> @@ -480,6 +480,9 @@ void tpm_sysfs_add_device(struct tpm_chip *chip)
>> =20
>>  	WARN_ON(chip->groups_cnt !=3D 0);
>> =20
>> +	if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
>> +		return;
>> +
>>  	if (chip->flags & TPM_CHIP_FLAG_TPM2)
>>  		chip->groups[chip->groups_cnt++] =3D &tpm2_dev_group;
>>  	else
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index a25815a6f625..76245c861e59 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -729,7 +729,12 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>>  		goto out;
>> =20
>>  	rc =3D tpm2_do_selftest(chip);
>> -	if (rc && rc !=3D TPM2_RC_INITIALIZE)
>> +	if (rc =3D=3D TPM2_RC_UPGRADE) {
>> +		dev_info(&chip->dev, "TPM requires firmware upgrade\n");
>> +		chip->flags |=3D TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE;
>> +		rc =3D 0;
>> +		goto out;
>> +	} else if (rc && rc !=3D TPM2_RC_INITIALIZE)
>>  		goto out;
>> =20
>>  	if (rc =3D=3D TPM2_RC_INITIALIZE) {
>> @@ -743,6 +748,13 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>>  	}
>> =20
>>  	rc =3D tpm2_get_cc_attrs_tbl(chip);
>> +	if (rc) {
>> +		dev_info(&chip->dev,
>> +			 "TPM requires firmware recovery/upgrade\n");
> 
> Why these two dev_info() messages differ?
> 
> You should put a single info message that is unconditionally printed
> when in FW upgrade mode, and not sprikle bloat to fallback paths.
> 

I removed these dev_info() messages and added one in out which will be printed
if (rc == TPM2_RC_UPGRADE).

>> +		chip->flags |=3D TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE;
>> +		rc =3D 0;
>> +		goto out;
>> +	}
>> =20
>>  out:
>>  	if (rc > 0)
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index aa11fe323c56..498f487d786f 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -207,6 +207,7 @@ enum tpm2_return_codes {
>>  	TPM2_RC_INITIALIZE	=3D 0x0100, /* RC_VER1 */
>>  	TPM2_RC_FAILURE		=3D 0x0101,
>>  	TPM2_RC_DISABLED	=3D 0x0120,
>> +	TPM2_RC_UPGRADE		=3D 0x012D,
>>  	TPM2_RC_COMMAND_CODE    =3D 0x0143,
>>  	TPM2_RC_TESTING		=3D 0x090A, /* RC_WARN */
>>  	TPM2_RC_REFERENCE_H0	=3D 0x0910,
>> @@ -277,6 +278,7 @@ enum tpm_chip_flags {
>>  	TPM_CHIP_FLAG_HAVE_TIMEOUTS	=3D BIT(4),
>>  	TPM_CHIP_FLAG_ALWAYS_POWERED	=3D BIT(5),
>>  	TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED	=3D BIT(6),
>> +	TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE	=3D BIT(7),
>>  };
>> =20
>>  #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)

Best regards, Axel

Powered by blists - more mailing lists