Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array

From: Nayna Jain
Date: Thu Nov 08 2018 - 08:54:12 EST



On 11/07/2018 03:11 PM, Roberto Sassu wrote:
On 11/7/2018 7:14 AM, Nayna Jain wrote:


On 11/06/2018 08:31 PM, Roberto Sassu wrote:
This patch removes the hard-coded limit of the active_banks array size.


The hard-coded limit in static array active_banks[] represents the maximum possible banks.
A TPM might have three banks, but only one bank may be active.

To confirm my understanding, is the idea for this patch is to dynamically identify the number of possible banks or the number of active banks ?

The idea is to dynamically identify the number of active banks.

In the TPM Commands specification (section 30.2.1), I found:

TPM_CAP_PCRS â Returns the current allocation of PCR in a
TPML_PCR_SELECTION.

You mentioned:

#TPM_RC_SIZE response code when count is greater
than the possible number of banks

but TPML_PCR_SELECTION is provided by the TPM.

Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns the number of possible algorithms supported. In the example below, two possible algorithms - SHA1 and SHA256 - are returned.

# /usr/local/bin/tssgetcapability -cap 5
2 PCR selections
ÂÂÂÂhash TPM_ALG_SHA1
ÂÂÂÂTPMS_PCR_SELECTION length 3
ÂÂÂÂff ff ff
ÂÂÂÂhash TPM_ALG_SHA256
ÂÂÂÂTPMS_PCR_SELECTION length 3
ÂÂÂÂ00 00 00

The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256 bank is not enabled.

The current code works, but it unnecessarily extends some banks. Instead of basing the number of active banks on the number of algorithms returned, it should be based on the pcr_select field.

ÂÂ - Mimi & Nayna



Roberto


It stores in the tpm_chip structure the number of active PCR banks,
determined in tpm2_get_pcr_allocation(), and replaces the static array
with a pointer to a dynamically allocated array.

As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
does not check anymore if the algorithm stored in tpm_chip is equal to
zero. The active_banks array always contains valid algorithms.

Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
PCR banks")

Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
---
 drivers/char/tpm/tpm-chip.c | 1 +
 drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
 drivers/char/tpm/tpm.h | 3 ++-
 drivers/char/tpm/tpm2-cmd.c | 17 ++++++++---------
 4 files changed, 23 insertions(+), 17 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..2a9e8b744436 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
ÂÂÂÂÂ kfree(chip->log.bios_event_log);
ÂÂÂÂÂ kfree(chip->work_space.context_buf);
ÂÂÂÂÂ kfree(chip->work_space.session_buf);
+ÂÂÂ kfree(chip->active_banks);
ÂÂÂÂÂ kfree(chip);
 }

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..ba7ca6b3e664 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
 int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
ÂÂÂÂÂ int rc;
-ÂÂÂ struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-ÂÂÂ u32 count = 0;
+ÂÂÂ struct tpm2_digest *digest_list;
ÂÂÂÂÂ int i;

ÂÂÂÂÂ chip = tpm_find_get_ops(chip);
@@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
ÂÂÂÂÂÂÂÂÂ return -ENODEV;

ÂÂÂÂÂ if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-ÂÂÂÂÂÂÂ memset(digest_list, 0, sizeof(digest_list));
+ÂÂÂÂÂÂÂ digest_list = kmalloc_array(chip->nr_active_banks,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*digest_list), GFP_KERNEL);
+ÂÂÂÂÂÂÂ if (!digest_list)
+ÂÂÂÂÂÂÂÂÂÂÂ return -ENOMEM;

-ÂÂÂÂÂÂÂ for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+ÂÂÂÂÂÂÂ memset(digest_list, 0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ chip->nr_active_banks * sizeof(*digest_list));
+
+ÂÂÂÂÂÂÂ for (i = 0; i < chip->nr_active_banks; i++) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ digest_list[i].alg_id = chip->active_banks[i];
ÂÂÂÂÂÂÂÂÂÂÂÂÂ memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-ÂÂÂÂÂÂÂÂÂÂÂ count++;
ÂÂÂÂÂÂÂÂÂ }

-ÂÂÂÂÂÂÂ rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+ÂÂÂÂÂÂÂ rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ digest_list);
+ÂÂÂÂÂÂÂ kfree(digest_list);
ÂÂÂÂÂÂÂÂÂ tpm_put_ops(chip);
ÂÂÂÂÂÂÂÂÂ return rc;
ÂÂÂÂÂ }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..98368c3a6ff7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -248,7 +248,8 @@ struct tpm_chip {
ÂÂÂÂÂ const struct attribute_group *groups[3];
ÂÂÂÂÂ unsigned int groups_cnt;

-ÂÂÂ u16 active_banks[7];
+ÂÂÂ u32 nr_active_banks;
+ÂÂÂ u16 *active_banks;
 #ifdef CONFIG_ACPI
ÂÂÂÂÂ acpi_handle acpi_dev_handle;
ÂÂÂÂÂ char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c31b490bd41d..533089cede07 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
ÂÂÂÂÂ int i;
ÂÂÂÂÂ int j;

-ÂÂÂ if (count > ARRAY_SIZE(chip->active_banks))
+ÂÂÂ if (count > chip->nr_active_banks)
ÂÂÂÂÂÂÂÂÂ return -EINVAL;

ÂÂÂÂÂ rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
@@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
ÂÂÂÂÂ void *marker;
ÂÂÂÂÂ void *end;
ÂÂÂÂÂ void *pcr_select_offset;
-ÂÂÂ unsigned int count;
ÂÂÂÂÂ u32 sizeof_pcr_selection;
ÂÂÂÂÂ u32 rsp_len;
ÂÂÂÂÂ int rc;
@@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
ÂÂÂÂÂ if (rc)
ÂÂÂÂÂÂÂÂÂ goto out;

-ÂÂÂ count = be32_to_cpup(
+ÂÂÂ chip->nr_active_banks = be32_to_cpup(
ÂÂÂÂÂÂÂÂÂ (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);


As per my understanding, the count in the TPML_PCR_SELECTION represent the number of possible banks and not the number of active banks.
TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation of #TPM_RC_SIZE.

Thanks & Regards,
ÂÂÂÂ - Nayna



-ÂÂÂ if (count > ARRAY_SIZE(chip->active_banks)) {
-ÂÂÂÂÂÂÂ rc = -ENODEV;
+ÂÂÂ chip->active_banks = kmalloc_array(chip->nr_active_banks,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sizeof(*chip->active_banks),
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ GFP_KERNEL);
+ÂÂÂ if (!chip->active_banks) {
+ÂÂÂÂÂÂÂ rc = -ENOMEM;
ÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂ }

@@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
ÂÂÂÂÂ rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
ÂÂÂÂÂ end = &buf.data[rsp_len];

-ÂÂÂ for (i = 0; i < count; i++) {
+ÂÂÂ for (i = 0; i < chip->nr_active_banks; i++) {
ÂÂÂÂÂÂÂÂÂ pcr_select_offset = marker +
ÂÂÂÂÂÂÂÂÂÂÂÂÂ offsetof(struct tpm2_pcr_selection, size_of_select);
ÂÂÂÂÂÂÂÂÂ if (pcr_select_offset >= end) {
@@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
ÂÂÂÂÂ }

 out:
-ÂÂÂ if (i < ARRAY_SIZE(chip->active_banks))
-ÂÂÂÂÂÂÂ chip->active_banks[i] = TPM2_ALG_ERROR;
-
ÂÂÂÂÂ tpm_buf_destroy(&buf);

ÂÂÂÂÂ return rc;