LinuxLists.cc - [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM

2018-11-06 15:08:04

Subject: [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM

The TPM driver currently relies on the crypto subsystem to determine the
digest size of supported TPM algorithms. In the future, TPM vendors might
implement new algorithms in their chips, and those algorithms might not
be supported by the crypto subsystem.

Usually, vendors provide patches for the new hardware, and likely
the crypto subsystem will be updated before the new algorithm is
introduced. However, old kernels might be updated later, after patches
are included in the mainline kernel. This would leave the opportunity
for attackers to misuse PCRs, as PCR banks with an unknown algorithm
are not extended.

This patch set provides a long term solution for this issue. If a TPM
algorithm is not known by the crypto subsystem, the TPM driver retrieves
the digest size from the TPM with a PCR read. All the PCR banks are
extended, even if the algorithm is not yet supported by the crypto
subsystem.

PCR bank information (TPM algorithm ID, digest size, crypto subsystem ID)
is stored in the tpm_chip structure and available for users of the TPM
driver.

Changelog

v3:
- remove end marker change
- replace active_banks static array with pointer to dynamic array
- remove TPM2_ACTIVE_PCR_BANKS

v2:
- change the end marker of the active_banks array
- check digest size from output of PCR read command
- remove count parameter from tpm_pcr_read() and tpm2_pcr_read()

v1:
- modify definition of tpm_pcr_read()
- move hash algorithms and definition of tpm2_digest to include/linux/tpm.h

Roberto Sassu (6):
tpm: dynamically allocate active_banks array
tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
tpm: rename and export tpm2_digest and tpm2_algorithms
tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
tpm: retrieve digest size of unknown algorithms with PCR read
tpm: ensure that the output of PCR read contains the correct digest
size

drivers/char/tpm/tpm-chip.c | 1 +
drivers/char/tpm/tpm-interface.c | 34 +++++---
drivers/char/tpm/tpm.h | 19 ++---
drivers/char/tpm/tpm2-cmd.c | 115 ++++++++++++++++++++--------
include/linux/tpm.h | 30 +++++++-
include/linux/tpm_eventlog.h | 12 +--
security/integrity/ima/ima_crypto.c | 10 +--
7 files changed, 145 insertions(+), 76 deletions(-)

--
2.17.1

2018-11-06 15:08:25

by Roberto Sassu

[permalink] [raw]

Subject: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

tcg_efi_specid_event and tcg_pcr_event2 declaration contains static arrays
for a list of hash algorithms used for event logs and event log digests.
However, according to TCG EFI Protocol Specification, these arrays have
variable sizes and are not suitable for parsing events with type casting.

Since declaring static arrays with hard-coded sizes does not help to parse
data after these arrays, this patch removes the declaration of
TPM2_ACTIVE_PCR_BANKS and sets the size of the arrays above to zero.

Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
event log")

Signed-off-by: Roberto Sassu <[email protected]>
---
include/linux/tpm_eventlog.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 20d9da77fc11..3d5d162f09cc 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -8,7 +8,6 @@
#define TCG_EVENT_NAME_LEN_MAX 255
#define MAX_TEXT_EVENT 1000 /* Max event string length */
#define ACPI_TCPA_SIG "TCPA" /* 0x41504354 /'TCPA' */
-#define TPM2_ACTIVE_PCR_BANKS 3

#define EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2 0x1
#define EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 0x2
@@ -90,7 +89,7 @@ struct tcg_efi_specid_event {
u8 spec_errata;
u8 uintnsize;
u32 num_algs;
- struct tcg_efi_specid_event_algs digest_sizes[TPM2_ACTIVE_PCR_BANKS];
+ struct tcg_efi_specid_event_algs digest_sizes[0];
u8 vendor_info_size;
u8 vendor_info[0];
} __packed;
@@ -117,7 +116,7 @@ struct tcg_pcr_event2 {
u32 pcr_idx;
u32 event_type;
u32 count;
- struct tpm2_digest digests[TPM2_ACTIVE_PCR_BANKS];
+ struct tpm2_digest digests[0];
struct tcg_event_field event;
} __packed;

--
2.17.1

2018-11-06 15:08:25

by Roberto Sassu

[permalink] [raw]

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

This patch removes the hard-coded limit of the active_banks array size.
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 <[email protected]>
---
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]);

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

2018-11-06 15:08:29

by Roberto Sassu

[permalink] [raw]

Subject: [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms

Rename tpm2_* to tpm_* and move the definitions to include/linux/tpm.h so
that these can be used by other kernel subsystems (e.g. IMA).

Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 2 +-
drivers/char/tpm/tpm.h | 13 +------------
drivers/char/tpm/tpm2-cmd.c | 18 +++++++++---------
include/linux/tpm.h | 18 ++++++++++++++++++
include/linux/tpm_eventlog.h | 9 ++-------
5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ba7ca6b3e664..07b62734598c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,7 +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;
+ struct tpm_digest *digest_list;
int i;

chip = tpm_find_get_ops(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 98368c3a6ff7..fc2cc16e5080 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -122,17 +122,6 @@ enum tpm2_return_codes {
TPM2_RC_RETRY = 0x0922,
};

-enum tpm2_algorithms {
- TPM2_ALG_ERROR = 0x0000,
- TPM2_ALG_SHA1 = 0x0004,
- TPM2_ALG_KEYEDHASH = 0x0008,
- TPM2_ALG_SHA256 = 0x000B,
- TPM2_ALG_SHA384 = 0x000C,
- TPM2_ALG_SHA512 = 0x000D,
- TPM2_ALG_NULL = 0x0010,
- TPM2_ALG_SM3_256 = 0x0012,
-};
-
enum tpm2_command_codes {
TPM2_CC_FIRST = 0x011F,
TPM2_CC_CREATE_PRIMARY = 0x0131,
@@ -578,7 +567,7 @@ static inline u32 tpm2_rc_value(u32 rc)

int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
- struct tpm2_digest *digests);
+ struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
unsigned int flags);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 533089cede07..584dba6cdf3e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -33,11 +33,11 @@ struct tpm2_hash {
};

static struct tpm2_hash tpm2_hash_map[] = {
- {HASH_ALGO_SHA1, TPM2_ALG_SHA1},
- {HASH_ALGO_SHA256, TPM2_ALG_SHA256},
- {HASH_ALGO_SHA384, TPM2_ALG_SHA384},
- {HASH_ALGO_SHA512, TPM2_ALG_SHA512},
- {HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+ {HASH_ALGO_SHA1, TPM_ALG_SHA1},
+ {HASH_ALGO_SHA256, TPM_ALG_SHA256},
+ {HASH_ALGO_SHA384, TPM_ALG_SHA384},
+ {HASH_ALGO_SHA512, TPM_ALG_SHA512},
+ {HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
};

/*
@@ -200,7 +200,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);

tpm_buf_append_u32(&buf, 1);
- tpm_buf_append_u16(&buf, TPM2_ALG_SHA1);
+ tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
tpm_buf_append(&buf, (const unsigned char *)pcr_select,
sizeof(pcr_select));
@@ -234,7 +234,7 @@ struct tpm2_null_auth_area {
* Return: Same as with tpm_transmit_cmd.
*/
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
- struct tpm2_digest *digests)
+ struct tpm_digest *digests)
{
struct tpm_buf buf;
struct tpm2_null_auth_area auth_area;
@@ -457,7 +457,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,

/* public */
tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
- tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
+ tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
tpm_buf_append_u16(&buf, hash);

/* policy */
@@ -472,7 +472,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
}

/* public parameters */
- tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+ tpm_buf_append_u16(&buf, TPM_ALG_NULL);
tpm_buf_append_u16(&buf, 0);

/* outside info */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4609b94142d4..71d7bbf5f690 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,12 +22,30 @@
#ifndef __LINUX_TPM_H__
#define __LINUX_TPM_H__

+#include <crypto/hash_info.h>
+
#define TPM_DIGEST_SIZE 20 /* Max TPM v1.2 PCR size */

struct tpm_chip;
struct trusted_key_payload;
struct trusted_key_options;

+enum tpm_algorithms {
+ TPM_ALG_ERROR = 0x0000,
+ TPM_ALG_SHA1 = 0x0004,
+ TPM_ALG_KEYEDHASH = 0x0008,
+ TPM_ALG_SHA256 = 0x000B,
+ TPM_ALG_SHA384 = 0x000C,
+ TPM_ALG_SHA512 = 0x000D,
+ TPM_ALG_NULL = 0x0010,
+ TPM_ALG_SM3_256 = 0x0012,
+};
+
+struct tpm_digest {
+ u16 alg_id;
+ u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
enum TPM_OPS_FLAGS {
TPM_OPS_AUTO_STARTUP = BIT(0),
};
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 3d5d162f09cc..c9f28b4be4ae 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -3,7 +3,7 @@
#ifndef __LINUX_TPM_EVENTLOG_H__
#define __LINUX_TPM_EVENTLOG_H__

-#include <crypto/hash_info.h>
+#include <linux/tpm.h>

#define TCG_EVENT_NAME_LEN_MAX 255
#define MAX_TEXT_EVENT 1000 /* Max event string length */
@@ -107,16 +107,11 @@ struct tcg_event_field {
u8 event[0];
} __packed;

-struct tpm2_digest {
- u16 alg_id;
- u8 digest[SHA512_DIGEST_SIZE];
-} __packed;
-
struct tcg_pcr_event2 {
u32 pcr_idx;
u32 event_type;
u32 count;
- struct tpm2_digest digests[0];
+ struct tpm_digest digests[0];
struct tcg_event_field event;
} __packed;

--
2.17.1

2018-11-06 15:08:45

by Roberto Sassu

[permalink] [raw]

Subject: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Currently the TPM driver allows other kernel subsystems to read only the
SHA1 PCR bank. This patch modifies the parameters of tpm_pcr_read() and
tpm2_pcr_read() to pass a tpm_digest structure, which contains the desired
hash algorithm. Also, since commit 125a22105410 ("tpm: React correctly to
RC_TESTING from TPM 2.0 self tests") removed the call to tpm2_pcr_read(),
the new parameter is expected to be always not NULL.

Due to the API change, IMA functions have been modified.

Signed-off-by: Roberto Sassu <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 9 +++++----
drivers/char/tpm/tpm.h | 3 ++-
drivers/char/tpm/tpm2-cmd.c | 23 ++++++++++++++++-------
include/linux/tpm.h | 6 ++++--
security/integrity/ima/ima_crypto.c | 10 +++++-----
5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 07b62734598c..e341ed9c232a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -976,11 +976,12 @@ EXPORT_SYMBOL_GPL(tpm_is_tpm2);
* tpm_pcr_read - read a PCR value from SHA1 bank
* @chip: a &struct tpm_chip instance, %NULL for the default chip
* @pcr_idx: the PCR to be retrieved
- * @res_buf: the value of the PCR
+ * @digest_struct: pcr bank and buffer current PCR value is written to
*
* Return: same as with tpm_transmit_cmd()
*/
-int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+ struct tpm_digest *digest_struct)
{
int rc;

@@ -988,9 +989,9 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
if (!chip)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2)
- rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
+ rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
else
- rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
+ rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
tpm_put_ops(chip);
return rc;
}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index fc2cc16e5080..2fd4379e75d6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -565,7 +565,8 @@ static inline u32 tpm2_rc_value(u32 rc)
return (rc & BIT(7)) ? rc & 0xff : rc;
}

-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+ struct tpm_digest *digest_struct);
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 584dba6cdf3e..499f4f17b3f3 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,16 +179,18 @@ struct tpm2_pcr_read_out {
* tpm2_pcr_read() - read a PCR value
* @chip: TPM chip to use.
* @pcr_idx: index of the PCR to read.
- * @res_buf: buffer to store the resulting hash.
+ * @digest_struct: pcr bank and buffer current PCR value is written to.
*
* Return: Same as with tpm_transmit_cmd.
*/
-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+ struct tpm_digest *digest_struct)
{
int rc;
struct tpm_buf buf;
struct tpm2_pcr_read_out *out;
u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
+ u16 digest_size;

if (pcr_idx >= TPM2_PLATFORM_PCR)
return -EINVAL;
@@ -200,18 +202,25 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);

tpm_buf_append_u32(&buf, 1);
- tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
+ tpm_buf_append_u16(&buf, digest_struct->alg_id);
tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
tpm_buf_append(&buf, (const unsigned char *)pcr_select,
sizeof(pcr_select));

rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
- res_buf ? "attempting to read a pcr value" : NULL);
- if (rc == 0 && res_buf) {
- out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
- memcpy(res_buf, out->digest, SHA1_DIGEST_SIZE);
+ "attempting to read a pcr value");
+ if (rc)
+ goto out;
+
+ out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
+ digest_size = be16_to_cpu(out->digest_size);
+ if (digest_size > sizeof(digest_struct->digest)) {
+ rc = -EINVAL;
+ goto out;
}

+ memcpy(digest_struct->digest, out->digest, digest_size);
+out:
tpm_buf_destroy(&buf);
return rc;
}
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 71d7bbf5f690..4f00daf44dd2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -71,7 +71,8 @@ struct tpm_class_ops {
#if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)

extern int tpm_is_tpm2(struct tpm_chip *chip);
-extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+ struct tpm_digest *digest_struct);
extern int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
@@ -87,7 +88,8 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
{
return -ENODEV;
}
-static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+ struct tpm_digest *digest_struct)
{
return -ENODEV;
}
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7e7e7e7c250a..8985e34eb3ac 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -629,12 +629,12 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
return calc_buffer_shash(buf, len, hash);
}

-static void __init ima_pcrread(int idx, u8 *pcr)
+static void __init ima_pcrread(int idx, struct tpm_digest *d)
{
if (!ima_tpm_chip)
return;

- if (tpm_pcr_read(ima_tpm_chip, idx, pcr) != 0)
+ if (tpm_pcr_read(ima_tpm_chip, idx, d) != 0)
pr_err("Error Communicating to TPM chip\n");
}

@@ -644,7 +644,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
static int __init ima_calc_boot_aggregate_tfm(char *digest,
struct crypto_shash *tfm)
{
- u8 pcr_i[TPM_DIGEST_SIZE];
+ struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
int rc, i;
SHASH_DESC_ON_STACK(shash, tfm);

@@ -657,9 +657,9 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,

/* cumulative sha1 over tpm registers 0-7 */
for (i = TPM_PCR0; i < TPM_PCR8; i++) {
- ima_pcrread(i, pcr_i);
+ ima_pcrread(i, &d);
/* now accumulate with current aggregate */
- rc = crypto_shash_update(shash, pcr_i, TPM_DIGEST_SIZE);
+ rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
}
if (!rc)
crypto_shash_final(shash, digest);
--
2.17.1

2018-11-06 15:09:17

by Roberto Sassu

[permalink] [raw]

Subject: [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read

Currently, the TPM driver retrieves the digest size from a table mapping
TPM algorithms identifiers to identifiers defined by the crypto subsystem.
If the algorithm is not defined by the latter, the digest size can be
retrieved from the output of the PCR read command.

The patch retrieves at TPM startup the digest sizes for each PCR bank and
stores them in the new structure tpm_bank_info, member of tpm_chip, so that
the information can be passed to other kernel subsystems.

tpm_bank_info contains: the TPM algorithm identifier, necessary to generate
the event log as defined by Trusted Computing Group (TCG); the digest size,
to pad/truncate a digest calculated with a different algorithm; the crypto
subsystem identifier, to calculate the digest of event data.

Signed-off-by: Roberto Sassu <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Acked-by: Mimi Zohar <[email protected]>
---
drivers/char/tpm/tpm-interface.c | 8 ++++--
drivers/char/tpm/tpm.h | 4 +--
drivers/char/tpm/tpm2-cmd.c | 47 ++++++++++++++++++++++++--------
include/linux/tpm.h | 6 ++++
4 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e341ed9c232a..c864b1645856 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -989,7 +989,7 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
if (!chip)
return -ENODEV;
if (chip->flags & TPM_CHIP_FLAG_TPM2)
- rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
+ rc = tpm2_pcr_read(chip, pcr_idx, digest_struct, NULL);
else
rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
tpm_put_ops(chip);
@@ -1057,7 +1057,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
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];
+ digest_list[i].alg_id = chip->active_banks[i].alg_id;
memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
}

@@ -1159,6 +1159,10 @@ int tpm1_auto_startup(struct tpm_chip *chip)
goto out;
}

+ chip->active_banks[0].alg_id = TPM_ALG_SHA1;
+ chip->active_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+ chip->active_banks[0].crypto_id = HASH_ALGO_SHA1;
+
return rc;
out:
if (rc > 0)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2fd4379e75d6..dfa54fc6c730 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -238,7 +238,7 @@ struct tpm_chip {
unsigned int groups_cnt;

u32 nr_active_banks;
- u16 *active_banks;
+ struct tpm_bank_info *active_banks;
#ifdef CONFIG_ACPI
acpi_handle acpi_dev_handle;
char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -566,7 +566,7 @@ static inline u32 tpm2_rc_value(u32 rc)
}

int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
- struct tpm_digest *digest_struct);
+ struct tpm_digest *digest_struct, u16 *digest_size_ptr);
int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm_digest *digests);
int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f4f17b3f3..e2d5b84286a7 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -180,11 +180,12 @@ struct tpm2_pcr_read_out {
* @chip: TPM chip to use.
* @pcr_idx: index of the PCR to read.
* @digest_struct: pcr bank and buffer current PCR value is written to.
+ * @digest_size_ptr: pointer to variable that stores the digest size.
*
* Return: Same as with tpm_transmit_cmd.
*/
int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
- struct tpm_digest *digest_struct)
+ struct tpm_digest *digest_struct, u16 *digest_size_ptr)
{
int rc;
struct tpm_buf buf;
@@ -219,6 +220,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
goto out;
}

+ if (digest_size_ptr)
+ *digest_size_ptr = digest_size;
+
memcpy(digest_struct->digest, out->digest, digest_size);
out:
tpm_buf_destroy(&buf);
@@ -249,7 +253,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
struct tpm2_null_auth_area auth_area;
int rc;
int i;
- int j;

if (count > chip->nr_active_banks)
return -EINVAL;
@@ -271,14 +274,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
tpm_buf_append_u32(&buf, count);

for (i = 0; i < count; i++) {
- for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
- if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
- continue;
- tpm_buf_append_u16(&buf, digests[i].alg_id);
- tpm_buf_append(&buf, (const unsigned char
- *)&digests[i].digest,
- hash_digest_size[tpm2_hash_map[j].crypto_id]);
- }
+ tpm_buf_append_u16(&buf, digests[i].alg_id);
+ tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+ chip->active_banks[i].digest_size);
}

rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -855,6 +853,26 @@ int tpm2_probe(struct tpm_chip *chip)
}
EXPORT_SYMBOL_GPL(tpm2_probe);

+static int tpm2_init_bank_info(struct tpm_chip *chip,
+ struct tpm_bank_info *bank)
+{
+ struct tpm_digest digest = { .alg_id = bank->alg_id };
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+ enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
+
+ if (bank->alg_id != tpm2_hash_map[i].tpm_id)
+ continue;
+
+ bank->digest_size = hash_digest_size[crypto_algo];
+ bank->crypto_id = crypto_algo;
+ return 0;
+ }
+
+ return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
+}
+
struct tpm2_pcr_selection {
__be16 hash_alg;
u8 size_of_select;
@@ -870,6 +888,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
void *pcr_select_offset;
u32 sizeof_pcr_selection;
u32 rsp_len;
+ u16 alg_id;
int rc;
int i = 0;

@@ -911,7 +930,13 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
}

memcpy(&pcr_selection, marker, sizeof(pcr_selection));
- chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
+ alg_id = be16_to_cpu(pcr_selection.hash_alg);
+ chip->active_banks[i].alg_id = alg_id;
+
+ rc = tpm2_init_bank_info(chip, &chip->active_banks[i]);
+ if (rc)
+ break;
+
sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
sizeof(pcr_selection.size_of_select) +
pcr_selection.size_of_select;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4f00daf44dd2..3f91124837cf 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -46,6 +46,12 @@ struct tpm_digest {
u8 digest[SHA512_DIGEST_SIZE];
} __packed;

+struct tpm_bank_info {
+ u16 alg_id;
+ u16 digest_size;
+ u16 crypto_id;
+};
+
enum TPM_OPS_FLAGS {
TPM_OPS_AUTO_STARTUP = BIT(0),
};
--
2.17.1

2018-11-06 15:11:26

by Roberto Sassu

[permalink] [raw]

Subject: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

This patch protects against data corruption that could happen in the bus,
by checking that that the digest size returned by the TPM during a PCR read
matches the size of the algorithm passed as argument to tpm2_pcr_read().

This check is performed after information about the PCR banks has been
retrieved.

Signed-off-by: Roberto Sassu <[email protected]>
---
drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e2d5b84286a7..3b0b5b032901 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
struct tpm_digest *digest_struct, u16 *digest_size_ptr)
{
+ int i;
int rc;
struct tpm_buf buf;
struct tpm2_pcr_read_out *out;
u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
u16 digest_size;
+ u16 expected_digest_size = 0;

if (pcr_idx >= TPM2_PLATFORM_PCR)
return -EINVAL;

+ if (!digest_size_ptr) {
+ for (i = 0; i < chip->nr_active_banks &&
+ chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
+ ;
+
+ if (i == chip->nr_active_banks)
+ return -EINVAL;
+
+ expected_digest_size = chip->active_banks[i].digest_size;
+ }
+
rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
if (rc)
return rc;
@@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,

out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
digest_size = be16_to_cpu(out->digest_size);
- if (digest_size > sizeof(digest_struct->digest)) {
+ if (digest_size > sizeof(digest_struct->digest) ||
+ (!digest_size_ptr && digest_size != expected_digest_size)) {
rc = -EINVAL;
goto out;
}
--
2.17.1

2018-11-07 06:18:35

by Nayna Jain

[permalink] [raw]

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

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 ?

> 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 <[email protected]>
> ---
> 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;

2018-11-07 09:44:44

by Roberto Sassu

[permalink] [raw]

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

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.

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 <[email protected]>
>> ---
>> 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;
>

--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI

2018-11-07 12:08:22

by Mimi Zohar

[permalink] [raw]

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

On Wed, 2018-11-07 at 11:44 +0530, Nayna Jain wrote:
> On 11/06/2018 08:31 PM, Roberto Sassu wrote:

> > @@ -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.

Instead of storing the result in a local variable, the only change
here is saving the result in the chip info (nr_active_banks).
Everything else remains the same.

> >
> > - if (count > ARRAY_SIZE(chip->active_banks)) {
> > - rc = -ENODEV;
> > + chip->active_banks = kmalloc_array(chip->nr_active_banks,
> > + sizeof(*chip->active_banks),
> > + GFP_KERNEL);

With this change, the exact number of banks can be allocated, as done
here. Nice!

Mimi

> > + if (!chip->active_banks) {
> > + rc = -ENOMEM;
> > goto out;
> > }
> >
>

2018-11-08 13:49:19

Subject: [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM

Subject: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

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

Subject: [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms

Subject: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read

Subject: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

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

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

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

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

Subject: Re: [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM

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

Subject: Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

Subject: Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

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

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

Subject: Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

Subject: Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

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

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

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

Subject: Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

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

Subject: Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

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

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

Subject: Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

Subject: Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size

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