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

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@linux.ibm.com>
---
 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;
+	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;
 
+	rc = tpm_pcr_allocation(chip);
+	if (rc)
+		return rc;
+
 	tpm_sysfs_add_device(chip);
 
 	rc = tpm_bios_log_setup(chip);

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2cce072f25b5..eabe6b755fa6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h

@@ -454,6 +454,7 @@ int tpm2_unseal_trusted(struct tpm_chip *chip,
 ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 			u32 *value, const char *desc);
 
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip);
 int tpm2_auto_startup(struct tpm_chip *chip);
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type);
 unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);

diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 85dcf2654d11..ec5f3693c096 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c

@@ -696,18 +696,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	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 rc;
 out:
 	if (rc > 0)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e74c5b7b64bf..b4384d0e3741 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c

@@ -841,7 +841,7 @@ struct tpm2_pcr_selection {
 	u8  pcr_select[3];
 } __packed;
 
-static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
+ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 {
 	struct tpm2_pcr_selection pcr_selection;
 	struct tpm_buf buf;

@@ -1041,10 +1041,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 			goto out;
 	}
 
-	rc = tpm2_get_pcr_allocation(chip);
-	if (rc)
-		goto out;
-
 	rc = tpm2_get_cc_attrs_tbl(chip);
 
 out:

-- 
2.20.1

On Wed, 2019-07-03 at 23:32 -0400, 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@linux.ibm.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

On 04-Jul-2019, at 5:29 PM, Mimi Zohar [off-list ref] wrote:

On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:

quoted

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@linux.ibm.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Thanks for the fix. Kernel boots fine with this fix.

Tested-by: Sachin Sant <redacted>

Thanks
-Sachin

On Thu, 4 Jul 2019 19:26:36 +0530
Sachin Sant [off-list ref] wrote:

quoted

On 04-Jul-2019, at 5:29 PM, Mimi Zohar [off-list ref] wrote:

On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:  

quoted

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@linux.ibm.com>  

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>  

Thanks for the fix. Kernel boots fine with this fix.

Tested-by: Sachin Sant <redacted>

Tested-by: Michal Suchánek <redacted>

Thanks

Michal

On Wed, 2019-07-03 at 23:32 -0400, 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@linux.ibm.com>

Please add

Reported-by: Michal Suchanek <redacted>

It is missing. Michal is there a chance you could try this out once
Nayna send a new version?

Why that name and not tpm_get_pcr_allocation()? Do not get why
"get_" has been dropped. Please add it back.

Would be senseful to create tpm1_get_pcr_allocation() to tpm1-cmd.c
now that a new function needs to be introduced anyway. Please do
it for that for TPM 1.x part.

/Jarkko

On Fri, 05 Jul 2019 13:42:18 +0300
Jarkko Sakkinen [off-list ref] wrote:

On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:

quoted

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@linux.ibm.com>  

Please add

Reported-by: Michal Suchanek <redacted>

It is missing. Michal is there a chance you could try this out once
Nayna send a new version?

Should be easy to test. Without the patch the machine crashes on boot
so it is quite obvious case. I have a few VMs with the vTPM available.

Thanks

Michal

On Fri, 2019-07-05 at 13:42 +0300, Jarkko Sakkinen wrote:

On Wed, 2019-07-03 at 23:32 -0400, Nayna Jain wrote:

quoted

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@linux.ibm.com>

Please add

Reported-by: Michal Suchanek <redacted>

It is missing. Michal is there a chance you could try this out once
Nayna send a new version?

Hey, I saw Michal's tested-by. I can do that minor reorg cosmetic
bits myeslf and add Micha's tag.

Some issue with the network but I'll push a commit soonish.

/Jarkko

On 7/3/19 11:32 PM, Nayna Jain wrote:

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;

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

  Stefan

On 07/05/2019 10:13 AM, Stefan Berger wrote:

On 7/3/19 11:32 PM, Nayna Jain wrote:

quoted

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@linux.ibm.com>
---
  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..

quoted

+    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().

quoted

+    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

On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote:

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 ?

Stefan right. Your does not work, or will randomly work or not work
depending on the chip.

You need to turn the TPM on with tpm_chip_start() and turn it off with
tpm_chip_stop() once you are done. This is done in tpm_chip_register()
before calling tpm_auto_startup().

TPM power management was once in tpm_transmit() but not anymore after my
patch set that removed nested tpm_transmit() calls.

While you're on it please take into account my earlier feedback.

Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks"

Some oddballs in your patch that I have to ask.

if (chip->flags & TPM_CHIP_FLAG_TPM2) {
	rc = tpm2_get_pcr_allocation(chip);
	if (rc)
		goto out;
}

chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
		GFP_KERNEL);
if (!chip->allocated_banks) {
	rc = -ENOMEM;
	goto out;
}

Why you don't return on site and instead jump somewhere? Also the
2nd line for kcalloc() is misaligned.

out:
	if (rc < 0)
		rc = -ENODEV;

This will cause a new regression i.e. you let TPM error codes
through.

To summarize this patch fixes one regression and introduces two
completely new ones...

/Jarkko`

On Fri, 2019-07-05 at 20:50 +0300, Jarkko Sakkinen wrote:

To summarize this patch fixes one regression and introduces two
completely new ones...

Anyway, take the time and update it. The principle is right
anyway. I'll merge it once it is in a better shape...

/Jarkko

On 07/05/2019 01:50 PM, Jarkko Sakkinen wrote:

On Fri, 2019-07-05 at 11:32 -0400, Nayna wrote:

quoted

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 ?

Stefan right. Your does not work, or will randomly work or not work
depending on the chip.

You need to turn the TPM on with tpm_chip_start() and turn it off with
tpm_chip_stop() once you are done. This is done in tpm_chip_register()
before calling tpm_auto_startup().

TPM power management was once in tpm_transmit() but not anymore after my
patch set that removed nested tpm_transmit() calls.

While you're on it please take into account my earlier feedback.

Also, short summary could be "tpm: tpm_ibm_vtpm: Fix unallocated banks"

Some oddballs in your patch that I have to ask.

if (chip->flags & TPM_CHIP_FLAG_TPM2) {
	rc = tpm2_get_pcr_allocation(chip);
	if (rc)
		goto out;
}

chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
		GFP_KERNEL);
if (!chip->allocated_banks) {
	rc = -ENOMEM;
	goto out;
}

Why you don't return on site and instead jump somewhere? Also the
2nd line for kcalloc() is misaligned.

out:
	if (rc < 0)
		rc = -ENODEV;

This will cause a new regression i.e. you let TPM error codes
through.

To summarize this patch fixes one regression and introduces two
completely new ones...

Thanks Jarkko. I just now posted the v2 version that includes your and Stefan's feedbacks.

Thanks & Regards,
       - Nayna

On Sat, 2019-07-06 at 20:25 -0400, Nayna wrote:

Thanks Jarkko. I just now posted the v2 version that includes your and
Stefan's feedbacks.

Looks almost legit :-)

/Jarkko