Re: [PATCH] tpm: fixes uninitialized allocated banks for IBM vtpm driver

From: Stefan Berger
Date: Fri Jul 05 2019 - 10:15:21 EST


On 7/3/19 11:32 PM, Nayna Jain wrote:
The nr_allocated_banks and allocated banks are initialized as part of
tpm_chip_register. Currently, this is done as part of auto startup
function. However, some drivers, like the ibm vtpm driver, do not run
auto startup during initialization. This results in uninitialized memory
issue and causes a kernel panic during boot.

This patch moves the pcr allocation outside the auto startup function
into tpm_chip_register. This ensures that allocated banks are initialized
in any case.

Fixes: 879b589210a9 ("tpm: retrieve digest size of unknown algorithms with
PCR read")
Signed-off-by: Nayna Jain <nayna@xxxxxxxxxxxxx>
---
drivers/char/tpm/tpm-chip.c | 37 +++++++++++++++++++++++++++++++++++++
drivers/char/tpm/tpm.h | 1 +
drivers/char/tpm/tpm1-cmd.c | 12 ------------
drivers/char/tpm/tpm2-cmd.c | 6 +-----
4 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 8804c9e916fd..958508bb8379 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -550,6 +550,39 @@ static int tpm_add_hwrng(struct tpm_chip *chip)
return hwrng_register(&chip->hwrng);
}

+/*
+ * tpm_pcr_allocation() - initializes the chip allocated banks for PCRs
+ */
+static int tpm_pcr_allocation(struct tpm_chip *chip)
+{
+ int rc = 0;
+
+ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+ rc = tpm2_get_pcr_allocation(chip);
+ if (rc)
+ goto out;
+ }
+
+ /* Initialize TPM 1.2 */
+ chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
+ GFP_KERNEL);
+ if (!chip->allocated_banks) {
+ rc = -ENOMEM;
+ goto out;
+ }
+
+ 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;
+out:
+ if (rc < 0)
+ rc = -ENODEV;


The old code where you lifted this from said:

out:
ÂÂÂ if (rc > 0)
ÂÂÂ ÂÂÂ rc = -ENODEV;
ÂÂÂ return rc;

It would not overwrite -ENOMEM with -ENODEV but yours does.

I think the correct fix would be to use:

if (rc > 0)

ÂÂÂ rc = -ENODEV;





+ return rc;
+}
+
/*
* tpm_chip_register() - create a character device for the TPM chip
* @chip: TPM chip to use.
@@ -573,6 +606,10 @@ int tpm_chip_register(struct tpm_chip *chip)
if (rc)
return rc;

Above this is tpm_chip_stop(chip) because (afaik) none of the following function calls in tpm_chip_register() needed the TPM, but now with tpm_pcr_allocation() you will need to send a command to the TPM. So I would say you should move the tpm_chip_stop() into the error branch visible above and also after the tpm_pcr_allocation().


+ rc = tpm_pcr_allocation(chip);
tpm_chip_stop(chip);
+ if (rc)
+ return rc;
+
tpm_sysfs_add_device(chip);

rc = tpm_bios_log_setup(chip);


 Stefan