Re: [PATCH v2 1/1] tpm: reject duplicate PCR banks in tpm2_get_pcr_allocation

[Jarkko Sakkinen <jarkko@kernel.org>]

On Mon, Sep 28, 2026 at 04:27:00PM +0800, Yuqi Xu wrote:
> The TPM2_CAP_PCRS response is external input. tpm2_get_pcr_allocation()
> adds every active selection to allocated_banks, while
> tpm_sysfs_add_device() has one group slot per distinct hash algorithm.
> Eight duplicate selections can therefore write past chip->groups[].
> 
> The TPM protocol does not allow a hash algorithm to appear twice in
> this capability response. Treat a duplicate as a broken TPM and return
> -EIO so the chip is not enabled, instead of silently dropping the
> extra selection.
> 
> Fixes: aab73d952402 ("tpm: add sysfs exports for all banks of PCR registers")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Assisted-by: LLM
> Signed-off-by: Yuqi Xu <xuyuqiabc@gmail.com>
> Reviewed-by: Ren Wei <weir@nebusec.ai>
> ---
> Changes in v2:
>  - Return -EIO on a duplicate PCR bank instead of silently ignoring
>    it, as requested by Jarkko Sakkinen. A duplicate violates the TPM
>    protocol, so the chip is not enabled.
>  - v1 Link: https://lore.kernel.org/all/cover.1789800840.git.xuyuqiabc@gmail.com/
>  drivers/char/tpm/tpm2-cmd.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index ae22295df798..a94f4395c198 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -527,6 +527,7 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	u32 rsp_len;
>  	int rc;
>  	int i = 0;
> +	int j;
>  
>  	struct tpm_buf *buf __free(kfree) = kzalloc(TPM_BUFSIZE, GFP_KERNEL);
>  	if (!buf)
> @@ -569,6 +570,23 @@ ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  		pcr_select_offset = memchr_inv(pcr_selection.pcr_select, 0,
>  					       pcr_selection.size_of_select);
>  		if (pcr_select_offset) {
> +			for (j = 0; j < nr_alloc_banks; j++) {
> +				if (chip->allocated_banks[j].alg_id == hash_alg)
> +					break;
> +			}
> +
> +			/*
> +			 * The TPM protocol does not allow the same hash
> +			 * algorithm twice.  A duplicate means this device
> +			 * is not behaving; do not enable it.
> +			 */
> +			if (j != nr_alloc_banks) {
> +				pr_err("tpm: duplicate PCR bank for algorithm 0x%04x\n",
> +				       hash_alg);
> +				rc = -EIO;
> +				break;
> +			}
> +
>  			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
>  
>  			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
> -- 
> 2.55.0
> 

Yeah, so in some places like this some of the TPM driver code does
return -EFAULT but I'd like to progressively refactor so that any
corrupted traffic is always -EIO and use -EFAULT for memory related
issues. 

Thank you!

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

Br, Jarkko