[PATCH v5 0/7] tpm: retrieve digest size of unknown algorithms from TPM

[PATCH v5 0/7] tpm: retrieve digest size of unknown algorithms from TPM

[Roberto Sassu]

Update

This version of the patch set includes an additional patch (7/7) which
modifies the definition of tpm_pcr_extend() and tpm2_pcr_extend(). The new
patch has been included to facilitate the review of the changes to support
TPM 2.0 crypto agility for reading/extending PCRs.


Original patch set description

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

v4:
- rename active_banks to allocated_banks
- replace kmalloc_array() with kcalloc()
- increment nr_allocated_banks if at least one PCR in the bank is selected
- pass multiple digests to tpm_pcr_extend()

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 (7):
  tpm: dynamically allocate the allocated_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
  tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

 drivers/char/tpm/tpm-chip.c         |   1 +
 drivers/char/tpm/tpm-interface.c    |  36 +++----
 drivers/char/tpm/tpm.h              |  23 ++---
 drivers/char/tpm/tpm1-cmd.c         |  26 ++++-
 drivers/char/tpm/tpm2-cmd.c         | 155 +++++++++++++++++++++-------
 include/linux/tpm.h                 |  43 +++++++-
 include/linux/tpm_eventlog.h        |  12 +--
 security/integrity/ima/ima_crypto.c |  10 +-
 security/integrity/ima/ima_queue.c  |   5 +-
 security/keys/trusted.c             |   5 +-
 10 files changed, 213 insertions(+), 103 deletions(-)

-- 
2.17.1

[PATCH v5 1/7] tpm: dynamically allocate the allocated_banks array

[Roberto Sassu]

This patch renames active_banks (member of tpm_chip) to allocated_banks,
stores the number of allocated PCR banks in nr_allocated_banks (new member
of tpm_chip), and replaces the static array with a pointer to a dynamically
allocated array.

tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
the mask in the TPML_PCR_SELECTION structure returned by the TPM for
TPM2_Get_Capability(). One PCR bank with algorithm set to SHA1 is always
allocated for TPM 1.x.

As a consequence of the introduction of nr_allocated_banks,
tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
is equal to zero.

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

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm-chip.c      |  1 +
 drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
 drivers/char/tpm/tpm.h           |  3 ++-
 drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
 drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
 5 files changed, 47 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 32db84683c40..ce851c62bb68 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->allocated_banks);
 	kfree(chip);
 }
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index d9439f9abe78..7b80919228be 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
 int tpm_pcr_extend(struct tpm_chip *chip, u32 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);
@@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		memset(digest_list, 0, sizeof(digest_list));
+		digest_list = kcalloc(chip->nr_allocated_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++) {
-			digest_list[i].alg_id = chip->active_banks[i];
+		for (i = 0; i < chip->nr_allocated_banks; i++) {
+			digest_list[i].alg_id = chip->allocated_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_allocated_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 f27d1f38a93d..6b94306ab7c5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -257,7 +257,8 @@ struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	u32 nr_allocated_banks;
+	u16 *allocated_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 6f306338953b..0874743ca96d 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -709,6 +709,16 @@ 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] = TPM2_ALG_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 a6bec13afa69..245669a7aba5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -234,7 +234,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	int i;
 	int j;
 
-	if (count > ARRAY_SIZE(chip->active_banks))
+	if (count > chip->nr_allocated_banks)
 		return -EINVAL;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
@@ -825,11 +825,14 @@ 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 nr_possible_banks;
+	u32 nr_alloc_banks = 0;
+	u16 hash_alg;
 	u32 rsp_len;
 	int rc;
 	int i = 0;
+	int j;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
 	if (rc)
@@ -844,11 +847,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	if (rc)
 		goto out;
 
-	count = be32_to_cpup(
+	nr_possible_banks = be32_to_cpup(
 		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
 
-	if (count > ARRAY_SIZE(chip->active_banks)) {
-		rc = -ENODEV;
+	chip->allocated_banks = kcalloc(nr_possible_banks,
+					sizeof(*chip->allocated_banks),
+					GFP_KERNEL);
+	if (!chip->allocated_banks) {
+		rc = -ENOMEM;
 		goto out;
 	}
 
@@ -857,7 +863,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 < nr_possible_banks; i++) {
 		pcr_select_offset = marker +
 			offsetof(struct tpm2_pcr_selection, size_of_select);
 		if (pcr_select_offset >= end) {
@@ -866,17 +872,25 @@ 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);
+		hash_alg = be16_to_cpu(pcr_selection.hash_alg);
+
+		for (j = 0; j < pcr_selection.size_of_select; j++)
+			if (pcr_selection.pcr_select[j])
+				break;
+
+		if (j < pcr_selection.size_of_select) {
+			chip->allocated_banks[nr_alloc_banks] = hash_alg;
+			nr_alloc_banks++;
+		}
+
 		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
 			sizeof(pcr_selection.size_of_select) +
 			pcr_selection.size_of_select;
 		marker = marker + sizeof_pcr_selection;
 	}
 
+	chip->nr_allocated_banks = nr_alloc_banks;
 out:
-	if (i < ARRAY_SIZE(chip->active_banks))
-		chip->active_banks[i] = TPM2_ALG_ERROR;
-
 	tpm_buf_destroy(&buf);
 
 	return rc;
-- 
2.17.1

[PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Roberto Sassu]

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. Setting the array size to zero or 3 does not make any
difference, because the parser has to adjust the offset depending on the
actual array size to access structure members after the static arrays.

Thus, this patch removes the declaration of TPM2_ACTIVE_PCR_BANKS and sets
the array size to zero.

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

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 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

[PATCH v5 3/7] tpm: rename and export tpm2_digest and tpm2_algorithms

[Roberto Sassu]

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 <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c |  2 +-
 drivers/char/tpm/tpm.h           | 13 +------------
 drivers/char/tpm/tpm1-cmd.c      |  2 +-
 drivers/char/tpm/tpm2-cmd.c      | 18 +++++++++---------
 include/linux/tpm.h              | 18 ++++++++++++++++++
 include/linux/tpm_eventlog.h     |  9 ++-------
 6 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7b80919228be..96c8261d0f04 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -488,7 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
 int tpm_pcr_extend(struct tpm_chip *chip, u32 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 6b94306ab7c5..e961e5c5d197 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_HIERARCHY_CONTROL       = 0x0121,
@@ -561,7 +550,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 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/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 0874743ca96d..ec4f197bae0a 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -716,7 +716,7 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	chip->allocated_banks[0] = TPM2_ALG_SHA1;
+	chip->allocated_banks[0] = TPM_ALG_SHA1;
 	chip->nr_allocated_banks = 1;
 
 	return rc;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 245669a7aba5..4afd14892cbf 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},
 };
 
 int tpm2_get_timeouts(struct tpm_chip *chip)
@@ -192,7 +192,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 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));
@@ -226,7 +226,7 @@ struct tpm2_null_auth_area {
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm2_digest *digests)
+		    struct tpm_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
@@ -449,7 +449,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 */
@@ -464,7 +464,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 b49a55cf775f..330c0a481581 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

[PATCH v5 4/7] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

[Roberto Sassu]

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 <roberto.sassu@huawei.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 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 96c8261d0f04..04aa39432f58 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -451,11 +451,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:	the 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, u32 pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+		 struct tpm_digest *digest_struct)
 {
 	int rc;
 
@@ -464,9 +465,9 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 		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 = tpm1_pcr_read(chip, pcr_idx, res_buf);
+		rc = tpm1_pcr_read(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 e961e5c5d197..33b293da69d6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -548,7 +548,8 @@ static inline u32 tpm2_rc_value(u32 rc)
 }
 
 int tpm2_get_timeouts(struct tpm_chip *chip);
-int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
+int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+		  struct tpm_digest *digest_struct);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 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 4afd14892cbf..0d72c2f98bd6 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -171,16 +171,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, u32 pcr_idx, u8 *res_buf)
+int tpm2_pcr_read(struct tpm_chip *chip, u32 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;
@@ -192,18 +194,25 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 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 330c0a481581..077fd31783ad 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, u32 pcr_idx, u8 *res_buf);
+extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+			struct tpm_digest *digest_struct);
 extern int tpm_pcr_extend(struct tpm_chip *chip, u32 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);
@@ -88,7 +89,8 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
 	return -ENODEV;
 }
 
-static inline int tpm_pcr_read(struct tpm_chip *chip, u32 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 acf2c7df7145..16a4f45863b1 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -643,12 +643,12 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
 	return calc_buffer_shash(buf, len, hash);
 }
 
-static void __init ima_pcrread(u32 idx, u8 *pcr)
+static void __init ima_pcrread(u32 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");
 }
 
@@ -658,7 +658,7 @@ static void __init ima_pcrread(u32 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;
 	u32 i;
 	SHASH_DESC_ON_STACK(shash, tfm);
@@ -672,9 +672,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

[PATCH v5 5/7] tpm: retrieve digest size of unknown algorithms with PCR read

[Roberto Sassu]

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 <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Acked-by: Mimi Zohar <zohar@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c |  9 +++----
 drivers/char/tpm/tpm.h           |  4 +--
 drivers/char/tpm/tpm1-cmd.c      |  4 ++-
 drivers/char/tpm/tpm2-cmd.c      | 45 ++++++++++++++++++++++++--------
 include/linux/tpm.h              |  6 +++++
 5 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 04aa39432f58..f0e1456440d7 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -465,7 +465,7 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		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 = tpm1_pcr_read(chip, pcr_idx, digest_struct->digest);
 
@@ -480,9 +480,8 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  * @pcr_idx:	the PCR to be retrieved
  * @hash:	the hash value used to extend the PCR value
  *
- * Note: with TPM 2.0 extends also those banks with a known digest size to the
- * cryto subsystem in order to prevent malicious use of those PCR banks. In the
- * future we should dynamically determine digest sizes.
+ * Note: with TPM 2.0 extends also those banks for which no digest was
+ * specified in order to prevent malicious use of those PCR banks.
  *
  * Return: same as with tpm_transmit_cmd()
  */
@@ -503,7 +502,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
 			return -ENOMEM;
 
 		for (i = 0; i < chip->nr_allocated_banks; i++) {
-			digest_list[i].alg_id = chip->allocated_banks[i];
+			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 		}
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 33b293da69d6..023ddf42038b 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -247,7 +247,7 @@ struct tpm_chip {
 	unsigned int groups_cnt;
 
 	u32 nr_allocated_banks;
-	u16 *allocated_banks;
+	struct tpm_bank_info *allocated_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -549,7 +549,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 
 int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
-		  struct tpm_digest *digest_struct);
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 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/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index ec4f197bae0a..8b70a7f884a7 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -716,7 +716,9 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	chip->allocated_banks[0] = TPM_ALG_SHA1;
+	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;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0d72c2f98bd6..acaaab72ef2e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -172,11 +172,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, u32 pcr_idx,
-		  struct tpm_digest *digest_struct)
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
 	int rc;
 	struct tpm_buf buf;
@@ -211,6 +212,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 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);
@@ -241,7 +245,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > chip->nr_allocated_banks)
 		return -EINVAL;
@@ -263,14 +266,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 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->allocated_banks[i].digest_size);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -821,6 +819,26 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
+{
+	struct tpm_bank_info *bank = chip->allocated_banks + bank_index;
+	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;
@@ -888,7 +906,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 				break;
 
 		if (j < pcr_selection.size_of_select) {
-			chip->allocated_banks[nr_alloc_banks] = hash_alg;
+			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
+
+			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
+			if (rc < 0)
+				break;
+
 			nr_alloc_banks++;
 		}
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 077fd31783ad..fcdd33ae9969 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

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

[Roberto Sassu]

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 to tpm2_pcr_read().

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

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: stable@vger.kernel.org
---
 drivers/char/tpm/tpm2-cmd.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index acaaab72ef2e..974465f04b78 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,15 +179,29 @@ struct tpm2_pcr_read_out {
 int tpm2_pcr_read(struct tpm_chip *chip, u32 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_allocated_banks &&
+		     chip->allocated_banks[i].alg_id != digest_struct->alg_id;
+		     i++)
+			;
+
+		if (i == chip->nr_allocated_banks)
+			return -EINVAL;
+
+		expected_digest_size = chip->allocated_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -207,7 +221,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 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

[PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Roberto Sassu]

Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.

This patch modifies the definition of tpm_pcr_extend() to allow other
kernel subsystems to pass a digest for each algorithm supported by the TPM.
All digests are processed by the TPM in one operation.

If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
the TPM driver extends the remaining PCR banks with the first digest
passed as an argument to the function.

The new tpm_bank_list structure has been preferred to the tpm_digest
structure, to let the caller specify the size of the digest (which may be
unknown to the TPM driver).

Due to the API change, ima_pcr_extend() and pcrlock() have been modified.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
 drivers/char/tpm/tpm.h             |  6 ++---
 drivers/char/tpm/tpm1-cmd.c        | 14 ++++++++----
 drivers/char/tpm/tpm2-cmd.c        | 36 +++++++++++++++++++++---------
 include/linux/tpm.h                | 13 ++++++++---
 security/integrity/ima/ima_queue.c |  5 ++++-
 security/keys/trusted.c            |  5 ++++-
 7 files changed, 63 insertions(+), 40 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index f0e1456440d7..5495223d3f7b 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  * tpm_pcr_extend - extend a PCR value in SHA1 bank.
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
  * @pcr_idx:	the PCR to be retrieved
- * @hash:	the hash value used to extend the PCR value
+ * @count:	number of tpm_bank_list structures
+ * @bank_list:	array of tpm_bank_list structures used to extend the PCR value
  *
  * Note: with TPM 2.0 extends also those banks for which no digest was
  * specified in order to prevent malicious use of those PCR banks.
  *
  * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
+int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		   const struct tpm_bank_list *bank_list)
 {
 	int rc;
-	struct tpm_digest *digest_list;
-	int i;
 
 	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		digest_list = kcalloc(chip->nr_allocated_banks,
-				      sizeof(*digest_list), GFP_KERNEL);
-		if (!digest_list)
-			return -ENOMEM;
-
-		for (i = 0; i < chip->nr_allocated_banks; i++) {
-			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
-			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-		}
-
-		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
-				     digest_list);
-		kfree(digest_list);
+		rc = tpm2_pcr_extend(chip, pcr_idx, count, bank_list);
 		tpm_put_ops(chip);
 		return rc;
 	}
 
-	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
+	rc = tpm1_pcr_extend(chip, pcr_idx, count, bank_list,
 			     "attempting extend a PCR value");
 	tpm_put_ops(chip);
 	return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 023ddf42038b..4296c720ebb4 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -504,8 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm1_do_selftest(struct tpm_chip *chip);
 int tpm1_get_timeouts(struct tpm_chip *chip);
 unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
-int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
-		    const char *log_msg);
+int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		    const struct tpm_bank_list *bank_list, const char *log_msg);
 int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
 ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
 		    const char *desc, size_t min_cap_length);
@@ -551,7 +551,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm_digest *digests);
+		    const struct tpm_bank_list *bank_list);
 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/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 8b70a7f884a7..439ac749517c 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -449,12 +449,19 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
 }
 
 #define TPM_ORD_PCR_EXTEND 20
-int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
-		    const char *log_msg)
+int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		    const struct tpm_bank_list *bank_list, const char *log_msg)
 {
 	struct tpm_buf buf;
+	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
+	const u8 *hash;
 	int rc;
 
+	hash = dummy_hash;
+	if (count)
+		memcpy(dummy_hash, bank_list[0].extend_array,
+		       min(bank_list[0].digest_size, (u16)sizeof(dummy_hash)));
+
 	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
 	if (rc)
 		return rc;
@@ -743,7 +750,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
  */
 int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 {
-	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
 	struct tpm_buf buf;
 	unsigned int try;
 	int rc;
@@ -751,7 +757,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 
 	/* for buggy tpm, flush pcrs with extend to selected dummy */
 	if (tpm_suspend_pcr)
-		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
+		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
 				     "extending dummy pcr before suspend");
 
 	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 974465f04b78..40eb1a044451 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -248,21 +248,22 @@ struct tpm2_null_auth_area {
  *
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR.
- * @count:	number of digests passed.
- * @digests:	list of pcr banks and corresponding digest values to extend.
+ * @count:	number of tpm_bank_list passed.
+ * @bank_list:	array of tpm_bank_list with digest values to extend.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm_digest *digests)
+		    const struct tpm_bank_list *bank_list)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
+	const struct tpm_bank_list *item;
+	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
+	const u8 *hash;
 	int rc;
 	int i;
-
-	if (count > chip->nr_allocated_banks)
-		return -EINVAL;
+	int j;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
 	if (rc)
@@ -278,11 +279,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
 	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
 		       sizeof(auth_area));
-	tpm_buf_append_u32(&buf, count);
+	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+
+	if (count)
+		memcpy(dummy_hash, bank_list[0].extend_array,
+		       bank_list[0].digest_size);
+
+	for (i = 0; i < chip->nr_allocated_banks; i++) {
+		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
+
+		hash = dummy_hash;
+		for (j = 0; j < count; j++) {
+			item = bank_list + j;
+
+			if (item->alg_id == chip->allocated_banks[i].alg_id) {
+				hash = item->extend_array;
+				break;
+			}
+		}
 
-	for (i = 0; i < count; i++) {
-		tpm_buf_append_u16(&buf, digests[i].alg_id);
-		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+		tpm_buf_append(&buf, hash,
 			       chip->allocated_banks[i].digest_size);
 	}
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index fcdd33ae9969..16e5ff1f0294 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -52,6 +52,12 @@ struct tpm_bank_info {
 	u16 crypto_id;
 };
 
+struct tpm_bank_list {
+	u8 alg_id;
+	u16 digest_size;
+	const u8 *extend_array;
+};
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
@@ -79,7 +85,8 @@ struct tpm_class_ops {
 extern int tpm_is_tpm2(struct tpm_chip *chip);
 extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 			struct tpm_digest *digest_struct);
-extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
+extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+			  const struct tpm_bank_list *bank_list);
 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);
 extern int tpm_seal_trusted(struct tpm_chip *chip,
@@ -101,8 +108,8 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
 	return -ENODEV;
 }
 
-static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
-				 const u8 *hash)
+static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+				 const struct tpm_bank_list *bank_list)
 {
 	return -ENODEV;
 }
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index b186819bd5aa..24024edef316 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -140,12 +140,15 @@ unsigned long ima_get_binary_runtime_size(void)
 
 static int ima_pcr_extend(const u8 *hash, int pcr)
 {
+	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
+					   .digest_size = TPM_DIGEST_SIZE,
+					   .extend_array = hash };
 	int result = 0;
 
 	if (!ima_tpm_chip)
 		return result;
 
-	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
+	result = tpm_pcr_extend(ima_tpm_chip, pcr, 1, &bank_item);
 	if (result != 0)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	return result;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ff6789365a12..d2a3129a95f9 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -380,6 +380,9 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send);
 static int pcrlock(const int pcrnum)
 {
 	unsigned char hash[SHA1_DIGEST_SIZE];
+	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
+					   .digest_size = sizeof(hash),
+					   .extend_array = hash }
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -387,7 +390,7 @@ static int pcrlock(const int pcrnum)
 	ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
 	if (ret != SHA1_DIGEST_SIZE)
 		return ret;
-	return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
+	return tpm_pcr_extend(NULL, pcrnum, 1, &bank_item) ? -EINVAL : 0;
 }
 
 /*
-- 
2.17.1

Re: [PATCH v5 1/7] tpm: dynamically allocate the allocated_banks array

[Jarkko Sakkinen]

On Wed, Nov 14, 2018 at 04:31:02PM +0100, Roberto Sassu wrote:
> This patch renames active_banks (member of tpm_chip) to allocated_banks,
> stores the number of allocated PCR banks in nr_allocated_banks (new member
> of tpm_chip), and replaces the static array with a pointer to a dynamically
> allocated array.
> 
> tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
> the mask in the TPML_PCR_SELECTION structure returned by the TPM for
> TPM2_Get_Capability(). One PCR bank with algorithm set to SHA1 is always
> allocated for TPM 1.x.
> 
> As a consequence of the introduction of nr_allocated_banks,
> tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
> is equal to zero.
> 
> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks")
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

There should not be a newline between tags and I don't think this should
have fixes tag. Otherwise, looks good.

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Jarkko Sakkinen]

On Wed, Nov 14, 2018 at 04:31:03PM +0100, Roberto Sassu wrote:
> 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. Setting the array size to zero or 3 does not make any
> difference, because the parser has to adjust the offset depending on the
> actual array size to access structure members after the static arrays.
> 
> Thus, this patch removes the declaration of TPM2_ACTIVE_PCR_BANKS and sets
> the array size to zero.
> 
> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
> event log")
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  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
> 

NAK for the same reason as last time.

/Jarkko

Re: [PATCH v5 4/7] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm

[Jarkko Sakkinen]

On Wed, Nov 14, 2018 at 04:31:05PM +0100, Roberto Sassu wrote:
> 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 <roberto.sassu@huawei.com>
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  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 96c8261d0f04..04aa39432f58 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -451,11 +451,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:	the 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, u32 pcr_idx, u8 *res_buf)
> +int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> +		 struct tpm_digest *digest_struct)

Should be just struct tpm_digest *digest

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> 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 to tpm2_pcr_read().
> 
> This check is performed after information about the PCR banks has been
> retrieved.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Cc: stable@vger.kernel.org

Missing fixes tag.

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Jarkko Sakkinen]

On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> 
> This patch modifies the definition of tpm_pcr_extend() to allow other
> kernel subsystems to pass a digest for each algorithm supported by the TPM.
> All digests are processed by the TPM in one operation.
> 
> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> the TPM driver extends the remaining PCR banks with the first digest
> passed as an argument to the function.

What is the legit use case for this?

> The new tpm_bank_list structure has been preferred to the tpm_digest
> structure, to let the caller specify the size of the digest (which may be
> unknown to the TPM driver).
> 
> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Should be dropped from the patch set as it is not taken advatange of
but I can still try to give some feedback.

> ---
>  drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>  drivers/char/tpm/tpm.h             |  6 ++---
>  drivers/char/tpm/tpm1-cmd.c        | 14 ++++++++----
>  drivers/char/tpm/tpm2-cmd.c        | 36 +++++++++++++++++++++---------
>  include/linux/tpm.h                | 13 ++++++++---
>  security/integrity/ima/ima_queue.c |  5 ++++-
>  security/keys/trusted.c            |  5 ++++-
>  7 files changed, 63 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index f0e1456440d7..5495223d3f7b 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>   * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>   * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>   * @pcr_idx:	the PCR to be retrieved
> - * @hash:	the hash value used to extend the PCR value
> + * @count:	number of tpm_bank_list structures
> + * @bank_list:	array of tpm_bank_list structures used to extend the PCR value
>   *
>   * Note: with TPM 2.0 extends also those banks for which no digest was
>   * specified in order to prevent malicious use of those PCR banks.
>   *
>   * Return: same as with tpm_transmit_cmd()
>   */
> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		   const struct tpm_bank_list *bank_list)
>  {
>  	int rc;
> -	struct tpm_digest *digest_list;
> -	int i;
>  
>  	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		digest_list = kcalloc(chip->nr_allocated_banks,
> -				      sizeof(*digest_list), GFP_KERNEL);
> -		if (!digest_list)
> -			return -ENOMEM;
> -
> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -		}
> -
> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
> -				     digest_list);
> -		kfree(digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, bank_list);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
>  
> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, bank_list,
>  			     "attempting extend a PCR value");
>  	tpm_put_ops(chip);
>  	return rc;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 023ddf42038b..4296c720ebb4 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -504,8 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm1_do_selftest(struct tpm_chip *chip);
>  int tpm1_get_timeouts(struct tpm_chip *chip);
>  unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> -		    const char *log_msg);
> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		    const struct tpm_bank_list *bank_list, const char *log_msg);
>  int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>  ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>  		    const char *desc, size_t min_cap_length);
> @@ -551,7 +551,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>  int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> -		    struct tpm_digest *digests);
> +		    const struct tpm_bank_list *bank_list);
>  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/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index 8b70a7f884a7..439ac749517c 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -449,12 +449,19 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>  }
>  
>  #define TPM_ORD_PCR_EXTEND 20
> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> -		    const char *log_msg)
> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		    const struct tpm_bank_list *bank_list, const char *log_msg)
>  {
>  	struct tpm_buf buf;
> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> +	const u8 *hash;
>  	int rc;
>  
> +	hash = dummy_hash;
> +	if (count)
> +		memcpy(dummy_hash, bank_list[0].extend_array,
> +		       min(bank_list[0].digest_size, (u16)sizeof(dummy_hash)));
> +
>  	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);

This is probably mistake? I mean dummy_hash is not used.

>  	if (rc)
>  		return rc;
> @@ -743,7 +750,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>   */
>  int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>  {
> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>  	struct tpm_buf buf;
>  	unsigned int try;
>  	int rc;
> @@ -751,7 +757,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>  
>  	/* for buggy tpm, flush pcrs with extend to selected dummy */
>  	if (tpm_suspend_pcr)
> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>  				     "extending dummy pcr before suspend");
>  
>  	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 974465f04b78..40eb1a044451 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -248,21 +248,22 @@ struct tpm2_null_auth_area {
>   *
>   * @chip:	TPM chip to use.
>   * @pcr_idx:	index of the PCR.
> - * @count:	number of digests passed.
> - * @digests:	list of pcr banks and corresponding digest values to extend.
> + * @count:	number of tpm_bank_list passed.
> + * @bank_list:	array of tpm_bank_list with digest values to extend.
>   *
>   * Return: Same as with tpm_transmit_cmd.
>   */
>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> -		    struct tpm_digest *digests)
> +		    const struct tpm_bank_list *bank_list)
>  {
>  	struct tpm_buf buf;
>  	struct tpm2_null_auth_area auth_area;
> +	const struct tpm_bank_list *item;
> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
> +	const u8 *hash;
>  	int rc;
>  	int i;
> -
> -	if (count > chip->nr_allocated_banks)
> -		return -EINVAL;
> +	int j;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>  	if (rc)
> @@ -278,11 +279,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>  	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>  	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>  		       sizeof(auth_area));
> -	tpm_buf_append_u32(&buf, count);
> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> +
> +	if (count)
> +		memcpy(dummy_hash, bank_list[0].extend_array,
> +		       bank_list[0].digest_size);
> +
> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
> +
> +		hash = dummy_hash;
> +		for (j = 0; j < count; j++) {
> +			item = bank_list + j;
> +
> +			if (item->alg_id == chip->allocated_banks[i].alg_id) {
> +				hash = item->extend_array;
> +				break;
> +			}
> +		}
>  
> -	for (i = 0; i < count; i++) {
> -		tpm_buf_append_u16(&buf, digests[i].alg_id);
> -		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
> +		tpm_buf_append(&buf, hash,
>  			       chip->allocated_banks[i].digest_size);
>  	}
>  
> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> index fcdd33ae9969..16e5ff1f0294 100644
> --- a/include/linux/tpm.h
> +++ b/include/linux/tpm.h
> @@ -52,6 +52,12 @@ struct tpm_bank_info {
>  	u16 crypto_id;
>  };
>  
> +struct tpm_bank_list {
> +	u8 alg_id;
> +	u16 digest_size;
> +	const u8 *extend_array;
> +};

You should document this structure.

> +
>  enum TPM_OPS_FLAGS {
>  	TPM_OPS_AUTO_STARTUP = BIT(0),
>  };
> @@ -79,7 +85,8 @@ struct tpm_class_ops {
>  extern int tpm_is_tpm2(struct tpm_chip *chip);
>  extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  			struct tpm_digest *digest_struct);
> -extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
> +extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +			  const struct tpm_bank_list *bank_list);
>  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);
>  extern int tpm_seal_trusted(struct tpm_chip *chip,
> @@ -101,8 +108,8 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  	return -ENODEV;
>  }
>  
> -static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
> -				 const u8 *hash)
> +static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +				 const struct tpm_bank_list *bank_list)
>  {
>  	return -ENODEV;
>  }
> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
> index b186819bd5aa..24024edef316 100644
> --- a/security/integrity/ima/ima_queue.c
> +++ b/security/integrity/ima/ima_queue.c
> @@ -140,12 +140,15 @@ unsigned long ima_get_binary_runtime_size(void)
>  
>  static int ima_pcr_extend(const u8 *hash, int pcr)
>  {
> +	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
> +					   .digest_size = TPM_DIGEST_SIZE,
> +					   .extend_array = hash };

Item is a list?

>  	int result = 0;
>  
>  	if (!ima_tpm_chip)
>  		return result;
>  
> -	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, 1, &bank_item);
>  	if (result != 0)
>  		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>  	return result;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index ff6789365a12..d2a3129a95f9 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -380,6 +380,9 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send);
>  static int pcrlock(const int pcrnum)
>  {
>  	unsigned char hash[SHA1_DIGEST_SIZE];
> +	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
> +					   .digest_size = sizeof(hash),
> +					   .extend_array = hash }
>  	int ret;
>  
>  	if (!capable(CAP_SYS_ADMIN))
> @@ -387,7 +390,7 @@ static int pcrlock(const int pcrnum)
>  	ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
>  	if (ret != SHA1_DIGEST_SIZE)
>  		return ret;
> -	return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
> +	return tpm_pcr_extend(NULL, pcrnum, 1, &bank_item) ? -EINVAL : 0;
>  }
>  
>  /*
> -- 
> 2.17.1
> 

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Roberto Sassu]

On 11/16/2018 4:03 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>
>> This patch modifies the definition of tpm_pcr_extend() to allow other
>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>> All digests are processed by the TPM in one operation.
>>
>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>> the TPM driver extends the remaining PCR banks with the first digest
>> passed as an argument to the function.
> 
> What is the legit use case for this?

A subset could be chosen for better performance, or when a TPM algorithm
is not supported by the crypto subsystem.


>> The new tpm_bank_list structure has been preferred to the tpm_digest
>> structure, to let the caller specify the size of the digest (which may be
>> unknown to the TPM driver).
>>
>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Should be dropped from the patch set as it is not taken advatange of
> but I can still try to give some feedback.

I understood from a previous email that you want to include all API
changes for crypto agility in the same patch set.

Roberto


>> ---
>>   drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>>   drivers/char/tpm/tpm.h             |  6 ++---
>>   drivers/char/tpm/tpm1-cmd.c        | 14 ++++++++----
>>   drivers/char/tpm/tpm2-cmd.c        | 36 +++++++++++++++++++++---------
>>   include/linux/tpm.h                | 13 ++++++++---
>>   security/integrity/ima/ima_queue.c |  5 ++++-
>>   security/keys/trusted.c            |  5 ++++-
>>   7 files changed, 63 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index f0e1456440d7..5495223d3f7b 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>    * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>>    * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>>    * @pcr_idx:	the PCR to be retrieved
>> - * @hash:	the hash value used to extend the PCR value
>> + * @count:	number of tpm_bank_list structures
>> + * @bank_list:	array of tpm_bank_list structures used to extend the PCR value
>>    *
>>    * Note: with TPM 2.0 extends also those banks for which no digest was
>>    * specified in order to prevent malicious use of those PCR banks.
>>    *
>>    * Return: same as with tpm_transmit_cmd()
>>    */
>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		   const struct tpm_bank_list *bank_list)
>>   {
>>   	int rc;
>> -	struct tpm_digest *digest_list;
>> -	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>>   	if (!chip)
>>   		return -ENODEV;
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		digest_list = kcalloc(chip->nr_allocated_banks,
>> -				      sizeof(*digest_list), GFP_KERNEL);
>> -		if (!digest_list)
>> -			return -ENOMEM;
>> -
>> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
>> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
>> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -		}
>> -
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>> -				     digest_list);
>> -		kfree(digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, bank_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>>   
>> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
>> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, bank_list,
>>   			     "attempting extend a PCR value");
>>   	tpm_put_ops(chip);
>>   	return rc;
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 023ddf42038b..4296c720ebb4 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -504,8 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>>   int tpm1_do_selftest(struct tpm_chip *chip);
>>   int tpm1_get_timeouts(struct tpm_chip *chip);
>>   unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>> -		    const char *log_msg);
>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		    const struct tpm_bank_list *bank_list, const char *log_msg);
>>   int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>>   ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>>   		    const char *desc, size_t min_cap_length);
>> @@ -551,7 +551,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>>   int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> -		    struct tpm_digest *digests);
>> +		    const struct tpm_bank_list *bank_list);
>>   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/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>> index 8b70a7f884a7..439ac749517c 100644
>> --- a/drivers/char/tpm/tpm1-cmd.c
>> +++ b/drivers/char/tpm/tpm1-cmd.c
>> @@ -449,12 +449,19 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>>   }
>>   
>>   #define TPM_ORD_PCR_EXTEND 20
>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>> -		    const char *log_msg)
>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		    const struct tpm_bank_list *bank_list, const char *log_msg)
>>   {
>>   	struct tpm_buf buf;
>> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>> +	const u8 *hash;
>>   	int rc;
>>   
>> +	hash = dummy_hash;
>> +	if (count)
>> +		memcpy(dummy_hash, bank_list[0].extend_array,
>> +		       min(bank_list[0].digest_size, (u16)sizeof(dummy_hash)));
>> +
>>   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
> 
> This is probably mistake? I mean dummy_hash is not used.
> 
>>   	if (rc)
>>   		return rc;
>> @@ -743,7 +750,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>    */
>>   int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>   {
>> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>   	struct tpm_buf buf;
>>   	unsigned int try;
>>   	int rc;
>> @@ -751,7 +757,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>   
>>   	/* for buggy tpm, flush pcrs with extend to selected dummy */
>>   	if (tpm_suspend_pcr)
>> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
>> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>>   				     "extending dummy pcr before suspend");
>>   
>>   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 974465f04b78..40eb1a044451 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -248,21 +248,22 @@ struct tpm2_null_auth_area {
>>    *
>>    * @chip:	TPM chip to use.
>>    * @pcr_idx:	index of the PCR.
>> - * @count:	number of digests passed.
>> - * @digests:	list of pcr banks and corresponding digest values to extend.
>> + * @count:	number of tpm_bank_list passed.
>> + * @bank_list:	array of tpm_bank_list with digest values to extend.
>>    *
>>    * Return: Same as with tpm_transmit_cmd.
>>    */
>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> -		    struct tpm_digest *digests)
>> +		    const struct tpm_bank_list *bank_list)
>>   {
>>   	struct tpm_buf buf;
>>   	struct tpm2_null_auth_area auth_area;
>> +	const struct tpm_bank_list *item;
>> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
>> +	const u8 *hash;
>>   	int rc;
>>   	int i;
>> -
>> -	if (count > chip->nr_allocated_banks)
>> -		return -EINVAL;
>> +	int j;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>   	if (rc)
>> @@ -278,11 +279,26 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>   	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>>   	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>>   		       sizeof(auth_area));
>> -	tpm_buf_append_u32(&buf, count);
>> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>> +
>> +	if (count)
>> +		memcpy(dummy_hash, bank_list[0].extend_array,
>> +		       bank_list[0].digest_size);
>> +
>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
>> +
>> +		hash = dummy_hash;
>> +		for (j = 0; j < count; j++) {
>> +			item = bank_list + j;
>> +
>> +			if (item->alg_id == chip->allocated_banks[i].alg_id) {
>> +				hash = item->extend_array;
>> +				break;
>> +			}
>> +		}
>>   
>> -	for (i = 0; i < count; i++) {
>> -		tpm_buf_append_u16(&buf, digests[i].alg_id);
>> -		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
>> +		tpm_buf_append(&buf, hash,
>>   			       chip->allocated_banks[i].digest_size);
>>   	}
>>   
>> diff --git a/include/linux/tpm.h b/include/linux/tpm.h
>> index fcdd33ae9969..16e5ff1f0294 100644
>> --- a/include/linux/tpm.h
>> +++ b/include/linux/tpm.h
>> @@ -52,6 +52,12 @@ struct tpm_bank_info {
>>   	u16 crypto_id;
>>   };
>>   
>> +struct tpm_bank_list {
>> +	u8 alg_id;
>> +	u16 digest_size;
>> +	const u8 *extend_array;
>> +};
> 
> You should document this structure.
> 
>> +
>>   enum TPM_OPS_FLAGS {
>>   	TPM_OPS_AUTO_STARTUP = BIT(0),
>>   };
>> @@ -79,7 +85,8 @@ struct tpm_class_ops {
>>   extern int tpm_is_tpm2(struct tpm_chip *chip);
>>   extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   			struct tpm_digest *digest_struct);
>> -extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
>> +extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +			  const struct tpm_bank_list *bank_list);
>>   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);
>>   extern int tpm_seal_trusted(struct tpm_chip *chip,
>> @@ -101,8 +108,8 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   	return -ENODEV;
>>   }
>>   
>> -static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
>> -				 const u8 *hash)
>> +static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +				 const struct tpm_bank_list *bank_list)
>>   {
>>   	return -ENODEV;
>>   }
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index b186819bd5aa..24024edef316 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -140,12 +140,15 @@ unsigned long ima_get_binary_runtime_size(void)
>>   
>>   static int ima_pcr_extend(const u8 *hash, int pcr)
>>   {
>> +	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
>> +					   .digest_size = TPM_DIGEST_SIZE,
>> +					   .extend_array = hash };
> 
> Item is a list?
> 
>>   	int result = 0;
>>   
>>   	if (!ima_tpm_chip)
>>   		return result;
>>   
>> -	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
>> +	result = tpm_pcr_extend(ima_tpm_chip, pcr, 1, &bank_item);
>>   	if (result != 0)
>>   		pr_err("Error Communicating to TPM chip, result: %d\n", result);
>>   	return result;
>> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
>> index ff6789365a12..d2a3129a95f9 100644
>> --- a/security/keys/trusted.c
>> +++ b/security/keys/trusted.c
>> @@ -380,6 +380,9 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send);
>>   static int pcrlock(const int pcrnum)
>>   {
>>   	unsigned char hash[SHA1_DIGEST_SIZE];
>> +	struct tpm_bank_list bank_item = { .alg_id = TPM_ALG_SHA1,
>> +					   .digest_size = sizeof(hash),
>> +					   .extend_array = hash }
>>   	int ret;
>>   
>>   	if (!capable(CAP_SYS_ADMIN))
>> @@ -387,7 +390,7 @@ static int pcrlock(const int pcrnum)
>>   	ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
>>   	if (ret != SHA1_DIGEST_SIZE)
>>   		return ret;
>> -	return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
>> +	return tpm_pcr_extend(NULL, pcrnum, 1, &bank_item) ? -EINVAL : 0;
>>   }
>>   
>>   /*
>> -- 
>> 2.17.1
>>
> 
> /Jarkko
> 

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

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

[Roberto Sassu]

On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>> 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 to tpm2_pcr_read().
>>
>> This check is performed after information about the PCR banks has been
>> retrieved.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> Cc: stable@vger.kernel.org
> 
> Missing fixes tag.

Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
output sent by the TPM.

Roberto


> /Jarkko
> 

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

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Jarkko Sakkinen]

On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
> On 11/16/2018 4:03 PM, Jarkko Sakkinen wrote:
> > On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
> > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> > > 
> > > This patch modifies the definition of tpm_pcr_extend() to allow other
> > > kernel subsystems to pass a digest for each algorithm supported by the TPM.
> > > All digests are processed by the TPM in one operation.
> > > 
> > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> > > the TPM driver extends the remaining PCR banks with the first digest
> > > passed as an argument to the function.
> > 
> > What is the legit use case for this?
> 
> A subset could be chosen for better performance, or when a TPM algorithm
> is not supported by the crypto subsystem.

Doesn't extending a subset a security concern?

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Jarkko Sakkinen]

On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
> I understood from a previous email that you want to include all API
> changes for crypto agility in the same patch set.

Hmm.. maybe there is some misunderstading. Can you point me to the
comment? Thanks.

/Jarkko

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

[Jarkko Sakkinen]

On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > 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 to tpm2_pcr_read().
> > > 
> > > This check is performed after information about the PCR banks has been
> > > retrieved.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Missing fixes tag.
> 
> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> output sent by the TPM.
> 
> Roberto

Aah, right, of course. Well the patch set is ATM somewhat broken because
this would require a fixes tag that points to a patch insdie the patch
set.

Probably good way to fix the issue is to just merge this with the
earlier commit.

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Mimi Zohar]

On Sun, 2018-11-18 at 09:27 +0200, Jarkko Sakkinen wrote:
> On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
> > On 11/16/2018 4:03 PM, Jarkko Sakkinen wrote:
> > > On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
> > > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> > > > 
> > > > This patch modifies the definition of tpm_pcr_extend() to allow other
> > > > kernel subsystems to pass a digest for each algorithm supported by the TPM.
> > > > All digests are processed by the TPM in one operation.
> > > > 
> > > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> > > > the TPM driver extends the remaining PCR banks with the first digest
> > > > passed as an argument to the function.
> > > 
> > > What is the legit use case for this?
> > 
> > A subset could be chosen for better performance, or when a TPM algorithm
> > is not supported by the crypto subsystem.
> 
> Doesn't extending a subset a security concern?

Right, so instead of extending a subset of the allocated banks, all of
the allocated banks need to be extended, even for those banks that a
digest was not included.  This is no different than what is being done
today.  IMA is currently only calculating the SHA1 hash, padding the
digest with 0's, and extending the padded value(s) into all of the
allocated banks.

If there is a vulnerability with the hash algorithm, then any bank
extended with the padded/truncated digest would be susceptible.

IMA will need to become TPM 2.0 aware, calculating and extending
multiple banks and define a new measurement list format containing the
multiple digests.

Mimi

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

[Roberto Sassu]

On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>> 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 to tpm2_pcr_read().
>>>>
>>>> This check is performed after information about the PCR banks has been
>>>> retrieved.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> Cc: stable@vger.kernel.org
>>>
>>> Missing fixes tag.
>>
>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>> output sent by the TPM.
>>
>> Roberto
> 
> Aah, right, of course. Well the patch set is ATM somewhat broken because
> this would require a fixes tag that points to a patch insdie the patch
> set.
> 
> Probably good way to fix the issue is to just merge this with the
> earlier commit.

Unfortunately, it is not possible. The exact digest size has been
introduced with patch 5/7.

Roberto


> /Jarkko
> 

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

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Roberto Sassu]

On 11/19/2018 5:57 AM, Mimi Zohar wrote:
> On Sun, 2018-11-18 at 09:27 +0200, Jarkko Sakkinen wrote:
>> On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
>>> On 11/16/2018 4:03 PM, Jarkko Sakkinen wrote:
>>>> On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
>>>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>>>>
>>>>> This patch modifies the definition of tpm_pcr_extend() to allow other
>>>>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>>>>> All digests are processed by the TPM in one operation.
>>>>>
>>>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>>>>> the TPM driver extends the remaining PCR banks with the first digest
>>>>> passed as an argument to the function.
>>>>
>>>> What is the legit use case for this?
>>>
>>> A subset could be chosen for better performance, or when a TPM algorithm
>>> is not supported by the crypto subsystem.
>>
>> Doesn't extending a subset a security concern?
> 
> Right, so instead of extending a subset of the allocated banks, all of
> the allocated banks need to be extended, even for those banks that a
> digest was not included.  This is no different than what is being done
> today.  IMA is currently only calculating the SHA1 hash, padding the
> digest with 0's, and extending the padded value(s) into all of the
> allocated banks.

The caller of tpm_pcr_extend() could pass a subset of the allocated
banks, but the TPM driver extends all banks as before.

Roberto


> If there is a vulnerability with the hash algorithm, then any bank
> extended with the padded/truncated digest would be susceptible.
> 
> IMA will need to become TPM 2.0 aware, calculating and extending
> multiple banks and define a new measurement list format containing the
> multiple digests.
> 
> Mimi
> 

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

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Roberto Sassu]

On 11/18/2018 8:29 AM, Jarkko Sakkinen wrote:
> On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
>> I understood from a previous email that you want to include all API
>> changes for crypto agility in the same patch set.
> 
> Hmm.. maybe there is some misunderstading. Can you point me to the
> comment? Thanks.

---
On Thu, Nov 08, 2018 at 03:24:46PM +0100, Roberto Sassu wrote:
 > Should I include the patch for tpm_pcr_extend() in this patch set, even
 > if the set fixes the digest size issue?

Just add some note to the cover letter. Makes sense here to have a
prequel patch for that because otherwise the implementation will be
a bit messy and half-baked.
---

Roberto


> /Jarkko
> 

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

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Mimi Zohar]

On Mon, 2018-11-19 at 09:16 +0100, Roberto Sassu wrote:
> On 11/19/2018 5:57 AM, Mimi Zohar wrote:
> > On Sun, 2018-11-18 at 09:27 +0200, Jarkko Sakkinen wrote:
> >> On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
> >>> On 11/16/2018 4:03 PM, Jarkko Sakkinen wrote:
> >>>> On Wed, Nov 14, 2018 at 04:31:08PM +0100, Roberto Sassu wrote:
> >>>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> >>>>>
> >>>>> This patch modifies the definition of tpm_pcr_extend() to allow other
> >>>>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
> >>>>> All digests are processed by the TPM in one operation.
> >>>>>
> >>>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> >>>>> the TPM driver extends the remaining PCR banks with the first digest
> >>>>> passed as an argument to the function.
> >>>>
> >>>> What is the legit use case for this?
> >>>
> >>> A subset could be chosen for better performance, or when a TPM algorithm
> >>> is not supported by the crypto subsystem.
> >>
> >> Doesn't extending a subset a security concern?
> > 
> > Right, so instead of extending a subset of the allocated banks, all of
> > the allocated banks need to be extended, even for those banks that a
> > digest was not included.  This is no different than what is being done
> > today.  IMA is currently only calculating the SHA1 hash, padding the
> > digest with 0's, and extending the padded value(s) into all of the
> > allocated banks.
> 
> The caller of tpm_pcr_extend() could pass a subset of the allocated
> banks, but the TPM driver extends all banks as before.

Agreed, there should be a clear division.

1) The caller shouldn't need to know anything about the chip->info.
2) The TPM driver should not rely on the caller to supply all the
hashes, but verify that all allocated banks are being extended.

Mimi

> 
> 
> > If there is a vulnerability with the hash algorithm, then any bank
> > extended with the padded/truncated digest would be susceptible.
> > 
> > IMA will need to become TPM 2.0 aware, calculating and extending
> > multiple banks and define a new measurement list format containing the
> > multiple digests.
> > 
> > Mimi
> > 
> 

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

[Jarkko Sakkinen]

On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > 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 to tpm2_pcr_read().
> > > > > 
> > > > > This check is performed after information about the PCR banks has been
> > > > > retrieved.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > Cc: stable@vger.kernel.org
> > > > 
> > > > Missing fixes tag.
> > > 
> > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > output sent by the TPM.
> > > 
> > > Roberto
> > 
> > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > this would require a fixes tag that points to a patch insdie the patch
> > set.
> > 
> > Probably good way to fix the issue is to just merge this with the
> > earlier commit.
> 
> Unfortunately, it is not possible. The exact digest size has been
> introduced with patch 5/7.

I put this in simple terms: if I merge 5/7 the driver will be broken.
Any commit in the patch set should not leave the tree in broken state.

That implies that 5/7 and 6/7 cannot be separate commits.

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Jarkko Sakkinen]

On Mon, Nov 19, 2018 at 09:16:45AM +0100, Roberto Sassu wrote:
> The caller of tpm_pcr_extend() could pass a subset of the allocated
> banks, but the TPM driver extends all banks as before.

Ah, of course. Then it should be legit I guess...

/Jarkko

Re: [PATCH v5 7/7] tpm: pass an array of tpm_bank_list structures to tpm_pcr_extend()

[Jarkko Sakkinen]

On Mon, Nov 19, 2018 at 09:22:32AM +0100, Roberto Sassu wrote:
> On 11/18/2018 8:29 AM, Jarkko Sakkinen wrote:
> > On Fri, Nov 16, 2018 at 04:55:36PM +0100, Roberto Sassu wrote:
> > > I understood from a previous email that you want to include all API
> > > changes for crypto agility in the same patch set.
> > 
> > Hmm.. maybe there is some misunderstading. Can you point me to the
> > comment? Thanks.
> 
> ---
> On Thu, Nov 08, 2018 at 03:24:46PM +0100, Roberto Sassu wrote:
> > Should I include the patch for tpm_pcr_extend() in this patch set, even
> > if the set fixes the digest size issue?
> 
> Just add some note to the cover letter. Makes sense here to have a
> prequel patch for that because otherwise the implementation will be
> a bit messy and half-baked.

Ok, I guess it is OK as long as IMA (namely Mimi) gives that patch
reviewed-by. It is not a huge change.

PS. I noticed that earlier patches have postfixes like '_struct' and
'_ptr' for function arguments. Can you remove them in the next version?

/Jarkko

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

[Roberto Sassu]

On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
>> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
>>> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>>>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>>>> 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 to tpm2_pcr_read().
>>>>>>
>>>>>> This check is performed after information about the PCR banks has been
>>>>>> retrieved.
>>>>>>
>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>> Cc: stable@vger.kernel.org
>>>>>
>>>>> Missing fixes tag.
>>>>
>>>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>>>> output sent by the TPM.
>>>>
>>>> Roberto
>>>
>>> Aah, right, of course. Well the patch set is ATM somewhat broken because
>>> this would require a fixes tag that points to a patch insdie the patch
>>> set.
>>>
>>> Probably good way to fix the issue is to just merge this with the
>>> earlier commit.
>>
>> Unfortunately, it is not possible. The exact digest size has been
>> introduced with patch 5/7.
> 
> I put this in simple terms: if I merge 5/7 the driver will be broken.
> Any commit in the patch set should not leave the tree in broken state.

Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
the PCR read performed by patch 5/7, because the digest sizes are not
yet available.

Roberto


> That implies that 5/7 and 6/7 cannot be separate commits.
> 
> /Jarkko
> 

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

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

[Jarkko Sakkinen]

On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
> On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> > > On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > > > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > > > 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 to tpm2_pcr_read().
> > > > > > > 
> > > > > > > This check is performed after information about the PCR banks has been
> > > > > > > retrieved.
> > > > > > > 
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > Cc: stable@vger.kernel.org
> > > > > > 
> > > > > > Missing fixes tag.
> > > > > 
> > > > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > > > output sent by the TPM.
> > > > > 
> > > > > Roberto
> > > > 
> > > > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > > > this would require a fixes tag that points to a patch insdie the patch
> > > > set.
> > > > 
> > > > Probably good way to fix the issue is to just merge this with the
> > > > earlier commit.
> > > 
> > > Unfortunately, it is not possible. The exact digest size has been
> > > introduced with patch 5/7.
> > 
> > I put this in simple terms: if I merge 5/7 the driver will be broken.
> > Any commit in the patch set should not leave the tree in broken state.
> 
> Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
> the PCR read performed by patch 5/7, because the digest sizes are not
> yet available.

Ah, right. Point taken :-)

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Roberto Sassu]

On 11/16/2018 2:38 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 14, 2018 at 04:31:03PM +0100, Roberto Sassu wrote:
>> 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. Setting the array size to zero or 3 does not make any
>> difference, because the parser has to adjust the offset depending on the
>> actual array size to access structure members after the static arrays.
>>
>> Thus, this patch removes the declaration of TPM2_ACTIVE_PCR_BANKS and sets
>> the array size to zero.
>>
>> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
>> event log")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   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
>>
> 
> NAK for the same reason as last time.

No Fixes tag, or Fixes tag without newline?

Roberto


> /Jarkko
> 

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

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

[Roberto Sassu]

On 11/19/2018 5:07 PM, Jarkko Sakkinen wrote:
> On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
>> On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
>>> On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
>>>> On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
>>>>> On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
>>>>>> On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
>>>>>>> On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
>>>>>>>> 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 to tpm2_pcr_read().
>>>>>>>>
>>>>>>>> This check is performed after information about the PCR banks has been
>>>>>>>> retrieved.
>>>>>>>>
>>>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>
>>>>>>> Missing fixes tag.
>>>>>>
>>>>>> Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
>>>>>> output sent by the TPM.
>>>>>>
>>>>>> Roberto
>>>>>
>>>>> Aah, right, of course. Well the patch set is ATM somewhat broken because
>>>>> this would require a fixes tag that points to a patch insdie the patch
>>>>> set.
>>>>>
>>>>> Probably good way to fix the issue is to just merge this with the
>>>>> earlier commit.
>>>>
>>>> Unfortunately, it is not possible. The exact digest size has been
>>>> introduced with patch 5/7.
>>>
>>> I put this in simple terms: if I merge 5/7 the driver will be broken.
>>> Any commit in the patch set should not leave the tree in broken state.
>>
>> Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
>> the PCR read performed by patch 5/7, because the digest sizes are not
>> yet available.
> 
> Ah, right. Point taken :-)

Should I keep CC: stable@vger.kernel.org?

Roberto


> /Jarkko
> 

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

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Roberto Sassu]

On 11/16/2018 2:38 PM, Jarkko Sakkinen wrote:
> On Wed, Nov 14, 2018 at 04:31:03PM +0100, Roberto Sassu wrote:
>> 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. Setting the array size to zero or 3 does not make any
>> difference, because the parser has to adjust the offset depending on the
>> actual array size to access structure members after the static arrays.
>>
>> Thus, this patch removes the declaration of TPM2_ACTIVE_PCR_BANKS and sets
>> the array size to zero.
>>
>> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
>> event log")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   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
>>
> 
> NAK for the same reason as last time.

I added this comment to include/linux/tpm_eventlog.h:

/*
  * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
  *
  * Set the size of 'digest_sizes' and 'digests', members of 
tcg_efi_specid_event
  * and tcg_pcr_event2, to zero. Structures with variable-sized arrays 
placed
  * midway are not suitable for type casting.
  */

Is it ok?

Thanks

Roberto


> /Jarkko
> 

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

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

[Jarkko Sakkinen]

On Wed, Nov 28, 2018 at 09:40:37AM +0100, Roberto Sassu wrote:
> On 11/19/2018 5:07 PM, Jarkko Sakkinen wrote:
> > On Mon, Nov 19, 2018 at 04:09:34PM +0100, Roberto Sassu wrote:
> > > On 11/19/2018 3:33 PM, Jarkko Sakkinen wrote:
> > > > On Mon, Nov 19, 2018 at 09:14:00AM +0100, Roberto Sassu wrote:
> > > > > On 11/18/2018 8:32 AM, Jarkko Sakkinen wrote:
> > > > > > On Fri, Nov 16, 2018 at 05:06:48PM +0100, Roberto Sassu wrote:
> > > > > > > On 11/16/2018 2:41 PM, Jarkko Sakkinen wrote:
> > > > > > > > On Wed, Nov 14, 2018 at 04:31:07PM +0100, Roberto Sassu wrote:
> > > > > > > > > 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 to tpm2_pcr_read().
> > > > > > > > > 
> > > > > > > > > This check is performed after information about the PCR banks has been
> > > > > > > > > retrieved.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > > > Cc: stable@vger.kernel.org
> > > > > > > > 
> > > > > > > > Missing fixes tag.
> > > > > > > 
> > > > > > > Before this patch set, tpm2_pcr_extend() always copied 20 bytes from the
> > > > > > > output sent by the TPM.
> > > > > > > 
> > > > > > > Roberto
> > > > > > 
> > > > > > Aah, right, of course. Well the patch set is ATM somewhat broken because
> > > > > > this would require a fixes tag that points to a patch insdie the patch
> > > > > > set.
> > > > > > 
> > > > > > Probably good way to fix the issue is to just merge this with the
> > > > > > earlier commit.
> > > > > 
> > > > > Unfortunately, it is not possible. The exact digest size has been
> > > > > introduced with patch 5/7.
> > > > 
> > > > I put this in simple terms: if I merge 5/7 the driver will be broken.
> > > > Any commit in the patch set should not leave the tree in broken state.
> > > 
> > > Patch 6/7 does not fix patch 5/7. The check in patch 6/7 is not done for
> > > the PCR read performed by patch 5/7, because the digest sizes are not
> > > yet available.
> > 
> > Ah, right. Point taken :-)
> 
> Should I keep CC: stable@vger.kernel.org?

Nope, my bad, sorry.

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Roberto Sassu]

On 11/28/2018 1:17 PM, Roberto Sassu wrote:
> On 11/16/2018 2:38 PM, Jarkko Sakkinen wrote:
>> On Wed, Nov 14, 2018 at 04:31:03PM +0100, Roberto Sassu wrote:
>>> 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. Setting the array size to zero or 3 does not make any
>>> difference, because the parser has to adjust the offset depending on the
>>> actual array size to access structure members after the static arrays.
>>>
>>> Thus, this patch removes the declaration of TPM2_ACTIVE_PCR_BANKS and 
>>> sets
>>> the array size to zero.
>>>
>>> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
>>> event log")
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>>   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
>>>
>>
>> NAK for the same reason as last time.
> 
> I added this comment to include/linux/tpm_eventlog.h:
> 
> /*
>   * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
>   *
>   * Set the size of 'digest_sizes' and 'digests', members of 
> tcg_efi_specid_event
>   * and tcg_pcr_event2, to zero. Structures with variable-sized arrays 
> placed
>   * midway are not suitable for type casting.
>   */
> 

If this comment is ok, I will send a new version of the patch set.

Roberto


>> /Jarkko
>>
> 

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

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Jarkko Sakkinen]

On Thu, Nov 29, 2018 at 01:04:40PM +0100, Roberto Sassu wrote:
> > > > +    struct tpm2_digest digests[0];
> > > >       struct tcg_event_field event;
> > > >   } __packed;
> > > > -- 
> > > > 2.17.1
> > > > 
> > > 
> > > NAK for the same reason as last time.
> > 
> > I added this comment to include/linux/tpm_eventlog.h:
> > 
> > /*
> >   * http://www.trustedcomputinggroup.org/tcg-efi-protocol-specification/
> >   *
> >   * Set the size of 'digest_sizes' and 'digests', members of
> > tcg_efi_specid_event
> >   * and tcg_pcr_event2, to zero. Structures with variable-sized arrays
> > placed
> >   * midway are not suitable for type casting.
> >   */
> > 
> 
> If this comment is ok, I will send a new version of the patch set.
> 
> Roberto

Even after looking at the spec the last field does not make sense as the
event after digests and digests are not in union. It is just not right.

The comment does not fix that.

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Jarkko Sakkinen]

On Fri, Nov 30, 2018 at 11:41:49AM -0800, Jarkko Sakkinen wrote:
> Even after looking at the spec the last field does not make sense as the
> event after digests and digests are not in union. It is just not right.
> 
> The comment does not fix that.

You should remove the last field and rename the struct as maybe
struct tcg_pcr_event2_head. If we ever need the very last field
we can create struct *_tail.

Not your fault but there is clear bug in the struct definition
that should be fixed in a commit of its own.

No new comments are needed.

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Jarkko Sakkinen]

On Fri, Nov 30, 2018 at 11:45:27AM -0800, Jarkko Sakkinen wrote:
> On Fri, Nov 30, 2018 at 11:41:49AM -0800, Jarkko Sakkinen wrote:
> > Even after looking at the spec the last field does not make sense as the
> > event after digests and digests are not in union. It is just not right.
> > 
> > The comment does not fix that.
> 
> You should remove the last field and rename the struct as maybe
> struct tcg_pcr_event2_head. If we ever need the very last field
> we can create struct *_tail.
> 
> Not your fault but there is clear bug in the struct definition
> that should be fixed in a commit of its own.
> 
> No new comments are needed.

I checked and did not find the field even used anywhere. Can you
peer check this and then just remove it? And yeah maybe the struct
should have _head suffix.

/Jarkko

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Roberto Sassu]

On 11/30/2018 8:45 PM, Jarkko Sakkinen wrote:
> On Fri, Nov 30, 2018 at 11:41:49AM -0800, Jarkko Sakkinen wrote:
>> Even after looking at the spec the last field does not make sense as the
>> event after digests and digests are not in union. It is just not right.
>>
>> The comment does not fix that.
> 
> You should remove the last field and rename the struct as maybe
> struct tcg_pcr_event2_head. If we ever need the very last field
> we can create struct *_tail.
> 
> Not your fault but there is clear bug in the struct definition
> that should be fixed in a commit of its own.

Given that I explain the problem in the new patch, I would also remove
TPM2_ACTIVE_PCR_BANKS in the same patch. Is it ok?

Thanks

Roberto

> No new comments are needed.
> 
> /Jarkko
> 

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

Re: [PATCH v5 2/7] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS

[Jarkko Sakkinen]

On Mon, Dec 03, 2018 at 10:59:10AM +0100, Roberto Sassu wrote:
> On 11/30/2018 8:45 PM, Jarkko Sakkinen wrote:
> > On Fri, Nov 30, 2018 at 11:41:49AM -0800, Jarkko Sakkinen wrote:
> > > Even after looking at the spec the last field does not make sense as the
> > > event after digests and digests are not in union. It is just not right.
> > > 
> > > The comment does not fix that.
> > 
> > You should remove the last field and rename the struct as maybe
> > struct tcg_pcr_event2_head. If we ever need the very last field
> > we can create struct *_tail.
> > 
> > Not your fault but there is clear bug in the struct definition
> > that should be fixed in a commit of its own.
> 
> Given that I explain the problem in the new patch, I would also remove
> TPM2_ACTIVE_PCR_BANKS in the same patch. Is it ok?

Yes.

/Jarkko