Re: [PATCH v3 01/10] tpm: Cap the number of PCR banks

[Jonathan McDowell <noodles@earth.li>]

On Mon, Sep 29, 2025 at 10:48:23PM +0300, Jarkko Sakkinen wrote:
> From: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> 
> tpm2_get_pcr_allocation() does not cap any upper limit for the number of
> banks. Cap the limit to four banks so that out of bounds values coming
> from external I/O cause on only limited harm.
> 
> Cc: Roberto Sassu <roberto.sassu@huawei.com>
> Fixes: bcfff8384f6c ("tpm: dynamically allocate the allocated_banks array")
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@opinsys.com>
> ---
> v3:
> - Wrote a more clear commit message.
> - Fixed pr_err() message.
> v2:
> - A new patch.
> ---
>  drivers/char/tpm/tpm-chip.c | 13 +++++++++----
>  drivers/char/tpm/tpm.h      |  1 -
>  drivers/char/tpm/tpm1-cmd.c | 25 -------------------------
>  drivers/char/tpm/tpm2-cmd.c |  8 +++-----
>  include/linux/tpm.h         | 18 ++++++++----------
>  5 files changed, 20 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 687f6d8cd601..9a6538f76f50 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -559,14 +559,19 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
>  
>  static int tpm_get_pcr_allocation(struct tpm_chip *chip)
>  {
> -	int rc;
> +	int rc = 0;
>  
>  	if (tpm_is_firmware_upgrade(chip))
>  		return 0;
>  
> -	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
> -	     tpm2_get_pcr_allocation(chip) :
> -	     tpm1_get_pcr_allocation(chip);
> +	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
> +		chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> +		chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> +		chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> +		chip->nr_allocated_banks = 1;
> +	} else {
> +		rc = tpm2_get_pcr_allocation(chip);
> +	}
>  
>  	if (rc > 0)
>  		return -ENODEV;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 57ef8589f5f5..769fa6b00c54 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -252,7 +252,6 @@ int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>  ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>  		    const char *desc, size_t min_cap_length);
>  int tpm1_get_random(struct tpm_chip *chip, u8 *out, size_t max);
> -int tpm1_get_pcr_allocation(struct tpm_chip *chip);
>  unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>  int tpm_pm_suspend(struct device *dev);
>  int tpm_pm_resume(struct device *dev);
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index cf64c7385105..5c49bdff33de 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -786,28 +786,3 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>  
>  	return rc;
>  }
> -
> -/**
> - * tpm1_get_pcr_allocation() - initialize the allocated bank
> - * @chip: TPM chip to use.
> - *
> - * The function initializes the SHA1 allocated bank to extend PCR
> - *
> - * Return:
> - * * 0 on success,
> - * * < 0 on error.
> - */
> -int tpm1_get_pcr_allocation(struct tpm_chip *chip)
> -{
> -	chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
> -					GFP_KERNEL);
> -	if (!chip->allocated_banks)
> -		return -ENOMEM;
> -
> -	chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
> -	chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
> -	chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
> -	chip->nr_allocated_banks = 1;
> -
> -	return 0;
> -}
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 7d77f6fbc152..a7cddd4b5626 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -538,11 +538,9 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  
>  	nr_possible_banks = be32_to_cpup(
>  		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> -
> -	chip->allocated_banks = kcalloc(nr_possible_banks,
> -					sizeof(*chip->allocated_banks),
> -					GFP_KERNEL);
> -	if (!chip->allocated_banks) {
> +	if (nr_possible_banks > TPM2_MAX_BANKS) {
> +		pr_err("tpm: unexpected number of banks: %u > %u",
> +		       nr_possible_banks, TPM2_MAX_BANKS);
>  		rc = -ENOMEM;
>  		goto out;
>  	}
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index 900c81a2bc41..fc7df87dfb9a 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -27,7 +27,12 @@
>  #include <crypto/aes.h>
>  
>  #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
> -#define TPM_MAX_DIGEST_SIZE SHA512_DIGEST_SIZE
> +#define TPM_HEADER_SIZE		10
> +
> +#define TPM2_PLATFORM_PCR	24
> +#define TPM2_PCR_SELECT_MIN	3

By changing this to 3 we lose the fact it's related to TPM2_PLATFORM_PCR 
- it's the number of bytes required to hold a bitmap with at least 
 TPM2_PLATFORM_PCR entries. Can we at least have a comment about that 
fact?

> +#define TPM2_MAX_DIGEST_SIZE	SHA512_DIGEST_SIZE
> +#define TPM2_MAX_BANKS		4

Where does this max come from? It matches what I see with swtpm by 
default (SHA1, SHA2-256, SHA2-384, SHA-512), so I haven't seen anything 
that exceeds it myself.

>  struct tpm_chip;
>  struct trusted_key_payload;
> @@ -69,7 +74,7 @@ enum tpm2_curves {
>  
>  struct tpm_digest {
>  	u16 alg_id;
> -	u8 digest[TPM_MAX_DIGEST_SIZE];
> +	u8 digest[TPM2_MAX_DIGEST_SIZE];
>  } __packed;
>  
>  struct tpm_bank_info {
> @@ -190,7 +195,7 @@ struct tpm_chip {
>  	unsigned int groups_cnt;
>  
>  	u32 nr_allocated_banks;
> -	struct tpm_bank_info *allocated_banks;
> +	struct tpm_bank_info allocated_banks[TPM2_MAX_BANKS];
>  #ifdef CONFIG_ACPI
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> @@ -217,13 +222,6 @@ struct tpm_chip {
>  #endif
>  };
>  
> -#define TPM_HEADER_SIZE		10
> -
> -enum tpm2_const {
> -	TPM2_PLATFORM_PCR       =     24,
> -	TPM2_PCR_SELECT_MIN     = ((TPM2_PLATFORM_PCR + 7) / 8),
> -};
> -
>  enum tpm2_timeouts {
>  	TPM2_TIMEOUT_A          =    750,
>  	TPM2_TIMEOUT_B          =   4000,
> -- 
> 2.39.5
> 
> 

J.

-- 
"How the f**k did you work that out?" -- Pythagoras