Linux-Security-Module Archive on lore.kernel.org
 help / Atom feed
* [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM
@ 2018-11-06 15:01 Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
                   ` (6 more replies)
  0 siblings, 7 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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

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

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

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

Changelog

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

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

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

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

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

-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-07  6:14   ` Nayna Jain
  2018-11-08 13:46   ` Jarkko Sakkinen
  2018-11-06 15:01 ` [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS Roberto Sassu
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

This patch removes the hard-coded limit of the active_banks array size.
It stores in the tpm_chip structure the number of active PCR banks,
determined in tpm2_get_pcr_allocation(), and replaces the static array
with a pointer to a dynamically allocated array.

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

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

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

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 46caadca916a..2a9e8b744436 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
 	kfree(chip->log.bios_event_log);
 	kfree(chip->work_space.context_buf);
 	kfree(chip->work_space.session_buf);
+	kfree(chip->active_banks);
 	kfree(chip);
 }
 
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 1a803b0cf980..ba7ca6b3e664 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
 int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
 	int rc;
-	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
-	u32 count = 0;
+	struct tpm2_digest *digest_list;
 	int i;
 
 	chip = tpm_find_get_ops(chip);
@@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		memset(digest_list, 0, sizeof(digest_list));
+		digest_list = kmalloc_array(chip->nr_active_banks,
+					    sizeof(*digest_list), GFP_KERNEL);
+		if (!digest_list)
+			return -ENOMEM;
 
-		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
-			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
+		memset(digest_list, 0,
+		       chip->nr_active_banks * sizeof(*digest_list));
+
+		for (i = 0; i < chip->nr_active_banks; i++) {
 			digest_list[i].alg_id = chip->active_banks[i];
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-			count++;
 		}
 
-		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
+		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
+				     digest_list);
+		kfree(digest_list);
 		tpm_put_ops(chip);
 		return rc;
 	}
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f3501d05264f..98368c3a6ff7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -248,7 +248,8 @@ struct tpm_chip {
 	const struct attribute_group *groups[3];
 	unsigned int groups_cnt;
 
-	u16 active_banks[7];
+	u32 nr_active_banks;
+	u16 *active_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index c31b490bd41d..533089cede07 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	int i;
 	int j;
 
-	if (count > ARRAY_SIZE(chip->active_banks))
+	if (count > chip->nr_active_banks)
 		return -EINVAL;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
@@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	void *marker;
 	void *end;
 	void *pcr_select_offset;
-	unsigned int count;
 	u32 sizeof_pcr_selection;
 	u32 rsp_len;
 	int rc;
@@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	if (rc)
 		goto out;
 
-	count = be32_to_cpup(
+	chip->nr_active_banks = be32_to_cpup(
 		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
 
-	if (count > ARRAY_SIZE(chip->active_banks)) {
-		rc = -ENODEV;
+	chip->active_banks = kmalloc_array(chip->nr_active_banks,
+					   sizeof(*chip->active_banks),
+					   GFP_KERNEL);
+	if (!chip->active_banks) {
+		rc = -ENOMEM;
 		goto out;
 	}
 
@@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
 	end = &buf.data[rsp_len];
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < chip->nr_active_banks; i++) {
 		pcr_select_offset = marker +
 			offsetof(struct tpm2_pcr_selection, size_of_select);
 		if (pcr_select_offset >= end) {
@@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	}
 
 out:
-	if (i < ARRAY_SIZE(chip->active_banks))
-		chip->active_banks[i] = TPM2_ALG_ERROR;
-
 	tpm_buf_destroy(&buf);
 
 	return rc;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-08 14:02   ` Jarkko Sakkinen
  2018-11-08 19:05   ` Jarkko Sakkinen
  2018-11-06 15:01 ` [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
                   ` (4 subsequent siblings)
  6 siblings, 2 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 and are not suitable for parsing events with type casting.

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

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

Signed-off-by: Roberto Sassu <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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm Roberto Sassu
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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/tpm2-cmd.c      | 18 +++++++++---------
 include/linux/tpm.h              | 18 ++++++++++++++++++
 include/linux/tpm_eventlog.h     |  9 ++-------
 5 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index ba7ca6b3e664..07b62734598c 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -1039,7 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
 int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
 	int rc;
-	struct tpm2_digest *digest_list;
+	struct tpm_digest *digest_list;
 	int i;
 
 	chip = tpm_find_get_ops(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 98368c3a6ff7..fc2cc16e5080 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -122,17 +122,6 @@ enum tpm2_return_codes {
 	TPM2_RC_RETRY		= 0x0922,
 };
 
-enum tpm2_algorithms {
-	TPM2_ALG_ERROR		= 0x0000,
-	TPM2_ALG_SHA1		= 0x0004,
-	TPM2_ALG_KEYEDHASH	= 0x0008,
-	TPM2_ALG_SHA256		= 0x000B,
-	TPM2_ALG_SHA384		= 0x000C,
-	TPM2_ALG_SHA512		= 0x000D,
-	TPM2_ALG_NULL		= 0x0010,
-	TPM2_ALG_SM3_256	= 0x0012,
-};
-
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		= 0x011F,
 	TPM2_CC_CREATE_PRIMARY  = 0x0131,
@@ -578,7 +567,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests);
+		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 			    unsigned int flags);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 533089cede07..584dba6cdf3e 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -33,11 +33,11 @@ struct tpm2_hash {
 };
 
 static struct tpm2_hash tpm2_hash_map[] = {
-	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
-	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
-	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
-	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
-	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
+	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
+	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
+	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
+	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
 };
 
 /*
@@ -200,7 +200,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM2_ALG_SHA1);
+	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
 	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
@@ -234,7 +234,7 @@ struct tpm2_null_auth_area {
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
-		    struct tpm2_digest *digests)
+		    struct tpm_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
@@ -457,7 +457,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 
 	/* public */
 	tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
-	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
+	tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
 	tpm_buf_append_u16(&buf, hash);
 
 	/* policy */
@@ -472,7 +472,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	}
 
 	/* public parameters */
-	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+	tpm_buf_append_u16(&buf, TPM_ALG_NULL);
 	tpm_buf_append_u16(&buf, 0);
 
 	/* outside info */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4609b94142d4..71d7bbf5f690 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,12 +22,30 @@
 #ifndef __LINUX_TPM_H__
 #define __LINUX_TPM_H__
 
+#include <crypto/hash_info.h>
+
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
 
 struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+enum tpm_algorithms {
+	TPM_ALG_ERROR		= 0x0000,
+	TPM_ALG_SHA1		= 0x0004,
+	TPM_ALG_KEYEDHASH	= 0x0008,
+	TPM_ALG_SHA256		= 0x000B,
+	TPM_ALG_SHA384		= 0x000C,
+	TPM_ALG_SHA512		= 0x000D,
+	TPM_ALG_NULL		= 0x0010,
+	TPM_ALG_SM3_256		= 0x0012,
+};
+
+struct tpm_digest {
+	u16 alg_id;
+	u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 3d5d162f09cc..c9f28b4be4ae 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -3,7 +3,7 @@
 #ifndef __LINUX_TPM_EVENTLOG_H__
 #define __LINUX_TPM_EVENTLOG_H__
 
-#include <crypto/hash_info.h>
+#include <linux/tpm.h>
 
 #define TCG_EVENT_NAME_LEN_MAX	255
 #define MAX_TEXT_EVENT		1000	/* Max event string length */
@@ -107,16 +107,11 @@ struct tcg_event_field {
 	u8 event[0];
 } __packed;
 
-struct tpm2_digest {
-	u16 alg_id;
-	u8 digest[SHA512_DIGEST_SIZE];
-} __packed;
-
 struct tcg_pcr_event2 {
 	u32 pcr_idx;
 	u32 event_type;
 	u32 count;
-	struct tpm2_digest digests[0];
+	struct tpm_digest digests[0];
 	struct tcg_event_field event;
 } __packed;
 
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (2 preceding siblings ...)
  2018-11-06 15:01 ` [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-08 14:04   ` Jarkko Sakkinen
  2018-11-06 15:01 ` [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 07b62734598c..e341ed9c232a 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -976,11 +976,12 @@ EXPORT_SYMBOL_GPL(tpm_is_tpm2);
  * tpm_pcr_read - read a PCR value from SHA1 bank
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
  * @pcr_idx:	the PCR to be retrieved
- * @res_buf:	the value of the PCR
+ * @digest_struct:	pcr bank and buffer current PCR value is written to
  *
  * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		 struct tpm_digest *digest_struct)
 {
 	int rc;
 
@@ -988,9 +989,9 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	if (!chip)
 		return -ENODEV;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
+		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
 	else
-		rc = tpm_pcr_read_dev(chip, pcr_idx, res_buf);
+		rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
 	tpm_put_ops(chip);
 	return rc;
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index fc2cc16e5080..2fd4379e75d6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -565,7 +565,8 @@ static inline u32 tpm2_rc_value(u32 rc)
 	return (rc & BIT(7)) ? rc & 0xff : rc;
 }
 
-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		  struct tpm_digest *digest_struct);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 584dba6cdf3e..499f4f17b3f3 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -179,16 +179,18 @@ struct tpm2_pcr_read_out {
  * tpm2_pcr_read() - read a PCR value
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR to read.
- * @res_buf:	buffer to store the resulting hash.
+ * @digest_struct:	pcr bank and buffer current PCR value is written to.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
-int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
+		  struct tpm_digest *digest_struct)
 {
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
+	u16 digest_size;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
@@ -200,18 +202,25 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
+	tpm_buf_append_u16(&buf, digest_struct->alg_id);
 	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
-			res_buf ? "attempting to read a pcr value" : NULL);
-	if (rc == 0 && res_buf) {
-		out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
-		memcpy(res_buf, out->digest, SHA1_DIGEST_SIZE);
+			      "attempting to read a pcr value");
+	if (rc)
+		goto out;
+
+	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
+	digest_size = be16_to_cpu(out->digest_size);
+	if (digest_size > sizeof(digest_struct->digest)) {
+		rc = -EINVAL;
+		goto out;
 	}
 
+	memcpy(digest_struct->digest, out->digest, digest_size);
+out:
 	tpm_buf_destroy(&buf);
 	return rc;
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 71d7bbf5f690..4f00daf44dd2 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -71,7 +71,8 @@ struct tpm_class_ops {
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(struct tpm_chip *chip);
-extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
+extern int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+			struct tpm_digest *digest_struct);
 extern int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
@@ -87,7 +88,8 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
 {
 	return -ENODEV;
 }
-static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
+static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+			       struct tpm_digest *digest_struct)
 {
 	return -ENODEV;
 }
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 7e7e7e7c250a..8985e34eb3ac 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -629,12 +629,12 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
 	return calc_buffer_shash(buf, len, hash);
 }
 
-static void __init ima_pcrread(int idx, u8 *pcr)
+static void __init ima_pcrread(int idx, struct tpm_digest *d)
 {
 	if (!ima_tpm_chip)
 		return;
 
-	if (tpm_pcr_read(ima_tpm_chip, idx, pcr) != 0)
+	if (tpm_pcr_read(ima_tpm_chip, idx, d) != 0)
 		pr_err("Error Communicating to TPM chip\n");
 }
 
@@ -644,7 +644,7 @@ static void __init ima_pcrread(int idx, u8 *pcr)
 static int __init ima_calc_boot_aggregate_tfm(char *digest,
 					      struct crypto_shash *tfm)
 {
-	u8 pcr_i[TPM_DIGEST_SIZE];
+	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
 	int rc, i;
 	SHASH_DESC_ON_STACK(shash, tfm);
 
@@ -657,9 +657,9 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 
 	/* cumulative sha1 over tpm registers 0-7 */
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
-		ima_pcrread(i, pcr_i);
+		ima_pcrread(i, &d);
 		/* now accumulate with current aggregate */
-		rc = crypto_shash_update(shash, pcr_i, TPM_DIGEST_SIZE);
+		rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (3 preceding siblings ...)
  2018-11-06 15:01 ` [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-06 15:01 ` [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size Roberto Sassu
  2018-11-08 13:51 ` [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Jarkko Sakkinen
  6 siblings, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 |  8 ++++--
 drivers/char/tpm/tpm.h           |  4 +--
 drivers/char/tpm/tpm2-cmd.c      | 47 ++++++++++++++++++++++++--------
 include/linux/tpm.h              |  6 ++++
 4 files changed, 50 insertions(+), 15 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e341ed9c232a..c864b1645856 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -989,7 +989,7 @@ int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
 	if (!chip)
 		return -ENODEV;
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct);
+		rc = tpm2_pcr_read(chip, pcr_idx, digest_struct, NULL);
 	else
 		rc = tpm_pcr_read_dev(chip, pcr_idx, digest_struct->digest);
 	tpm_put_ops(chip);
@@ -1057,7 +1057,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 		       chip->nr_active_banks * sizeof(*digest_list));
 
 		for (i = 0; i < chip->nr_active_banks; i++) {
-			digest_list[i].alg_id = chip->active_banks[i];
+			digest_list[i].alg_id = chip->active_banks[i].alg_id;
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 		}
 
@@ -1159,6 +1159,10 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
+	chip->active_banks[0].alg_id = TPM_ALG_SHA1;
+	chip->active_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+	chip->active_banks[0].crypto_id = HASH_ALGO_SHA1;
+
 	return rc;
 out:
 	if (rc > 0)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 2fd4379e75d6..dfa54fc6c730 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -238,7 +238,7 @@ struct tpm_chip {
 	unsigned int groups_cnt;
 
 	u32 nr_active_banks;
-	u16 *active_banks;
+	struct tpm_bank_info *active_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -566,7 +566,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 }
 
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
-		  struct tpm_digest *digest_struct);
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 499f4f17b3f3..e2d5b84286a7 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -180,11 +180,12 @@ struct tpm2_pcr_read_out {
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR to read.
  * @digest_struct:	pcr bank and buffer current PCR value is written to.
+ * @digest_size_ptr:	pointer to variable that stores the digest size.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
-		  struct tpm_digest *digest_struct)
+		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
 	int rc;
 	struct tpm_buf buf;
@@ -219,6 +220,9 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 		goto out;
 	}
 
+	if (digest_size_ptr)
+		*digest_size_ptr = digest_size;
+
 	memcpy(digest_struct->digest, out->digest, digest_size);
 out:
 	tpm_buf_destroy(&buf);
@@ -249,7 +253,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > chip->nr_active_banks)
 		return -EINVAL;
@@ -271,14 +274,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, count);
 
 	for (i = 0; i < count; i++) {
-		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
-			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
-				continue;
-			tpm_buf_append_u16(&buf, digests[i].alg_id);
-			tpm_buf_append(&buf, (const unsigned char
-					      *)&digests[i].digest,
-			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
-		}
+		tpm_buf_append_u16(&buf, digests[i].alg_id);
+		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+			       chip->active_banks[i].digest_size);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -855,6 +853,26 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_init_bank_info(struct tpm_chip *chip,
+			       struct tpm_bank_info *bank)
+{
+	struct tpm_digest digest = { .alg_id = bank->alg_id };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		enum hash_algo crypto_algo = tpm2_hash_map[i].crypto_id;
+
+		if (bank->alg_id != tpm2_hash_map[i].tpm_id)
+			continue;
+
+		bank->digest_size = hash_digest_size[crypto_algo];
+		bank->crypto_id = crypto_algo;
+		return 0;
+	}
+
+	return tpm2_pcr_read(chip, 0, &digest, &bank->digest_size);
+}
+
 struct tpm2_pcr_selection {
 	__be16  hash_alg;
 	u8  size_of_select;
@@ -870,6 +888,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 	void *pcr_select_offset;
 	u32 sizeof_pcr_selection;
 	u32 rsp_len;
+	u16 alg_id;
 	int rc;
 	int i = 0;
 
@@ -911,7 +930,13 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 		}
 
 		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
-		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
+		alg_id = be16_to_cpu(pcr_selection.hash_alg);
+		chip->active_banks[i].alg_id = alg_id;
+
+		rc = tpm2_init_bank_info(chip, &chip->active_banks[i]);
+		if (rc)
+			break;
+
 		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
 			sizeof(pcr_selection.size_of_select) +
 			pcr_selection.size_of_select;
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 4f00daf44dd2..3f91124837cf 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -46,6 +46,12 @@ struct tpm_digest {
 	u8 digest[SHA512_DIGEST_SIZE];
 } __packed;
 
+struct tpm_bank_info {
+	u16 alg_id;
+	u16 digest_size;
+	u16 crypto_id;
+};
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (4 preceding siblings ...)
  2018-11-06 15:01 ` [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
@ 2018-11-06 15:01 ` Roberto Sassu
  2018-11-08 14:08   ` Jarkko Sakkinen
  2018-11-08 13:51 ` [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Jarkko Sakkinen
  6 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-06 15:01 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, 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 as argument 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>
---
 drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index e2d5b84286a7..3b0b5b032901 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
 {
+	int i;
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
 	u16 digest_size;
+	u16 expected_digest_size = 0;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
+	if (!digest_size_ptr) {
+		for (i = 0; i < chip->nr_active_banks &&
+		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
+			;
+
+		if (i == chip->nr_active_banks)
+			return -EINVAL;
+
+		expected_digest_size = chip->active_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
 
 	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
 	digest_size = be16_to_cpu(out->digest_size);
-	if (digest_size > sizeof(digest_struct->digest)) {
+	if (digest_size > sizeof(digest_struct->digest) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
 		rc = -EINVAL;
 		goto out;
 	}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
@ 2018-11-07  6:14   ` Nayna Jain
  2018-11-07  9:41     ` Roberto Sassu
  2018-11-07 11:10     ` Mimi Zohar
  2018-11-08 13:46   ` Jarkko Sakkinen
  1 sibling, 2 replies; 41+ messages in thread
From: Nayna Jain @ 2018-11-07  6:14 UTC (permalink / raw)
  To: Roberto Sassu, jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel, silviu.vlasceanu



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


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

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


> It stores in the tpm_chip structure the number of active PCR banks,
> determined in tpm2_get_pcr_allocation(), and replaces the static array
> with a pointer to a dynamically allocated array.
>
> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> does not check anymore if the algorithm stored in tpm_chip is equal to
> zero. The active_banks array always contains valid algorithms.
>
> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks")
>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>   drivers/char/tpm/tpm-chip.c      |  1 +
>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>   drivers/char/tpm/tpm.h           |  3 ++-
>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>   4 files changed, 23 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 46caadca916a..2a9e8b744436 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>   	kfree(chip->log.bios_event_log);
>   	kfree(chip->work_space.context_buf);
>   	kfree(chip->work_space.session_buf);
> +	kfree(chip->active_banks);
>   	kfree(chip);
>   }
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..ba7ca6b3e664 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>   {
>   	int rc;
> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> -	u32 count = 0;
> +	struct tpm2_digest *digest_list;
>   	int i;
>
>   	chip = tpm_find_get_ops(chip);
> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>   		return -ENODEV;
>
>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		memset(digest_list, 0, sizeof(digest_list));
> +		digest_list = kmalloc_array(chip->nr_active_banks,
> +					    sizeof(*digest_list), GFP_KERNEL);
> +		if (!digest_list)
> +			return -ENOMEM;
>
> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		memset(digest_list, 0,
> +		       chip->nr_active_banks * sizeof(*digest_list));
> +
> +		for (i = 0; i < chip->nr_active_banks; i++) {
>   			digest_list[i].alg_id = chip->active_banks[i];
>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
>   		}
>
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
> +				     digest_list);
> +		kfree(digest_list);
>   		tpm_put_ops(chip);
>   		return rc;
>   	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f3501d05264f..98368c3a6ff7 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -248,7 +248,8 @@ struct tpm_chip {
>   	const struct attribute_group *groups[3];
>   	unsigned int groups_cnt;
>
> -	u16 active_banks[7];
> +	u32 nr_active_banks;
> +	u16 *active_banks;
>   #ifdef CONFIG_ACPI
>   	acpi_handle acpi_dev_handle;
>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..533089cede07 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>   	int i;
>   	int j;
>
> -	if (count > ARRAY_SIZE(chip->active_banks))
> +	if (count > chip->nr_active_banks)
>   		return -EINVAL;
>
>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	void *marker;
>   	void *end;
>   	void *pcr_select_offset;
> -	unsigned int count;
>   	u32 sizeof_pcr_selection;
>   	u32 rsp_len;
>   	int rc;
> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	if (rc)
>   		goto out;
>
> -	count = be32_to_cpup(
> +	chip->nr_active_banks = be32_to_cpup(
>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);


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

Thanks & Regards,
     - Nayna


>
> -	if (count > ARRAY_SIZE(chip->active_banks)) {
> -		rc = -ENODEV;
> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
> +					   sizeof(*chip->active_banks),
> +					   GFP_KERNEL);
> +	if (!chip->active_banks) {
> +		rc = -ENOMEM;
>   		goto out;
>   	}
>
> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>   	end = &buf.data[rsp_len];
>
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < chip->nr_active_banks; i++) {
>   		pcr_select_offset = marker +
>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>   		if (pcr_select_offset >= end) {
> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>   	}
>
>   out:
> -	if (i < ARRAY_SIZE(chip->active_banks))
> -		chip->active_banks[i] = TPM2_ALG_ERROR;
> -
>   	tpm_buf_destroy(&buf);
>
>   	return rc;


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-07  6:14   ` Nayna Jain
@ 2018-11-07  9:41     ` Roberto Sassu
  2018-11-08 13:50       ` Nayna Jain
  2018-12-13 20:21       ` Ken Goldman
  2018-11-07 11:10     ` Mimi Zohar
  1 sibling, 2 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-07  9:41 UTC (permalink / raw)
  To: Nayna Jain, jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel, silviu.vlasceanu

On 11/7/2018 7:14 AM, Nayna Jain wrote:
> 
> 
> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
> 
> 
> The hard-coded limit in static array active_banks[] represents the 
> maximum possible banks.
> A TPM might have three banks, but only one bank may be active.
> 
> To confirm my understanding, is the idea for this patch is to 
> dynamically identify the number of possible banks or the number of 
> active banks ?

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

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

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

You mentioned:

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

but TPML_PCR_SELECTION is provided by the TPM.

Roberto


>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>       kfree(chip->log.bios_event_log);
>>       kfree(chip->work_space.context_buf);
>>       kfree(chip->work_space.session_buf);
>> +    kfree(chip->active_banks);
>>       kfree(chip);
>>   }
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c 
>> b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip 
>> *chip, int pcr_idx, const u8 *hash,
>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   {
>>       int rc;
>> -    struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -    u32 count = 0;
>> +    struct tpm2_digest *digest_list;
>>       int i;
>>
>>       chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int 
>> pcr_idx, const u8 *hash)
>>           return -ENODEV;
>>
>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -        memset(digest_list, 0, sizeof(digest_list));
>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>> +                        sizeof(*digest_list), GFP_KERNEL);
>> +        if (!digest_list)
>> +            return -ENOMEM;
>>
>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +        memset(digest_list, 0,
>> +               chip->nr_active_banks * sizeof(*digest_list));
>> +
>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>               digest_list[i].alg_id = chip->active_banks[i];
>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -            count++;
>>           }
>>
>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +                     digest_list);
>> +        kfree(digest_list);
>>           tpm_put_ops(chip);
>>           return rc;
>>       }
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>       const struct attribute_group *groups[3];
>>       unsigned int groups_cnt;
>>
>> -    u16 active_banks[7];
>> +    u32 nr_active_banks;
>> +    u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>       acpi_handle acpi_dev_handle;
>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>> pcr_idx, u32 count,
>>       int i;
>>       int j;
>>
>> -    if (count > ARRAY_SIZE(chip->active_banks))
>> +    if (count > chip->nr_active_banks)
>>           return -EINVAL;
>>
>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       void *marker;
>>       void *end;
>>       void *pcr_select_offset;
>> -    unsigned int count;
>>       u32 sizeof_pcr_selection;
>>       u32 rsp_len;
>>       int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       if (rc)
>>           goto out;
>>
>> -    count = be32_to_cpup(
>> +    chip->nr_active_banks = be32_to_cpup(
>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> 
> 
> As per my understanding, the count in the TPML_PCR_SELECTION represent 
> the number of possible banks and not the number of active banks.
> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation 
> of #TPM_RC_SIZE.
> 
> Thanks & Regards,
>      - Nayna
> 
> 
>>
>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>> -        rc = -ENODEV;
>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +                       sizeof(*chip->active_banks),
>> +                       GFP_KERNEL);
>> +    if (!chip->active_banks) {
>> +        rc = -ENOMEM;
>>           goto out;
>>       }
>>
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>       end = &buf.data[rsp_len];
>>
>> -    for (i = 0; i < count; i++) {
>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>           pcr_select_offset = marker +
>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>           if (pcr_select_offset >= end) {
>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>> tpm_chip *chip)
>>       }
>>
>>   out:
>> -    if (i < ARRAY_SIZE(chip->active_banks))
>> -        chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>       tpm_buf_destroy(&buf);
>>
>>       return rc;
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-07  6:14   ` Nayna Jain
  2018-11-07  9:41     ` Roberto Sassu
@ 2018-11-07 11:10     ` Mimi Zohar
  1 sibling, 0 replies; 41+ messages in thread
From: Mimi Zohar @ 2018-11-07 11:10 UTC (permalink / raw)
  To: Nayna Jain, Roberto Sassu, jarkko.sakkinen
  Cc: linux-integrity, linux-security-module, linux-kernel, silviu.vlasceanu

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


> > @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
> >   	if (rc)
> >   		goto out;
> >
> > -	count = be32_to_cpup(
> > +	chip->nr_active_banks = be32_to_cpup(
> >   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
> 
> 
> As per my understanding, the count in the TPML_PCR_SELECTION represent 
> the number of possible banks and not the number of active banks.
> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as explanation 
> of #TPM_RC_SIZE.

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


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

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

Mimi

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


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
  2018-11-07  6:14   ` Nayna Jain
@ 2018-11-08 13:46   ` Jarkko Sakkinen
  2018-11-08 14:24     ` Roberto Sassu
                       ` (2 more replies)
  1 sibling, 3 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 13:46 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
> This patch removes the hard-coded limit of the active_banks array size.
> It stores in the tpm_chip structure the number of active PCR banks,
> determined in tpm2_get_pcr_allocation(), and replaces the static array
> with a pointer to a dynamically allocated array.
> 
> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> does not check anymore if the algorithm stored in tpm_chip is equal to
> zero. The active_banks array always contains valid algorithms.
> 
> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> PCR banks")
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm-chip.c      |  1 +
>  drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>  drivers/char/tpm/tpm.h           |  3 ++-
>  drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>  4 files changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 46caadca916a..2a9e8b744436 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>  	kfree(chip->log.bios_event_log);
>  	kfree(chip->work_space.context_buf);
>  	kfree(chip->work_space.session_buf);
> +	kfree(chip->active_banks);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 1a803b0cf980..ba7ca6b3e664 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>  int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>  {
>  	int rc;
> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> -	u32 count = 0;
> +	struct tpm2_digest *digest_list;
>  	int i;
>  
>  	chip = tpm_find_get_ops(chip);
> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>  		return -ENODEV;

Should take digest_list as input. Probably callers could re-use the
same digest_list array multiple times?

Move struct tpm_chip to include/linux/tpm.h so that the caller can query
nr_active_banks and active_banks and can create the digest array by
itself. Lets do this right at once now that this is being restructured.

>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		memset(digest_list, 0, sizeof(digest_list));
> +		digest_list = kmalloc_array(chip->nr_active_banks,
> +					    sizeof(*digest_list), GFP_KERNEL);
> +		if (!digest_list)
> +			return -ENOMEM;
>  
> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> +		memset(digest_list, 0,
> +		       chip->nr_active_banks * sizeof(*digest_list));

You should use kcalloc() to allocate digest_list.

> +
> +		for (i = 0; i < chip->nr_active_banks; i++) {
>  			digest_list[i].alg_id = chip->active_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
>  		}
>  
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
> +				     digest_list);
> +		kfree(digest_list);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f3501d05264f..98368c3a6ff7 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -248,7 +248,8 @@ struct tpm_chip {
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
>  
> -	u16 active_banks[7];
> +	u32 nr_active_banks;
> +	u16 *active_banks;
>  #ifdef CONFIG_ACPI
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index c31b490bd41d..533089cede07 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>  	int i;
>  	int j;
>  
> -	if (count > ARRAY_SIZE(chip->active_banks))
> +	if (count > chip->nr_active_banks)
>  		return -EINVAL;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	void *marker;
>  	void *end;
>  	void *pcr_select_offset;
> -	unsigned int count;
>  	u32 sizeof_pcr_selection;
>  	u32 rsp_len;
>  	int rc;
> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	if (rc)
>  		goto out;
>  
> -	count = be32_to_cpup(
> +	chip->nr_active_banks = be32_to_cpup(
>  		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>  
> -	if (count > ARRAY_SIZE(chip->active_banks)) {
> -		rc = -ENODEV;
> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
> +					   sizeof(*chip->active_banks),
> +					   GFP_KERNEL);
> +	if (!chip->active_banks) {
> +		rc = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>  	end = &buf.data[rsp_len];
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < chip->nr_active_banks; i++) {
>  		pcr_select_offset = marker +
>  			offsetof(struct tpm2_pcr_selection, size_of_select);
>  		if (pcr_select_offset >= end) {
> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	}
>  
>  out:
> -	if (i < ARRAY_SIZE(chip->active_banks))
> -		chip->active_banks[i] = TPM2_ALG_ERROR;
> -
>  	tpm_buf_destroy(&buf);
>  
>  	return rc;
> -- 
> 2.17.1
> 

/Jarkko


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-07  9:41     ` Roberto Sassu
@ 2018-11-08 13:50       ` Nayna Jain
  2018-11-08 14:40         ` Roberto Sassu
  2018-11-08 15:21         ` Jarkko Sakkinen
  2018-12-13 20:21       ` Ken Goldman
  1 sibling, 2 replies; 41+ messages in thread
From: Nayna Jain @ 2018-11-08 13:50 UTC (permalink / raw)
  To: Roberto Sassu, jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Ken Goldman, Kenneth Goldman



On 11/07/2018 03:11 PM, Roberto Sassu wrote:
> On 11/7/2018 7:14 AM, Nayna Jain wrote:
>>
>>
>> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>>> This patch removes the hard-coded limit of the active_banks array size.
>>
>>
>> The hard-coded limit in static array active_banks[] represents the 
>> maximum possible banks.
>> A TPM might have three banks, but only one bank may be active.
>>
>> To confirm my understanding, is the idea for this patch is to 
>> dynamically identify the number of possible banks or the number of 
>> active banks ?
>
> The idea is to dynamically identify the number of active banks.
>
> In the TPM Commands specification (section 30.2.1), I found:
>
> TPM_CAP_PCRS – Returns the current allocation of PCR in a
> TPML_PCR_SELECTION.
>
> You mentioned:
>
> #TPM_RC_SIZE response code when count is greater
> than the possible number of banks
>
> but TPML_PCR_SELECTION is provided by the TPM.

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

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

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

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

    - Mimi & Nayna


>
> Roberto
>
>
>>> It stores in the tpm_chip structure the number of active PCR banks,
>>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>>> with a pointer to a dynamically allocated array.
>>>
>>> As a consequence of the introduction of nr_active_banks, 
>>> tpm_pcr_extend()
>>> does not check anymore if the algorithm stored in tpm_chip is equal to
>>> zero. The active_banks array always contains valid algorithms.
>>>
>>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>>> PCR banks")
>>>
>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>> ---
>>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>>   drivers/char/tpm/tpm.h           |  3 ++-
>>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>> index 46caadca916a..2a9e8b744436 100644
>>> --- a/drivers/char/tpm/tpm-chip.c
>>> +++ b/drivers/char/tpm/tpm-chip.c
>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>       kfree(chip->log.bios_event_log);
>>>       kfree(chip->work_space.context_buf);
>>>       kfree(chip->work_space.session_buf);
>>> +    kfree(chip->active_banks);
>>>       kfree(chip);
>>>   }
>>>
>>> diff --git a/drivers/char/tpm/tpm-interface.c 
>>> b/drivers/char/tpm/tpm-interface.c
>>> index 1a803b0cf980..ba7ca6b3e664 100644
>>> --- a/drivers/char/tpm/tpm-interface.c
>>> +++ b/drivers/char/tpm/tpm-interface.c
>>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip 
>>> *chip, int pcr_idx, const u8 *hash,
>>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 
>>> *hash)
>>>   {
>>>       int rc;
>>> -    struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>> -    u32 count = 0;
>>> +    struct tpm2_digest *digest_list;
>>>       int i;
>>>
>>>       chip = tpm_find_get_ops(chip);
>>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, 
>>> int pcr_idx, const u8 *hash)
>>>           return -ENODEV;
>>>
>>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>> -        memset(digest_list, 0, sizeof(digest_list));
>>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>>> +                        sizeof(*digest_list), GFP_KERNEL);
>>> +        if (!digest_list)
>>> +            return -ENOMEM;
>>>
>>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>> +        memset(digest_list, 0,
>>> +               chip->nr_active_banks * sizeof(*digest_list));
>>> +
>>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>>               digest_list[i].alg_id = chip->active_banks[i];
>>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>> -            count++;
>>>           }
>>>
>>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>>> +                     digest_list);
>>> +        kfree(digest_list);
>>>           tpm_put_ops(chip);
>>>           return rc;
>>>       }
>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>> index f3501d05264f..98368c3a6ff7 100644
>>> --- a/drivers/char/tpm/tpm.h
>>> +++ b/drivers/char/tpm/tpm.h
>>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>>       const struct attribute_group *groups[3];
>>>       unsigned int groups_cnt;
>>>
>>> -    u16 active_banks[7];
>>> +    u32 nr_active_banks;
>>> +    u16 *active_banks;
>>>   #ifdef CONFIG_ACPI
>>>       acpi_handle acpi_dev_handle;
>>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>> index c31b490bd41d..533089cede07 100644
>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>>> pcr_idx, u32 count,
>>>       int i;
>>>       int j;
>>>
>>> -    if (count > ARRAY_SIZE(chip->active_banks))
>>> +    if (count > chip->nr_active_banks)
>>>           return -EINVAL;
>>>
>>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       void *marker;
>>>       void *end;
>>>       void *pcr_select_offset;
>>> -    unsigned int count;
>>>       u32 sizeof_pcr_selection;
>>>       u32 rsp_len;
>>>       int rc;
>>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       if (rc)
>>>           goto out;
>>>
>>> -    count = be32_to_cpup(
>>> +    chip->nr_active_banks = be32_to_cpup(
>>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>
>>
>> As per my understanding, the count in the TPML_PCR_SELECTION 
>> represent the number of possible banks and not the number of active 
>> banks.
>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>> explanation of #TPM_RC_SIZE.
>>
>> Thanks & Regards,
>>      - Nayna
>>
>>
>>>
>>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>>> -        rc = -ENODEV;
>>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>>> +                       sizeof(*chip->active_banks),
>>> +                       GFP_KERNEL);
>>> +    if (!chip->active_banks) {
>>> +        rc = -ENOMEM;
>>>           goto out;
>>>       }
>>>
>>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>>       end = &buf.data[rsp_len];
>>>
>>> -    for (i = 0; i < count; i++) {
>>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>>           pcr_select_offset = marker +
>>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>>           if (pcr_select_offset >= end) {
>>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>> tpm_chip *chip)
>>>       }
>>>
>>>   out:
>>> -    if (i < ARRAY_SIZE(chip->active_banks))
>>> -        chip->active_banks[i] = TPM2_ALG_ERROR;
>>> -
>>>       tpm_buf_destroy(&buf);
>>>
>>>       return rc;
>>
>


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM
  2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (5 preceding siblings ...)
  2018-11-06 15:01 ` [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size Roberto Sassu
@ 2018-11-08 13:51 ` Jarkko Sakkinen
  6 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 13:51 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 06, 2018 at 04:01:53PM +0100, Roberto Sassu wrote:
> The TPM driver currently relies on the crypto subsystem to determine the
> digest size of supported TPM algorithms. In the future, TPM vendors might
> implement new algorithms in their chips, and those algorithms might not
> be supported by the crypto subsystem.
> 
> Usually, vendors provide patches for the new hardware, and likely
> the crypto subsystem will be updated before the new algorithm is
> introduced. However, old kernels might be updated later, after patches
> are included in the mainline kernel. This would leave the opportunity
> for attackers to misuse PCRs, as PCR banks with an unknown algorithm
> are not extended.
> 
> This patch set provides a long term solution for this issue. If a TPM
> algorithm is not known by the crypto subsystem, the TPM driver retrieves
> the digest size from the TPM with a PCR read. All the PCR banks are
> extended, even if the algorithm is not yet supported by the crypto
> subsystem.
> 
> PCR bank information (TPM algorithm ID, digest size, crypto subsystem ID)
> is stored in the tpm_chip structure and available for users of the TPM
> driver.
> 
> Changelog
> 
> v3:
> - remove end marker change
> - replace active_banks static array with pointer to dynamic array
> - remove TPM2_ACTIVE_PCR_BANKS
> 
> v2:
> - change the end marker of the active_banks array
> - check digest size from output of PCR read command
> - remove count parameter from tpm_pcr_read() and tpm2_pcr_read()
> 
> v1:
> - modify definition of tpm_pcr_read()
> - move hash algorithms and definition of tpm2_digest to include/linux/tpm.h
> 
> Roberto Sassu (6):
>   tpm: dynamically allocate active_banks array
>   tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
>   tpm: rename and export tpm2_digest and tpm2_algorithms
>   tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
>   tpm: retrieve digest size of unknown algorithms with PCR read
>   tpm: ensure that the output of PCR read contains the correct digest
>     size
> 
>  drivers/char/tpm/tpm-chip.c         |   1 +
>  drivers/char/tpm/tpm-interface.c    |  34 +++++---
>  drivers/char/tpm/tpm.h              |  19 ++---
>  drivers/char/tpm/tpm2-cmd.c         | 115 ++++++++++++++++++++--------
>  include/linux/tpm.h                 |  30 +++++++-
>  include/linux/tpm_eventlog.h        |  12 +--
>  security/integrity/ima/ima_crypto.c |  10 +--
>  7 files changed, 145 insertions(+), 76 deletions(-)
> 
> -- 
> 2.17.1
> 

You should rebase your series to the latest upstream.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
  2018-11-06 15:01 ` [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS Roberto Sassu
@ 2018-11-08 14:02   ` Jarkko Sakkinen
  2018-11-08 14:03     ` Jarkko Sakkinen
  2018-11-08 19:05   ` Jarkko Sakkinen
  1 sibling, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 14:02 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 06, 2018 at 04:01:55PM +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 and are not suitable for parsing events with type casting.
> 
> Since declaring static arrays with hard-coded sizes does not help to parse
> data after these arrays, this patch removes the declaration of
> TPM2_ACTIVE_PCR_BANKS and sets the size of the arrays above to zero.
> 
> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
> event log")
> 
> Signed-off-by: Roberto Sassu <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;

Last two fields make sense at least without comment as they overlap.

>  } __packed;
>  
> -- 
> 2.17.1
> 

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
  2018-11-08 14:02   ` Jarkko Sakkinen
@ 2018-11-08 14:03     ` Jarkko Sakkinen
  2018-11-08 14:52       ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 14:03 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Thu, Nov 08, 2018 at 04:02:08PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 04:01:55PM +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 and are not suitable for parsing events with type casting.
> > 
> > Since declaring static arrays with hard-coded sizes does not help to parse
> > data after these arrays, this patch removes the declaration of
> > TPM2_ACTIVE_PCR_BANKS and sets the size of the arrays above to zero.
> > 
> > Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
> > event log")
> > 
> > Signed-off-by: Roberto Sassu <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;
> 
> Last two fields make sense at least without comment as they overlap.

i.e. would be semantically equal to

union {
	struct tpm2_digest digests[0];
	struct tcg_event_field event;
};

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-06 15:01 ` [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm Roberto Sassu
@ 2018-11-08 14:04   ` Jarkko Sakkinen
  2018-11-08 14:16     ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 14:04 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 06, 2018 at 04:01:57PM +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>

Does not apply to the current upstream (with tpm1-cmd.c).

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-06 15:01 ` [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size Roberto Sassu
@ 2018-11-08 14:08   ` Jarkko Sakkinen
  2018-11-08 14:47     ` Roberto Sassu
  2018-11-13 13:08     ` Roberto Sassu
  0 siblings, 2 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 14:08 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 06, 2018 at 04:01:59PM +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 as argument 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>
> ---
>  drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index e2d5b84286a7..3b0b5b032901 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
>  int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>  {
> +	int i;
>  	int rc;
>  	struct tpm_buf buf;
>  	struct tpm2_pcr_read_out *out;
>  	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>  	u16 digest_size;
> +	u16 expected_digest_size = 0;
>  
>  	if (pcr_idx >= TPM2_PLATFORM_PCR)
>  		return -EINVAL;
>  
> +	if (!digest_size_ptr) {
> +		for (i = 0; i < chip->nr_active_banks &&
> +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
> +			;

Not sure if the semicolon should be in its own line.
`
> +
> +		if (i == chip->nr_active_banks)
> +			return -EINVAL;
> +
> +		expected_digest_size = chip->active_banks[i].digest_size;
> +	}
> +
>  	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>  	if (rc)
>  		return rc;
> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>  
>  	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>  	digest_size = be16_to_cpu(out->digest_size);
> -	if (digest_size > sizeof(digest_struct->digest)) {
> +	if (digest_size > sizeof(digest_struct->digest) ||
> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>  		rc = -EINVAL;
>  		goto out;
>  	}
> -- 
> 2.17.1
> 

Please add

Cc: stable@vger.kernel.org.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-08 14:04   ` Jarkko Sakkinen
@ 2018-11-08 14:16     ` Roberto Sassu
  2018-11-08 15:15       ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-08 14:16 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 04:01:57PM +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>
> 
> Does not apply to the current upstream (with tpm1-cmd.c).

Unfortunately, I cannot fetch the repository as infradead.org only
supports the git protocol (I'm behind a proxy).

Roberto


> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 13:46   ` Jarkko Sakkinen
@ 2018-11-08 14:24     ` Roberto Sassu
  2018-11-08 15:22       ` Jarkko Sakkinen
  2018-11-13 13:34     ` Roberto Sassu
  2018-11-13 13:53     ` Roberto Sassu
  2 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-08 14:24 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   {
>>   	int rc;
>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?

Should I include the patch for tpm_pcr_extend() in this patch set, even
if the set fixes the digest size issue?

Roberto


> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.
> 
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	}
>>   
>>   out:
>> -	if (i < ARRAY_SIZE(chip->active_banks))
>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>   	tpm_buf_destroy(&buf);
>>   
>>   	return rc;
>> -- 
>> 2.17.1
>>
> 
> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 13:50       ` Nayna Jain
@ 2018-11-08 14:40         ` Roberto Sassu
  2018-11-08 15:21         ` Jarkko Sakkinen
  1 sibling, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-08 14:40 UTC (permalink / raw)
  To: Nayna Jain, jarkko.sakkinen, zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Ken Goldman, Kenneth Goldman

On 11/8/2018 2:50 PM, Nayna Jain wrote:
> 
> 
> On 11/07/2018 03:11 PM, Roberto Sassu wrote:
>> On 11/7/2018 7:14 AM, Nayna Jain wrote:
>>>
>>>
>>> On 11/06/2018 08:31 PM, Roberto Sassu wrote:
>>>> This patch removes the hard-coded limit of the active_banks array size.
>>>
>>>
>>> The hard-coded limit in static array active_banks[] represents the 
>>> maximum possible banks.
>>> A TPM might have three banks, but only one bank may be active.
>>>
>>> To confirm my understanding, is the idea for this patch is to 
>>> dynamically identify the number of possible banks or the number of 
>>> active banks ?
>>
>> The idea is to dynamically identify the number of active banks.
>>
>> In the TPM Commands specification (section 30.2.1), I found:
>>
>> TPM_CAP_PCRS – Returns the current allocation of PCR in a
>> TPML_PCR_SELECTION.
>>
>> You mentioned:
>>
>> #TPM_RC_SIZE response code when count is greater
>> than the possible number of banks
>>
>> but TPML_PCR_SELECTION is provided by the TPM.
> 
> Based on a discussion with Ken, the count in the TPML_PCR_SELECTION 
> returns the number of possible algorithms supported. In the example 
> below, two possible algorithms - SHA1 and SHA256 - are returned.
> 
> # /usr/local/bin/tssgetcapability -cap 5
> 2 PCR selections
>      hash TPM_ALG_SHA1
>      TPMS_PCR_SELECTION length 3
>      ff ff ff
>      hash TPM_ALG_SHA256
>      TPMS_PCR_SELECTION length 3
>      00 00 00
> 
> The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for 
> the enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while 
> the SHA256 bank is not enabled.
> 
> The current code works, but it unnecessarily extends some banks. Instead 
> of basing the number of active banks on the number of algorithms 
> returned, it should be based on the pcr_select field.

Thanks. I will add a bank if at least one bit is set in the pcr_select
mask.

Roberto


>     - Mimi & Nayna
> 
> 
>>
>> Roberto
>>
>>
>>>> It stores in the tpm_chip structure the number of active PCR banks,
>>>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>>>> with a pointer to a dynamically allocated array.
>>>>
>>>> As a consequence of the introduction of nr_active_banks, 
>>>> tpm_pcr_extend()
>>>> does not check anymore if the algorithm stored in tpm_chip is equal to
>>>> zero. The active_banks array always contains valid algorithms.
>>>>
>>>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>>>> PCR banks")
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>>>   drivers/char/tpm/tpm.h           |  3 ++-
>>>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>>> index 46caadca916a..2a9e8b744436 100644
>>>> --- a/drivers/char/tpm/tpm-chip.c
>>>> +++ b/drivers/char/tpm/tpm-chip.c
>>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>>       kfree(chip->log.bios_event_log);
>>>>       kfree(chip->work_space.context_buf);
>>>>       kfree(chip->work_space.session_buf);
>>>> +    kfree(chip->active_banks);
>>>>       kfree(chip);
>>>>   }
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c 
>>>> b/drivers/char/tpm/tpm-interface.c
>>>> index 1a803b0cf980..ba7ca6b3e664 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip 
>>>> *chip, int pcr_idx, const u8 *hash,
>>>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 
>>>> *hash)
>>>>   {
>>>>       int rc;
>>>> -    struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>>> -    u32 count = 0;
>>>> +    struct tpm2_digest *digest_list;
>>>>       int i;
>>>>
>>>>       chip = tpm_find_get_ops(chip);
>>>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, 
>>>> int pcr_idx, const u8 *hash)
>>>>           return -ENODEV;
>>>>
>>>>       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> -        memset(digest_list, 0, sizeof(digest_list));
>>>> +        digest_list = kmalloc_array(chip->nr_active_banks,
>>>> +                        sizeof(*digest_list), GFP_KERNEL);
>>>> +        if (!digest_list)
>>>> +            return -ENOMEM;
>>>>
>>>> -        for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>>>> -                chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>>>> +        memset(digest_list, 0,
>>>> +               chip->nr_active_banks * sizeof(*digest_list));
>>>> +
>>>> +        for (i = 0; i < chip->nr_active_banks; i++) {
>>>>               digest_list[i].alg_id = chip->active_banks[i];
>>>>               memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>> -            count++;
>>>>           }
>>>>
>>>> -        rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>>>> +        rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>>>> +                     digest_list);
>>>> +        kfree(digest_list);
>>>>           tpm_put_ops(chip);
>>>>           return rc;
>>>>       }
>>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>>> index f3501d05264f..98368c3a6ff7 100644
>>>> --- a/drivers/char/tpm/tpm.h
>>>> +++ b/drivers/char/tpm/tpm.h
>>>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>>>       const struct attribute_group *groups[3];
>>>>       unsigned int groups_cnt;
>>>>
>>>> -    u16 active_banks[7];
>>>> +    u32 nr_active_banks;
>>>> +    u16 *active_banks;
>>>>   #ifdef CONFIG_ACPI
>>>>       acpi_handle acpi_dev_handle;
>>>>       char ppi_version[TPM_PPI_VERSION_LEN + 1];
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index c31b490bd41d..533089cede07 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int 
>>>> pcr_idx, u32 count,
>>>>       int i;
>>>>       int j;
>>>>
>>>> -    if (count > ARRAY_SIZE(chip->active_banks))
>>>> +    if (count > chip->nr_active_banks)
>>>>           return -EINVAL;
>>>>
>>>>       rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       void *marker;
>>>>       void *end;
>>>>       void *pcr_select_offset;
>>>> -    unsigned int count;
>>>>       u32 sizeof_pcr_selection;
>>>>       u32 rsp_len;
>>>>       int rc;
>>>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       if (rc)
>>>>           goto out;
>>>>
>>>> -    count = be32_to_cpup(
>>>> +    chip->nr_active_banks = be32_to_cpup(
>>>>           (__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>>
>>>
>>> As per my understanding, the count in the TPML_PCR_SELECTION 
>>> represent the number of possible banks and not the number of active 
>>> banks.
>>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>>> explanation of #TPM_RC_SIZE.
>>>
>>> Thanks & Regards,
>>>      - Nayna
>>>
>>>
>>>>
>>>> -    if (count > ARRAY_SIZE(chip->active_banks)) {
>>>> -        rc = -ENODEV;
>>>> +    chip->active_banks = kmalloc_array(chip->nr_active_banks,
>>>> +                       sizeof(*chip->active_banks),
>>>> +                       GFP_KERNEL);
>>>> +    if (!chip->active_banks) {
>>>> +        rc = -ENOMEM;
>>>>           goto out;
>>>>       }
>>>>
>>>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>>>       end = &buf.data[rsp_len];
>>>>
>>>> -    for (i = 0; i < count; i++) {
>>>> +    for (i = 0; i < chip->nr_active_banks; i++) {
>>>>           pcr_select_offset = marker +
>>>>               offsetof(struct tpm2_pcr_selection, size_of_select);
>>>>           if (pcr_select_offset >= end) {
>>>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct 
>>>> tpm_chip *chip)
>>>>       }
>>>>
>>>>   out:
>>>> -    if (i < ARRAY_SIZE(chip->active_banks))
>>>> -        chip->active_banks[i] = TPM2_ALG_ERROR;
>>>> -
>>>>       tpm_buf_destroy(&buf);
>>>>
>>>>       return rc;
>>>
>>
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-08 14:08   ` Jarkko Sakkinen
@ 2018-11-08 14:47     ` Roberto Sassu
  2018-11-08 18:52       ` Jarkko Sakkinen
  2018-11-13 13:08     ` Roberto Sassu
  1 sibling, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-08 14:47 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 04:01:59PM +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 as argument 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>
>> ---
>>   drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index e2d5b84286a7..3b0b5b032901 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
>>   int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>>   {
>> +	int i;
>>   	int rc;
>>   	struct tpm_buf buf;
>>   	struct tpm2_pcr_read_out *out;
>>   	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>>   	u16 digest_size;
>> +	u16 expected_digest_size = 0;
>>   
>>   	if (pcr_idx >= TPM2_PLATFORM_PCR)
>>   		return -EINVAL;
>>   
>> +	if (!digest_size_ptr) {
>> +		for (i = 0; i < chip->nr_active_banks &&
>> +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
>> +			;
> 
> Not sure if the semicolon should be in its own line.

scripts/Lindent suggests:

for (i = 0; i < chip->nr_active_banks &&
      chip->active_banks[i].alg_id != digest_struct->alg_id;
      i++) ;

but this is not accepted by scripts/checkpatch.pl (there are no
warnings/errors on the patch I sent).


>> +
>> +		if (i == chip->nr_active_banks)
>> +			return -EINVAL;
>> +
>> +		expected_digest_size = chip->active_banks[i].digest_size;
>> +	}
>> +
>>   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>>   	if (rc)
>>   		return rc;
>> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   
>>   	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>>   	digest_size = be16_to_cpu(out->digest_size);
>> -	if (digest_size > sizeof(digest_struct->digest)) {
>> +	if (digest_size > sizeof(digest_struct->digest) ||
>> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>>   		rc = -EINVAL;
>>   		goto out;
>>   	}
>> -- 
>> 2.17.1
>>
> 
> Please add
> 
> Cc: stable@vger.kernel.org.
> 
> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
  2018-11-08 14:03     ` Jarkko Sakkinen
@ 2018-11-08 14:52       ` Roberto Sassu
  0 siblings, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-08 14:52 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 3:03 PM, Jarkko Sakkinen wrote:
> On Thu, Nov 08, 2018 at 04:02:08PM +0200, Jarkko Sakkinen wrote:
>> On Tue, Nov 06, 2018 at 04:01:55PM +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 and are not suitable for parsing events with type casting.
>>>
>>> Since declaring static arrays with hard-coded sizes does not help to parse
>>> data after these arrays, this patch removes the declaration of
>>> TPM2_ACTIVE_PCR_BANKS and sets the size of the arrays above to zero.
>>>
>>> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
>>> event log")
>>>
>>> Signed-off-by: Roberto Sassu <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;
>>
>> Last two fields make sense at least without comment as they overlap.
> 
> i.e. would be semantically equal to
> 
> union {
> 	struct tpm2_digest digests[0];
> 	struct tcg_event_field event;
> };

I didn't understand. Should I add the union?

Roberto


> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-08 14:16     ` Roberto Sassu
@ 2018-11-08 15:15       ` Jarkko Sakkinen
  2018-11-08 15:19         ` Peter Huewe
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 15:15 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
> On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
> > On Tue, Nov 06, 2018 at 04:01:57PM +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>
> > 
> > Does not apply to the current upstream (with tpm1-cmd.c).
> 
> Unfortunately, I cannot fetch the repository as infradead.org only
> supports the git protocol (I'm behind a proxy).
> 
> Roberto

I use a proxy script similar to this:

https://gist.github.com/sit/49288

(random googling but gives the idea)

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-08 15:15       ` Jarkko Sakkinen
@ 2018-11-08 15:19         ` Peter Huewe
  2018-11-08 19:08           ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Peter Huewe @ 2018-11-08 15:19 UTC (permalink / raw)
  To: Jarkko Sakkinen, Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu



Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
>> On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
>> > On Tue, Nov 06, 2018 at 04:01:57PM +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>
>> > 
>> > Does not apply to the current upstream (with tpm1-cmd.c).
>> 
>> Unfortunately, I cannot fetch the repository as infradead.org only
>> supports the git protocol (I'm behind a proxy).
>> 
>> Roberto
>
>I use a proxy script similar to this:
>
>https://gist.github.com/sit/49288
>
>(random googling but gives the idea)
>
>/Jarkko
Moving to a kernel.org repo would be really a benefit or convincing them to have a https interface as well.
We have the same proxy issue with infradead.
Peter
-- 
Sent from my mobile

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 13:50       ` Nayna Jain
  2018-11-08 14:40         ` Roberto Sassu
@ 2018-11-08 15:21         ` Jarkko Sakkinen
  2018-11-08 15:29           ` Mimi Zohar
  2018-11-08 15:54           ` Ken Goldman
  1 sibling, 2 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 15:21 UTC (permalink / raw)
  To: Nayna Jain
  Cc: Roberto Sassu, zohar, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, Ken Goldman, Kenneth Goldman

On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> the number of possible algorithms supported. In the example below, two
> possible algorithms - SHA1 and SHA256 - are returned.
> 
> # /usr/local/bin/tssgetcapability -cap 5
> 2 PCR selections
>     hash TPM_ALG_SHA1
>     TPMS_PCR_SELECTION length 3
>     ff ff ff
>     hash TPM_ALG_SHA256
>     TPMS_PCR_SELECTION length 3
>     00 00 00
> 
> The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> bank is not enabled.
> 
> The current code works, but it unnecessarily extends some banks. Instead of
> basing the number of active banks on the number of algorithms returned, it
> should be based on the pcr_select field.
> 
>    - Mimi & Nayna

I would just allocate array of the size of possible banks and grow
nr_active_banks for active algorithms to keep the code simple because
we are talking about insignificant amount of wasted space (might be
even zero bytes given how kernel allocators works)>

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 14:24     ` Roberto Sassu
@ 2018-11-08 15:22       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 15:22 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

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.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 15:21         ` Jarkko Sakkinen
@ 2018-11-08 15:29           ` Mimi Zohar
  2018-11-08 18:57             ` Jarkko Sakkinen
  2018-11-08 15:54           ` Ken Goldman
  1 sibling, 1 reply; 41+ messages in thread
From: Mimi Zohar @ 2018-11-08 15:29 UTC (permalink / raw)
  To: Jarkko Sakkinen, Nayna Jain
  Cc: Roberto Sassu, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu, Ken Goldman, Kenneth Goldman

On Thu, 2018-11-08 at 17:21 +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> > Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> > the number of possible algorithms supported. In the example below, two
> > possible algorithms - SHA1 and SHA256 - are returned.
> > 
> > # /usr/local/bin/tssgetcapability -cap 5
> > 2 PCR selections
> >     hash TPM_ALG_SHA1
> >     TPMS_PCR_SELECTION length 3
> >     ff ff ff
> >     hash TPM_ALG_SHA256
> >     TPMS_PCR_SELECTION length 3
> >     00 00 00
> > 
> > The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> > enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> > bank is not enabled.
> > 
> > The current code works, but it unnecessarily extends some banks. Instead of
> > basing the number of active banks on the number of algorithms returned, it
> > should be based on the pcr_select field.
> > 
> >    - Mimi & Nayna
> 
> I would just allocate array of the size of possible banks and grow
> nr_active_banks for active algorithms to keep the code simple because
> we are talking about insignificant amount of wasted space (might be
> even zero bytes given how kernel allocators works)>

That's fine.  Remember the memory is just one concern, but the other
concerns are the performance of calculating the unneeded hash and the
TPM performance of including it in the PCR extend.

Mimi


^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 15:21         ` Jarkko Sakkinen
  2018-11-08 15:29           ` Mimi Zohar
@ 2018-11-08 15:54           ` Ken Goldman
  1 sibling, 0 replies; 41+ messages in thread
From: Ken Goldman @ 2018-11-08 15:54 UTC (permalink / raw)
  To: unlisted-recipients:; (no To-header on input)
  Cc: linux-integrity, linux-security-module

On 11/8/2018 10:21 AM, Jarkko Sakkinen wrote:

> I would just allocate array of the size of possible banks and grow
> nr_active_banks for active algorithms to keep the code simple because
> we are talking about insignificant amount of wasted space (might be
> even zero bytes given how kernel allocators works)>

Just beware that "possible banks" is tricky.  While 2 is typical, 
getcapability will return a bitmap for each digest algorithm.  This is
also currently 2, but will be 3 in the next TPM, is 4 in the SW TPM,
and is potentially even higher.

Also, account for a TPM that is malicious and can return a count
as high as 0xffffffff.  Range check count.




^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-08 14:47     ` Roberto Sassu
@ 2018-11-08 18:52       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 18:52 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Thu, Nov 08, 2018 at 03:47:39PM +0100, Roberto Sassu wrote:
> > > +		for (i = 0; i < chip->nr_active_banks &&
> > > +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
> > > +			;
> > 
> > Not sure if the semicolon should be in its own line.
> 
> scripts/Lindent suggests:
> 
> for (i = 0; i < chip->nr_active_banks &&
>      chip->active_banks[i].alg_id != digest_struct->alg_id;
>      i++) ;
> 
> but this is not accepted by scripts/checkpatch.pl (there are no
> warnings/errors on the patch I sent).

Yeah, not a really blocker for me.

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

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 15:29           ` Mimi Zohar
@ 2018-11-08 18:57             ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 18:57 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Nayna Jain, Roberto Sassu, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Ken Goldman, Kenneth Goldman

On Thu, Nov 08, 2018 at 10:29:53AM -0500, Mimi Zohar wrote:
> On Thu, 2018-11-08 at 17:21 +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 08, 2018 at 07:20:51PM +0530, Nayna Jain wrote:
> > > Based on a discussion with Ken, the count in the TPML_PCR_SELECTION returns
> > > the number of possible algorithms supported. In the example below, two
> > > possible algorithms - SHA1 and SHA256 - are returned.
> > > 
> > > # /usr/local/bin/tssgetcapability -cap 5
> > > 2 PCR selections
> > >     hash TPM_ALG_SHA1
> > >     TPMS_PCR_SELECTION length 3
> > >     ff ff ff
> > >     hash TPM_ALG_SHA256
> > >     TPMS_PCR_SELECTION length 3
> > >     00 00 00
> > > 
> > > The pcr_select fields - "ff ff ff" and "00 00 00" - are bit masks for the
> > > enabled PCRs. The SHA1 bank is enabled for all PCRs (0-23), while the SHA256
> > > bank is not enabled.
> > > 
> > > The current code works, but it unnecessarily extends some banks. Instead of
> > > basing the number of active banks on the number of algorithms returned, it
> > > should be based on the pcr_select field.
> > > 
> > >    - Mimi & Nayna
> > 
> > I would just allocate array of the size of possible banks and grow
> > nr_active_banks for active algorithms to keep the code simple because
> > we are talking about insignificant amount of wasted space (might be
> > even zero bytes given how kernel allocators works)>
> 
> That's fine.  Remember the memory is just one concern, but the other
> concerns are the performance of calculating the unneeded hash and the
> TPM performance of including it in the PCR extend.

The driver would initialize only as many entries as are active array and
set nr_active_banks accordingly.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS
  2018-11-06 15:01 ` [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS Roberto Sassu
  2018-11-08 14:02   ` Jarkko Sakkinen
@ 2018-11-08 19:05   ` Jarkko Sakkinen
  1 sibling, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 19:05 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 06, 2018 at 04:01:55PM +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 and are not suitable for parsing events with type casting.
> 
> Since declaring static arrays with hard-coded sizes does not help to parse
> data after these arrays, this patch removes the declaration of
> TPM2_ACTIVE_PCR_BANKS and sets the size of the arrays above to zero.
> 
> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware
> event log")
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 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
> 

I somehow lost your response but what you must do is to explain why is
it OK for last two fields to overlap.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-08 15:19         ` Peter Huewe
@ 2018-11-08 19:08           ` Jarkko Sakkinen
  2018-11-13 12:34             ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-08 19:08 UTC (permalink / raw)
  To: Peter Huewe
  Cc: Roberto Sassu, zohar, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu

On Thu, Nov 08, 2018 at 04:19:09PM +0100, Peter Huewe wrote:
> 
> 
> Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
> >On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
> >> On 11/8/2018 3:04 PM, Jarkko Sakkinen wrote:
> >> > On Tue, Nov 06, 2018 at 04:01:57PM +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>
> >> > 
> >> > Does not apply to the current upstream (with tpm1-cmd.c).
> >> 
> >> Unfortunately, I cannot fetch the repository as infradead.org only
> >> supports the git protocol (I'm behind a proxy).
> >> 
> >> Roberto
> >
> >I use a proxy script similar to this:
> >
> >https://gist.github.com/sit/49288
> >
> >(random googling but gives the idea)
> >
> >/Jarkko
> Moving to a kernel.org repo would be really a benefit or convincing them to have a https interface as well.
> We have the same proxy issue with infradead.
> Peter
> -- 
> Sent from my mobile

So you are unable to use core.gitproxy to configure the proxy?

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-08 19:08           ` Jarkko Sakkinen
@ 2018-11-13 12:34             ` Jarkko Sakkinen
  2018-11-13 12:39               ` Roberto Sassu
  0 siblings, 1 reply; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-13 12:34 UTC (permalink / raw)
  To: Peter Huewe
  Cc: Roberto Sassu, zohar, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu

On Thu, Nov 08, 2018 at 09:08:35PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 08, 2018 at 04:19:09PM +0100, Peter Huewe wrote:
> > Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
> > >On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
> > >> 
> > >> Unfortunately, I cannot fetch the repository as infradead.org only
> > >> supports the git protocol (I'm behind a proxy).
> > >> 
> > >> Roberto
> > >
> > >I use a proxy script similar to this:
> > >
> > >https://gist.github.com/sit/49288
> > >
> > >(random googling but gives the idea)
> > >
> > >/Jarkko
> > Moving to a kernel.org repo would be really a benefit or convincing them to have a https interface as well.
> > We have the same proxy issue with infradead.
> > Peter
> > -- 
> > Sent from my mobile
> 
> So you are unable to use core.gitproxy to configure the proxy?

AFAIK the kernel development process does not disallow to use direct
git protocol for maintainer branches. Please, correct me if I'm
mistaken.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-13 12:34             ` Jarkko Sakkinen
@ 2018-11-13 12:39               ` Roberto Sassu
  2018-11-13 16:56                 ` Jarkko Sakkinen
  0 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-13 12:39 UTC (permalink / raw)
  To: Jarkko Sakkinen, Peter Huewe
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/13/2018 1:34 PM, Jarkko Sakkinen wrote:
> On Thu, Nov 08, 2018 at 09:08:35PM +0200, Jarkko Sakkinen wrote:
>> On Thu, Nov 08, 2018 at 04:19:09PM +0100, Peter Huewe wrote:
>>> Am 8. November 2018 16:15:04 MEZ schrieb Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>:
>>>> On Thu, Nov 08, 2018 at 03:16:03PM +0100, Roberto Sassu wrote:
>>>>>
>>>>> Unfortunately, I cannot fetch the repository as infradead.org only
>>>>> supports the git protocol (I'm behind a proxy).
>>>>>
>>>>> Roberto
>>>>
>>>> I use a proxy script similar to this:
>>>>
>>>> https://gist.github.com/sit/49288
>>>>
>>>> (random googling but gives the idea)
>>>>
>>>> /Jarkko
>>> Moving to a kernel.org repo would be really a benefit or convincing them to have a https interface as well.
>>> We have the same proxy issue with infradead.
>>> Peter
>>> -- 
>>> Sent from my mobile
>>
>> So you are unable to use core.gitproxy to configure the proxy?
> 
> AFAIK the kernel development process does not disallow to use direct
> git protocol for maintainer branches. Please, correct me if I'm
> mistaken.

I solved the issue by creating a mirror of your repository with gitlab.

Roberto


> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-08 14:08   ` Jarkko Sakkinen
  2018-11-08 14:47     ` Roberto Sassu
@ 2018-11-13 13:08     ` Roberto Sassu
  2018-11-13 16:59       ` Jarkko Sakkinen
  1 sibling, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-13 13:08 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote:
> On Tue, Nov 06, 2018 at 04:01:59PM +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 as argument 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>
>> ---
>>   drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
>>   1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index e2d5b84286a7..3b0b5b032901 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
>>   int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
>>   {
>> +	int i;
>>   	int rc;
>>   	struct tpm_buf buf;
>>   	struct tpm2_pcr_read_out *out;
>>   	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
>>   	u16 digest_size;
>> +	u16 expected_digest_size = 0;
>>   
>>   	if (pcr_idx >= TPM2_PLATFORM_PCR)
>>   		return -EINVAL;
>>   
>> +	if (!digest_size_ptr) {
>> +		for (i = 0; i < chip->nr_active_banks &&
>> +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
>> +			;
> 
> Not sure if the semicolon should be in its own line.
> `
>> +
>> +		if (i == chip->nr_active_banks)
>> +			return -EINVAL;
>> +
>> +		expected_digest_size = chip->active_banks[i].digest_size;
>> +	}
>> +
>>   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
>>   	if (rc)
>>   		return rc;
>> @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
>>   
>>   	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
>>   	digest_size = be16_to_cpu(out->digest_size);
>> -	if (digest_size > sizeof(digest_struct->digest)) {
>> +	if (digest_size > sizeof(digest_struct->digest) ||
>> +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
>>   		rc = -EINVAL;
>>   		goto out;
>>   	}
>> -- 
>> 2.17.1
>>
> 
> Please add
> 
> Cc: stable@vger.kernel.org.

Should I do the same for the previous patches? This patch cannot be
applied alone.

Roberto


> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 13:46   ` Jarkko Sakkinen
  2018-11-08 14:24     ` Roberto Sassu
@ 2018-11-13 13:34     ` Roberto Sassu
  2018-11-13 17:04       ` Jarkko Sakkinen
  2018-11-13 13:53     ` Roberto Sassu
  2 siblings, 1 reply; 41+ messages in thread
From: Roberto Sassu @ 2018-11-13 13:34 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   {
>>   	int rc;
>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?
> 
> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.

I have to move also other structures and #define. Wouldn't be better to
introduce a new function to pass to the caller active_banks and
nr_active_banks?

Roberto


>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	}
>>   
>>   out:
>> -	if (i < ARRAY_SIZE(chip->active_banks))
>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>   	tpm_buf_destroy(&buf);
>>   
>>   	return rc;
>> -- 
>> 2.17.1
>>
> 
> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-08 13:46   ` Jarkko Sakkinen
  2018-11-08 14:24     ` Roberto Sassu
  2018-11-13 13:34     ` Roberto Sassu
@ 2018-11-13 13:53     ` Roberto Sassu
  2 siblings, 0 replies; 41+ messages in thread
From: Roberto Sassu @ 2018-11-13 13:53 UTC (permalink / raw)
  To: Jarkko Sakkinen, Safford, David (GE Global Research, US)
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Wiseman, Monty (GE Global Research, US)

Adding in CC Monty and Dave.


On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
>> This patch removes the hard-coded limit of the active_banks array size.
>> It stores in the tpm_chip structure the number of active PCR banks,
>> determined in tpm2_get_pcr_allocation(), and replaces the static array
>> with a pointer to a dynamically allocated array.
>>
>> As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
>> does not check anymore if the algorithm stored in tpm_chip is equal to
>> zero. The active_banks array always contains valid algorithms.
>>
>> Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
>> PCR banks")
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
>>   4 files changed, 23 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 46caadca916a..2a9e8b744436 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->active_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index 1a803b0cf980..ba7ca6b3e664 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
>>   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   {
>>   	int rc;
>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
>>   		return -ENODEV;
> 
> Should take digest_list as input. Probably callers could re-use the
> same digest_list array multiple times?

I plan to introduce a new structure with the digest size, as Monty
suggested at LSS. The name of the new structure is tpm_bank_list.

The benefit is that we don't have to rely on the TPM driver or the
crypto subsystem to find the digest size for the TPM algorithms
specified by the users of the TPM driver (it is not mandatory that the
users check the list of active banks).

It could happen that the TPM driver does not know the size of SHA256 if
the SHA256 bank was not allocated. Then, it has to search in the array
containing the mapping between TPM algorithm IDs and crypto IDs.

Since it is the user of the TPM driver that calculates the digest,
passing the digest size to tpm_pcr_extend() does not require too much
effort, and it also simplifies the TPM driver code.

Roberto


> Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> nr_active_banks and active_banks and can create the digest array by
> itself. Lets do this right at once now that this is being restructured.
> 
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kmalloc_array(chip->nr_active_banks,
>> +					    sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
>>   
>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> +		memset(digest_list, 0,
>> +		       chip->nr_active_banks * sizeof(*digest_list));
> 
> You should use kcalloc() to allocate digest_list.
> 
>> +
>> +		for (i = 0; i < chip->nr_active_banks; i++) {
>>   			digest_list[i].alg_id = chip->active_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_active_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f3501d05264f..98368c3a6ff7 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -248,7 +248,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_active_banks;
>> +	u16 *active_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index c31b490bd41d..533089cede07 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -242,7 +242,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_active_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -859,7 +859,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>>   	u32 rsp_len;
>>   	int rc;
>> @@ -878,11 +877,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	chip->nr_active_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->active_banks = kmalloc_array(chip->nr_active_banks,
>> +					   sizeof(*chip->active_banks),
>> +					   GFP_KERNEL);
>> +	if (!chip->active_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -891,7 +893,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < chip->nr_active_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -908,9 +910,6 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	}
>>   
>>   out:
>> -	if (i < ARRAY_SIZE(chip->active_banks))
>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>   	tpm_buf_destroy(&buf);
>>   
>>   	return rc;
>> -- 
>> 2.17.1
>>
> 
> /Jarkko
> 

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm
  2018-11-13 12:39               ` Roberto Sassu
@ 2018-11-13 16:56                 ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-13 16:56 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: Peter Huewe, zohar, linux-integrity, linux-security-module,
	linux-kernel, silviu.vlasceanu

On Tue, Nov 13, 2018 at 01:39:26PM +0100, Roberto Sassu wrote:
> > AFAIK the kernel development process does not disallow to use direct
> > git protocol for maintainer branches. Please, correct me if I'm
> > mistaken.
> 
> I solved the issue by creating a mirror of your repository with gitlab.

Ok, great!

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size
  2018-11-13 13:08     ` Roberto Sassu
@ 2018-11-13 16:59       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-13 16:59 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 13, 2018 at 02:08:39PM +0100, Roberto Sassu wrote:
> On 11/8/2018 3:08 PM, Jarkko Sakkinen wrote:
> > On Tue, Nov 06, 2018 at 04:01:59PM +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 as argument 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>
> > > ---
> > >   drivers/char/tpm/tpm2-cmd.c | 16 +++++++++++++++-
> > >   1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > > index e2d5b84286a7..3b0b5b032901 100644
> > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > @@ -187,15 +187,28 @@ struct tpm2_pcr_read_out {
> > >   int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
> > >   		  struct tpm_digest *digest_struct, u16 *digest_size_ptr)
> > >   {
> > > +	int i;
> > >   	int rc;
> > >   	struct tpm_buf buf;
> > >   	struct tpm2_pcr_read_out *out;
> > >   	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
> > >   	u16 digest_size;
> > > +	u16 expected_digest_size = 0;
> > >   	if (pcr_idx >= TPM2_PLATFORM_PCR)
> > >   		return -EINVAL;
> > > +	if (!digest_size_ptr) {
> > > +		for (i = 0; i < chip->nr_active_banks &&
> > > +		     chip->active_banks[i].alg_id != digest_struct->alg_id; i++)
> > > +			;
> > 
> > Not sure if the semicolon should be in its own line.
> > `
> > > +
> > > +		if (i == chip->nr_active_banks)
> > > +			return -EINVAL;
> > > +
> > > +		expected_digest_size = chip->active_banks[i].digest_size;
> > > +	}
> > > +
> > >   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
> > >   	if (rc)
> > >   		return rc;
> > > @@ -215,7 +228,8 @@ int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx,
> > >   	out = (struct tpm2_pcr_read_out *)&buf.data[TPM_HEADER_SIZE];
> > >   	digest_size = be16_to_cpu(out->digest_size);
> > > -	if (digest_size > sizeof(digest_struct->digest)) {
> > > +	if (digest_size > sizeof(digest_struct->digest) ||
> > > +	    (!digest_size_ptr && digest_size != expected_digest_size)) {
> > >   		rc = -EINVAL;
> > >   		goto out;
> > >   	}
> > > -- 
> > > 2.17.1
> > > 
> > 
> > Please add
> > 
> > Cc: stable@vger.kernel.org.
> 
> Should I do the same for the previous patches? This patch cannot be
> applied alone.
> 
> Roberto

No need. It is an issue that we deal with depenendent commits once it is
being backported. This could be dependent for example of a commit that
is even not in the series so does not make sense to do it now.

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-13 13:34     ` Roberto Sassu
@ 2018-11-13 17:04       ` Jarkko Sakkinen
  0 siblings, 0 replies; 41+ messages in thread
From: Jarkko Sakkinen @ 2018-11-13 17:04 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu

On Tue, Nov 13, 2018 at 02:34:39PM +0100, Roberto Sassu wrote:
> On 11/8/2018 2:46 PM, Jarkko Sakkinen wrote:
> > Orrayn Tue, Nov 06, 2018 at 04:01:54PM +0100, Roberto Sassu wrote:
> > > This patch removes the hard-coded limit of the active_banks array size.
> > > It stores in the tpm_chip structure the number of active PCR banks,
> > > determined in tpm2_get_pcr_allocation(), and replaces the static array
> > > with a pointer to a dynamically allocated array.
> > > 
> > > As a consequence of the introduction of nr_active_banks, tpm_pcr_extend()
> > > does not check anymore if the algorithm stored in tpm_chip is equal to
> > > zero. The active_banks array always contains valid algorithms.
> > > 
> > > Fixes: 1db15344f874 ("tpm: implement TPM 2.0 capability to get active
> > > PCR banks")
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >   drivers/char/tpm/tpm-chip.c      |  1 +
> > >   drivers/char/tpm/tpm-interface.c | 19 ++++++++++++-------
> > >   drivers/char/tpm/tpm.h           |  3 ++-
> > >   drivers/char/tpm/tpm2-cmd.c      | 17 ++++++++---------
> > >   4 files changed, 23 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 46caadca916a..2a9e8b744436 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
> > >   	kfree(chip->log.bios_event_log);
> > >   	kfree(chip->work_space.context_buf);
> > >   	kfree(chip->work_space.session_buf);
> > > +	kfree(chip->active_banks);
> > >   	kfree(chip);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 1a803b0cf980..ba7ca6b3e664 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -1039,8 +1039,7 @@ static int tpm1_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash,
> > >   int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> > >   {
> > >   	int rc;
> > > -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> > > -	u32 count = 0;
> > > +	struct tpm2_digest *digest_list;
> > >   	int i;
> > >   	chip = tpm_find_get_ops(chip);
> > > @@ -1048,16 +1047,22 @@ int tpm_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
> > >   		return -ENODEV;
> > 
> > Should take digest_list as input. Probably callers could re-use the
> > same digest_list array multiple times?
> > 
> > Move struct tpm_chip to include/linux/tpm.h so that the caller can query
> > nr_active_banks and active_banks and can create the digest array by
> > itself. Lets do this right at once now that this is being restructured.
> 
> I have to move also other structures and #define. Wouldn't be better to
> introduce a new function to pass to the caller active_banks and
> nr_active_banks?

Revisited. I think it is fine how it is for now and we reconsider
later. Only thing I want to remark is that use should use kcalloc()
instead of kalloc_array() + memset().

/Jarkko

^ permalink raw reply	[flat|nested] 41+ messages in thread

* Re: [PATCH v4 1/6] tpm: dynamically allocate active_banks array
  2018-11-07  9:41     ` Roberto Sassu
  2018-11-08 13:50       ` Nayna Jain
@ 2018-12-13 20:21       ` Ken Goldman
  1 sibling, 0 replies; 41+ messages in thread
From: Ken Goldman @ 2018-12-13 20:21 UTC (permalink / raw)
  To: Roberto Sassu; +Cc: linux-integrity, linux-security-module

On 11/7/2018 4:41 AM, Roberto Sassu wrote:
> On 11/7/2018 7:14 AM, Nayna Jain wrote:
> 
> In the TPM Commands specification (section 30.2.1), I found:
> 
> TPM_CAP_PCRS – Returns the current allocation of PCR in a
> TPML_PCR_SELECTION.
> 
> You mentioned:
> 
> #TPM_RC_SIZE response code when count is greater
> than the possible number of banks
> 
> but TPML_PCR_SELECTION is provided by the TPM.
> 
> Roberto
> 
> 
[snip]
>>
>>
>> As per my understanding, the count in the TPML_PCR_SELECTION represent 
>> the number of possible banks and not the number of active banks.
>> TCG Structures Spec for TPM 2.0 - Table 102 mentions this as 
>> explanation of #TPM_RC_SIZE.

FYI: This was clarified in the TCG's TPM work group today.  TPM_CAP_PCRS 
returns:

The TPML_PCR_SELECTION must include a TPMS_PCR_SELECTION for each PCR 
bank in which there is at least one allocated PCR. The 
TPML_PCR_SELECTION may return a TPMS_PCR_SELECTION for each implemented 
PCR bank.  The TPML_PCR_SELECTION may return a TPMS_PCR_SELECTION for 
each implemented hash algorithm.

Also:

The TPM doesn't use the term "active banks"

Allocated = a bank that has at least one PCR bit set in the selection 
bitmap.

Supported or implemented banks = the number of PCR banks that can be 
allocated, based on the TPM hardware.

Hash algorithms = The hash algorithms supported by the TPM

For example, the TPM may support 3 hash algorithms and 2 PCR banks, and 
have 1 bank allocated.





^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, back to index

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-06 15:01 [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
2018-11-06 15:01 ` [PATCH v4 1/6] tpm: dynamically allocate active_banks array Roberto Sassu
2018-11-07  6:14   ` Nayna Jain
2018-11-07  9:41     ` Roberto Sassu
2018-11-08 13:50       ` Nayna Jain
2018-11-08 14:40         ` Roberto Sassu
2018-11-08 15:21         ` Jarkko Sakkinen
2018-11-08 15:29           ` Mimi Zohar
2018-11-08 18:57             ` Jarkko Sakkinen
2018-11-08 15:54           ` Ken Goldman
2018-12-13 20:21       ` Ken Goldman
2018-11-07 11:10     ` Mimi Zohar
2018-11-08 13:46   ` Jarkko Sakkinen
2018-11-08 14:24     ` Roberto Sassu
2018-11-08 15:22       ` Jarkko Sakkinen
2018-11-13 13:34     ` Roberto Sassu
2018-11-13 17:04       ` Jarkko Sakkinen
2018-11-13 13:53     ` Roberto Sassu
2018-11-06 15:01 ` [PATCH v4 2/6] tpm: remove definition of TPM2_ACTIVE_PCR_BANKS Roberto Sassu
2018-11-08 14:02   ` Jarkko Sakkinen
2018-11-08 14:03     ` Jarkko Sakkinen
2018-11-08 14:52       ` Roberto Sassu
2018-11-08 19:05   ` Jarkko Sakkinen
2018-11-06 15:01 ` [PATCH v4 3/6] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
2018-11-06 15:01 ` [PATCH v4 4/6] tpm: modify tpm_pcr_read() definition to pass a TPM hash algorithm Roberto Sassu
2018-11-08 14:04   ` Jarkko Sakkinen
2018-11-08 14:16     ` Roberto Sassu
2018-11-08 15:15       ` Jarkko Sakkinen
2018-11-08 15:19         ` Peter Huewe
2018-11-08 19:08           ` Jarkko Sakkinen
2018-11-13 12:34             ` Jarkko Sakkinen
2018-11-13 12:39               ` Roberto Sassu
2018-11-13 16:56                 ` Jarkko Sakkinen
2018-11-06 15:01 ` [PATCH v4 5/6] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
2018-11-06 15:01 ` [PATCH v4 6/6] tpm: ensure that the output of PCR read contains the correct digest size Roberto Sassu
2018-11-08 14:08   ` Jarkko Sakkinen
2018-11-08 14:47     ` Roberto Sassu
2018-11-08 18:52       ` Jarkko Sakkinen
2018-11-13 13:08     ` Roberto Sassu
2018-11-13 16:59       ` Jarkko Sakkinen
2018-11-08 13:51 ` [PATCH v4 0/6] tpm: retrieve digest size of unknown algorithms from TPM Jarkko Sakkinen

Linux-Security-Module Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-security-module/0 linux-security-module/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-security-module linux-security-module/ https://lore.kernel.org/linux-security-module \
		linux-security-module@vger.kernel.org linux-security-module@archiver.kernel.org
	public-inbox-index linux-security-module


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-security-module


AGPL code for this site: git clone https://public-inbox.org/ public-inbox