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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 07/05/2019 10:13 AM, Stefan Berger wrote:
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;


Yes. I think I misread it. Thanks Stefan. Will fix this..






+    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);

I am not sure of the purpose of tpm_stop_chip(), so I have left it as it is. Jarkko, what do you think about the change ?

Thanks & Regards,
         - Nayna





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux