| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <59f8d5a835ff0f02c9efe0ecff0abbe1b4f08111.camel@kernel.org>
Date: Thu, 30 Sep 2021 19:28:34 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Borys Movchan <borysmn@...s.com>, Peter Huewe <peterhuewe@....de>,
Jason Gunthorpe <jgg@...pe.ca>
Cc: kernel@...s.com, linux-integrity@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6] tpm: Add Upgrade/Reduced mode support for TPM2
modules
On Thu, 2021-09-30 at 18:02 +0200, Borys Movchan wrote:
> 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.
>
> 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.
> * 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.
>
> 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.
>
> 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.
> 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.
> 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.
>
> Signed-off-by: Borys Movchan <borysmn@...s.com>
> ---
>
> Notes:
> v2:
> * Commit message updated.
>
> v3:
> * Commit message reworked.
>
> v4:
> * Description of how tpm2_auto_startup() detects the mode added to
> commit message.
>
> 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.
>
> v6:
> * Rename the TPM_CHIP_FLAG_LIMITED_MODE to TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE
> to reduce confusion. Update info messages.
>
> 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(-)
>
> 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;
> }
>
> - 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).
> rc = 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 *chip)
> {
> struct attribute **i;
>
> - 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;
>
> 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;
>
> - 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;
>
> rc = compat_only_sysfs_link_entry_to_kobj(
> @@ -536,7 +539,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))
> + if (!IS_ENABLED(CONFIG_HW_RANDOM_TPM) ||
> + chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
> return 0;
>
> snprintf(chip->hwrng_name, sizeof(chip->hwrng_name),
> @@ -550,6 +554,9 @@ static int tpm_get_pcr_allocation(struct tpm_chip *chip)
> {
> int rc;
>
> + if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
> + return 0;
> +
> rc = (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;
>
> out_hwrng:
> - 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);
> 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)
>
> WARN_ON(chip->groups_cnt != 0);
>
> + if (chip->flags & TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE)
> + return;
> +
> if (chip->flags & TPM_CHIP_FLAG_TPM2)
> chip->groups[chip->groups_cnt++] = &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;
>
> rc = tpm2_do_selftest(chip);
> - if (rc && rc != TPM2_RC_INITIALIZE)
> + if (rc == TPM2_RC_UPGRADE) {
> + dev_info(&chip->dev, "TPM requires firmware upgrade\n");
> + chip->flags |= TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE;
> + rc = 0;
> + goto out;
> + } else if (rc && rc != TPM2_RC_INITIALIZE)
> goto out;
>
> if (rc == TPM2_RC_INITIALIZE) {
> @@ -743,6 +748,13 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> }
>
> rc = 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.
> + chip->flags |= TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE;
> + rc = 0;
> + goto out;
> + }
>
> 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 = 0x0100, /* RC_VER1 */
> TPM2_RC_FAILURE = 0x0101,
> TPM2_RC_DISABLED = 0x0120,
> + TPM2_RC_UPGRADE = 0x012D,
> TPM2_RC_COMMAND_CODE = 0x0143,
> TPM2_RC_TESTING = 0x090A, /* RC_WARN */
> TPM2_RC_REFERENCE_H0 = 0x0910,
> @@ -277,6 +278,7 @@ enum tpm_chip_flags {
> TPM_CHIP_FLAG_HAVE_TIMEOUTS = BIT(4),
> TPM_CHIP_FLAG_ALWAYS_POWERED = BIT(5),
> TPM_CHIP_FLAG_FIRMWARE_POWER_MANAGED = BIT(6),
> + TPM_CHIP_FLAG_FIRMWARE_UPGRADE_MODE = BIT(7),
> };
>
> #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
/Jarkko
Powered by blists - more mailing lists