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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Tue, Sep 30, 2025 at 12:09:15PM +0100, Jonathan McDowell wrote:
> 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?

I'll revert this as it is essentially a spurious change.

> 
> > +#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.

I've never seen hardware TPM that would have more than one or two banks.
We can double it to leave some room. This was tested with swtpm
defaults.

> 
> >  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

BR, Jarkko