linux-security-module.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM
@ 2018-12-13 10:29 Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

Update

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


Original patch set description

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

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

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

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

Changelog

v6:
- squash patches 4-6
- rename tpm_bank_list to tpm_extend_digest, extend_size and extend_data
  members to size and data
- add comment in tpm2_init_bank_info()

v5:
- rename digest_struct variable to digest
- add _head suffix to tcg_efi_specid_event and tcg_pcr_event2
- rename digest_size member of tpm_bank_list to extend_size
- change type of alg_id member of tpm_bank_list from u8 to u16
- add missing semi-colon in pcrlock()

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

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

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

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

Roberto Sassu (5):
  tpm: dynamically allocate the allocated_banks array
  tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2
  tpm: rename and export tpm2_digest and tpm2_algorithms
  tpm: retrieve digest size of unknown algorithms with PCR read
  tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()

 drivers/char/tpm/eventlog/tpm2.c    |  12 +--
 drivers/char/tpm/tpm-chip.c         |   1 +
 drivers/char/tpm/tpm-interface.c    |  36 +++----
 drivers/char/tpm/tpm.h              |  22 ++--
 drivers/char/tpm/tpm1-cmd.c         |  25 ++++-
 drivers/char/tpm/tpm2-cmd.c         | 157 +++++++++++++++++++++-------
 include/linux/tpm.h                 |  43 +++++++-
 include/linux/tpm_eventlog.h        |  19 +---
 security/integrity/ima/ima_crypto.c |  10 +-
 security/integrity/ima/ima_queue.c  |   5 +-
 security/keys/trusted.c             |   5 +-
 11 files changed, 223 insertions(+), 112 deletions(-)

-- 
2.17.1


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

* [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
@ 2018-12-13 10:29 ` Roberto Sassu
  2018-12-20 14:55   ` Jarkko Sakkinen
  2018-12-13 10:29 ` [PATCH v7 2/5] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2 Roberto Sassu
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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

tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
the mask in the TPML_PCR_SELECTION structure returned by the TPM for
TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
case, the bank is not included in chip->allocated_banks, to avoid that TPM
driver users unnecessarily calculate a digest for that bank.

One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.

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

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

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


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

* [PATCH v7 2/5] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2
  2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
@ 2018-12-13 10:29 ` Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 3/5] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

TCG defines two structures, TCG_EfiSpecIDEventStruct and TCG_PCR_EVENT2,
which contain variable-sized arrays in the middle of the definition.

Since these structures are not suitable for type casting, this patch
removes structure members after the variable-sized arrays and adds the
_head suffix to the structure name, to indicate that the renamed structures
do not contain all fields defined by TCG.

Lastly, given that variable-sized arrays are now in the last position, and
given that the size of the arrays cannot be determined in advance, this
patch also sets the size of those arrays to zero and removes the definition
of TPM2_ACTIVE_PCR_BANKS.

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Nayna Jain <nayna@linux.ibm.com>
---
 drivers/char/tpm/eventlog/tpm2.c | 12 ++++++------
 include/linux/tpm_eventlog.h     | 12 ++++--------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c
index 1b8fa9de2cac..d8b77133a83a 100644
--- a/drivers/char/tpm/eventlog/tpm2.c
+++ b/drivers/char/tpm/eventlog/tpm2.c
@@ -37,10 +37,10 @@
  *
  * Returns size of the event. If it is an invalid event, returns 0.
  */
-static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
+static int calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 				struct tcg_pcr_event *event_header)
 {
-	struct tcg_efi_specid_event *efispecid;
+	struct tcg_efi_specid_event_head *efispecid;
 	struct tcg_event_field *event_field;
 	void *marker;
 	void *marker_start;
@@ -55,7 +55,7 @@ static int calc_tpm2_event_size(struct tcg_pcr_event2 *event,
 	marker = marker + sizeof(event->pcr_idx) + sizeof(event->event_type)
 		+ sizeof(event->count);
 
-	efispecid = (struct tcg_efi_specid_event *)event_header->event;
+	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
 
 	/* Check if event is malformed. */
 	if (event->count > efispecid->num_algs)
@@ -95,7 +95,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos)
 	void *addr = log->bios_event_log;
 	void *limit = log->bios_event_log_end;
 	struct tcg_pcr_event *event_header;
-	struct tcg_pcr_event2 *event;
+	struct tcg_pcr_event2_head *event;
 	size_t size;
 	int i;
 
@@ -136,7 +136,7 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
 					 loff_t *pos)
 {
 	struct tcg_pcr_event *event_header;
-	struct tcg_pcr_event2 *event;
+	struct tcg_pcr_event2_head *event;
 	struct tpm_chip *chip = m->private;
 	struct tpm_bios_log *log = &chip->log;
 	void *limit = log->bios_event_log_end;
@@ -180,7 +180,7 @@ static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v)
 	struct tpm_chip *chip = m->private;
 	struct tpm_bios_log *log = &chip->log;
 	struct tcg_pcr_event *event_header = log->bios_event_log;
-	struct tcg_pcr_event2 *event = v;
+	struct tcg_pcr_event2_head *event = v;
 	void *temp_ptr;
 	size_t size;
 
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 20d9da77fc11..f47342361e87 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
@@ -82,7 +81,7 @@ struct tcg_efi_specid_event_algs {
 	u16 digest_size;
 } __packed;
 
-struct tcg_efi_specid_event {
+struct tcg_efi_specid_event_head {
 	u8 signature[16];
 	u32 platform_class;
 	u8 spec_version_minor;
@@ -90,9 +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];
-	u8 vendor_info_size;
-	u8 vendor_info[0];
+	struct tcg_efi_specid_event_algs digest_sizes[];
 } __packed;
 
 struct tcg_pcr_event {
@@ -113,12 +110,11 @@ struct tpm2_digest {
 	u8 digest[SHA512_DIGEST_SIZE];
 } __packed;
 
-struct tcg_pcr_event2 {
+struct tcg_pcr_event2_head {
 	u32 pcr_idx;
 	u32 event_type;
 	u32 count;
-	struct tpm2_digest digests[TPM2_ACTIVE_PCR_BANKS];
-	struct tcg_event_field event;
+	struct tpm2_digest digests[];
 } __packed;
 
 #endif
-- 
2.17.1


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

* [PATCH v7 3/5] tpm: rename and export tpm2_digest and tpm2_algorithms
  2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 2/5] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2 Roberto Sassu
@ 2018-12-13 10:29 ` Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 4/5] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend() Roberto Sassu
  4 siblings, 0 replies; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  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/tpm1-cmd.c      |  2 +-
 drivers/char/tpm/tpm2-cmd.c      | 18 +++++++++---------
 include/linux/tpm.h              | 18 ++++++++++++++++++
 include/linux/tpm_eventlog.h     |  9 ++-------
 6 files changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 7b80919228be..96c8261d0f04 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -488,7 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
 int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
 {
 	int rc;
-	struct tpm2_digest *digest_list;
+	struct tpm_digest *digest_list;
 	int i;
 
 	chip = tpm_find_get_ops(chip);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 6b94306ab7c5..e961e5c5d197 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -122,17 +122,6 @@ enum tpm2_return_codes {
 	TPM2_RC_RETRY		= 0x0922,
 };
 
-enum tpm2_algorithms {
-	TPM2_ALG_ERROR		= 0x0000,
-	TPM2_ALG_SHA1		= 0x0004,
-	TPM2_ALG_KEYEDHASH	= 0x0008,
-	TPM2_ALG_SHA256		= 0x000B,
-	TPM2_ALG_SHA384		= 0x000C,
-	TPM2_ALG_SHA512		= 0x000D,
-	TPM2_ALG_NULL		= 0x0010,
-	TPM2_ALG_SM3_256	= 0x0012,
-};
-
 enum tpm2_command_codes {
 	TPM2_CC_FIRST		        = 0x011F,
 	TPM2_CC_HIERARCHY_CONTROL       = 0x0121,
@@ -561,7 +550,7 @@ static inline u32 tpm2_rc_value(u32 rc)
 int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm2_digest *digests);
+		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 			    unsigned int flags);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 0874743ca96d..ec4f197bae0a 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -716,7 +716,7 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	chip->allocated_banks[0] = TPM2_ALG_SHA1;
+	chip->allocated_banks[0] = TPM_ALG_SHA1;
 	chip->nr_allocated_banks = 1;
 
 	return rc;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 245669a7aba5..4afd14892cbf 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -33,11 +33,11 @@ struct tpm2_hash {
 };
 
 static struct tpm2_hash tpm2_hash_map[] = {
-	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
-	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
-	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
-	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
-	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+	{HASH_ALGO_SHA1, TPM_ALG_SHA1},
+	{HASH_ALGO_SHA256, TPM_ALG_SHA256},
+	{HASH_ALGO_SHA384, TPM_ALG_SHA384},
+	{HASH_ALGO_SHA512, TPM_ALG_SHA512},
+	{HASH_ALGO_SM3_256, TPM_ALG_SM3_256},
 };
 
 int tpm2_get_timeouts(struct tpm_chip *chip)
@@ -192,7 +192,7 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM2_ALG_SHA1);
+	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
 	tpm_buf_append_u8(&buf, TPM2_PCR_SELECT_MIN);
 	tpm_buf_append(&buf, (const unsigned char *)pcr_select,
 		       sizeof(pcr_select));
@@ -226,7 +226,7 @@ struct tpm2_null_auth_area {
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm2_digest *digests)
+		    struct tpm_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
@@ -449,7 +449,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 
 	/* public */
 	tpm_buf_append_u16(&buf, 14 + options->policydigest_len);
-	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
+	tpm_buf_append_u16(&buf, TPM_ALG_KEYEDHASH);
 	tpm_buf_append_u16(&buf, hash);
 
 	/* policy */
@@ -464,7 +464,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	}
 
 	/* public parameters */
-	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+	tpm_buf_append_u16(&buf, TPM_ALG_NULL);
 	tpm_buf_append_u16(&buf, 0);
 
 	/* outside info */
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index b49a55cf775f..330c0a481581 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -22,12 +22,30 @@
 #ifndef __LINUX_TPM_H__
 #define __LINUX_TPM_H__
 
+#include <crypto/hash_info.h>
+
 #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
 
 struct tpm_chip;
 struct trusted_key_payload;
 struct trusted_key_options;
 
+enum tpm_algorithms {
+	TPM_ALG_ERROR		= 0x0000,
+	TPM_ALG_SHA1		= 0x0004,
+	TPM_ALG_KEYEDHASH	= 0x0008,
+	TPM_ALG_SHA256		= 0x000B,
+	TPM_ALG_SHA384		= 0x000C,
+	TPM_ALG_SHA512		= 0x000D,
+	TPM_ALG_NULL		= 0x0010,
+	TPM_ALG_SM3_256		= 0x0012,
+};
+
+struct tpm_digest {
+	u16 alg_id;
+	u8 digest[SHA512_DIGEST_SIZE];
+} __packed;
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index f47342361e87..81519f163211 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 */
@@ -105,16 +105,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_head {
 	u32 pcr_idx;
 	u32 event_type;
 	u32 count;
-	struct tpm2_digest digests[];
+	struct tpm_digest digests[];
 } __packed;
 
 #endif
-- 
2.17.1


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

* [PATCH v7 4/5] tpm: retrieve digest size of unknown algorithms with PCR read
  2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (2 preceding siblings ...)
  2018-12-13 10:29 ` [PATCH v7 3/5] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
@ 2018-12-13 10:29 ` Roberto Sassu
  2018-12-13 10:29 ` [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend() Roberto Sassu
  4 siblings, 0 replies; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  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 modifies the definition of tpm_pcr_read() and tpm2_pcr_read() to
pass the desired hash algorithm and obtain the digest size at TPM startup.
Algorithms and corresponding digest sizes are stored in the new structure
tpm_bank_info, member of tpm_chip, so that the information can be used by
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.

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

For the initial PCR read, when digest sizes are not yet available, this
patch ensures that the amount of data copied from the output returned by
the TPM does not exceed the size of the array data are copied to.

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    | 16 +++---
 drivers/char/tpm/tpm.h              |  5 +-
 drivers/char/tpm/tpm1-cmd.c         |  4 +-
 drivers/char/tpm/tpm2-cmd.c         | 84 +++++++++++++++++++++++------
 include/linux/tpm.h                 | 12 ++++-
 security/integrity/ima/ima_crypto.c | 10 ++--
 6 files changed, 96 insertions(+), 35 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 96c8261d0f04..eb7c79ca8a94 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -451,11 +451,12 @@ EXPORT_SYMBOL_GPL(tpm_is_tpm2);
  * tpm_pcr_read - read a PCR value from SHA1 bank
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
  * @pcr_idx:	the PCR to be retrieved
- * @res_buf:	the value of the PCR
+ * @digest:	the PCR bank and buffer current PCR value is written to
  *
  * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
+int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+		 struct tpm_digest *digest)
 {
 	int rc;
 
@@ -464,9 +465,9 @@ int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2)
-		rc = tpm2_pcr_read(chip, pcr_idx, res_buf);
+		rc = tpm2_pcr_read(chip, pcr_idx, digest, NULL);
 	else
-		rc = tpm1_pcr_read(chip, pcr_idx, res_buf);
+		rc = tpm1_pcr_read(chip, pcr_idx, digest->digest);
 
 	tpm_put_ops(chip);
 	return rc;
@@ -479,9 +480,8 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  * @pcr_idx:	the PCR to be retrieved
  * @hash:	the hash value used to extend the PCR value
  *
- * Note: with TPM 2.0 extends also those banks with a known digest size to the
- * cryto subsystem in order to prevent malicious use of those PCR banks. In the
- * future we should dynamically determine digest sizes.
+ * Note: with TPM 2.0 extends also those banks for which no digest was
+ * specified in order to prevent malicious use of those PCR banks.
  *
  * Return: same as with tpm_transmit_cmd()
  */
@@ -502,7 +502,7 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
 			return -ENOMEM;
 
 		for (i = 0; i < chip->nr_allocated_banks; i++) {
-			digest_list[i].alg_id = chip->allocated_banks[i];
+			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
 			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
 		}
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index e961e5c5d197..64d93d26087f 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -247,7 +247,7 @@ struct tpm_chip {
 	unsigned int groups_cnt;
 
 	u32 nr_allocated_banks;
-	u16 *allocated_banks;
+	struct tpm_bank_info *allocated_banks;
 #ifdef CONFIG_ACPI
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
@@ -548,7 +548,8 @@ static inline u32 tpm2_rc_value(u32 rc)
 }
 
 int tpm2_get_timeouts(struct tpm_chip *chip);
-int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
+int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+		  struct tpm_digest *digest, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 		    struct tpm_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index ec4f197bae0a..8b70a7f884a7 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -716,7 +716,9 @@ int tpm1_auto_startup(struct tpm_chip *chip)
 		goto out;
 	}
 
-	chip->allocated_banks[0] = TPM_ALG_SHA1;
+	chip->allocated_banks[0].alg_id = TPM_ALG_SHA1;
+	chip->allocated_banks[0].digest_size = hash_digest_size[HASH_ALGO_SHA1];
+	chip->allocated_banks[0].crypto_id = HASH_ALGO_SHA1;
 	chip->nr_allocated_banks = 1;
 
 	return rc;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 4afd14892cbf..6ce5173cf0e5 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -171,20 +171,36 @@ 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:	PCR bank and buffer current PCR value is written to.
+ * @digest_size_ptr:	pointer to variable that stores the digest size.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
-int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
+int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+		  struct tpm_digest *digest, u16 *digest_size_ptr)
 {
+	int i;
 	int rc;
 	struct tpm_buf buf;
 	struct tpm2_pcr_read_out *out;
 	u8 pcr_select[TPM2_PCR_SELECT_MIN] = {0};
+	u16 digest_size;
+	u16 expected_digest_size = 0;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
+	if (!digest_size_ptr) {
+		for (i = 0; i < chip->nr_allocated_banks &&
+		     chip->allocated_banks[i].alg_id != digest->alg_id; i++)
+			;
+
+		if (i == chip->nr_allocated_banks)
+			return -EINVAL;
+
+		expected_digest_size = chip->allocated_banks[i].digest_size;
+	}
+
 	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
 	if (rc)
 		return rc;
@@ -192,18 +208,29 @@ int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
 	pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
 	tpm_buf_append_u32(&buf, 1);
-	tpm_buf_append_u16(&buf, TPM_ALG_SHA1);
+	tpm_buf_append_u16(&buf, digest->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->digest) ||
+	    (!digest_size_ptr && digest_size != expected_digest_size)) {
+		rc = -EINVAL;
+		goto out;
 	}
 
+	if (digest_size_ptr)
+		*digest_size_ptr = digest_size;
+
+	memcpy(digest->digest, out->digest, digest_size);
+out:
 	tpm_buf_destroy(&buf);
 	return rc;
 }
@@ -232,7 +259,6 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	struct tpm2_null_auth_area auth_area;
 	int rc;
 	int i;
-	int j;
 
 	if (count > chip->nr_allocated_banks)
 		return -EINVAL;
@@ -254,14 +280,9 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, count);
 
 	for (i = 0; i < count; i++) {
-		for (j = 0; j < ARRAY_SIZE(tpm2_hash_map); j++) {
-			if (digests[i].alg_id != tpm2_hash_map[j].tpm_id)
-				continue;
-			tpm_buf_append_u16(&buf, digests[i].alg_id);
-			tpm_buf_append(&buf, (const unsigned char
-					      *)&digests[i].digest,
-			       hash_digest_size[tpm2_hash_map[j].crypto_id]);
-		}
+		tpm_buf_append_u16(&buf, digests[i].alg_id);
+		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+			       chip->allocated_banks[i].digest_size);
 	}
 
 	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE, 0, 0,
@@ -812,6 +833,30 @@ int tpm2_probe(struct tpm_chip *chip)
 }
 EXPORT_SYMBOL_GPL(tpm2_probe);
 
+static int tpm2_init_bank_info(struct tpm_chip *chip, u32 bank_index)
+{
+	struct tpm_bank_info *bank = chip->allocated_banks + bank_index;
+	struct tpm_digest digest = { .alg_id = bank->alg_id };
+	int i;
+
+	/*
+	 * Avoid unnecessary PCR read operations to reduce overhead
+	 * and obtain identifiers of the crypto subsystem.
+	 */
+	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;
@@ -879,7 +924,12 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
 				break;
 
 		if (j < pcr_selection.size_of_select) {
-			chip->allocated_banks[nr_alloc_banks] = hash_alg;
+			chip->allocated_banks[nr_alloc_banks].alg_id = hash_alg;
+
+			rc = tpm2_init_bank_info(chip, nr_alloc_banks);
+			if (rc < 0)
+				break;
+
 			nr_alloc_banks++;
 		}
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 330c0a481581..72df8d4252ef 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),
 };
@@ -71,7 +77,8 @@ struct tpm_class_ops {
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
 extern int tpm_is_tpm2(struct tpm_chip *chip);
-extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
+extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
+			struct tpm_digest *digest);
 extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
@@ -88,7 +95,8 @@ static inline int tpm_is_tpm2(struct tpm_chip *chip)
 	return -ENODEV;
 }
 
-static inline int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf)
+static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
+			       struct tpm_digest *digest)
 {
 	return -ENODEV;
 }
diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index acf2c7df7145..16a4f45863b1 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -643,12 +643,12 @@ int ima_calc_buffer_hash(const void *buf, loff_t len,
 	return calc_buffer_shash(buf, len, hash);
 }
 
-static void __init ima_pcrread(u32 idx, u8 *pcr)
+static void __init ima_pcrread(u32 idx, struct tpm_digest *d)
 {
 	if (!ima_tpm_chip)
 		return;
 
-	if (tpm_pcr_read(ima_tpm_chip, idx, pcr) != 0)
+	if (tpm_pcr_read(ima_tpm_chip, idx, d) != 0)
 		pr_err("Error Communicating to TPM chip\n");
 }
 
@@ -658,7 +658,7 @@ static void __init ima_pcrread(u32 idx, u8 *pcr)
 static int __init ima_calc_boot_aggregate_tfm(char *digest,
 					      struct crypto_shash *tfm)
 {
-	u8 pcr_i[TPM_DIGEST_SIZE];
+	struct tpm_digest d = { .alg_id = TPM_ALG_SHA1, .digest = {0} };
 	int rc;
 	u32 i;
 	SHASH_DESC_ON_STACK(shash, tfm);
@@ -672,9 +672,9 @@ static int __init ima_calc_boot_aggregate_tfm(char *digest,
 
 	/* cumulative sha1 over tpm registers 0-7 */
 	for (i = TPM_PCR0; i < TPM_PCR8; i++) {
-		ima_pcrread(i, pcr_i);
+		ima_pcrread(i, &d);
 		/* now accumulate with current aggregate */
-		rc = crypto_shash_update(shash, pcr_i, TPM_DIGEST_SIZE);
+		rc = crypto_shash_update(shash, d.digest, TPM_DIGEST_SIZE);
 	}
 	if (!rc)
 		crypto_shash_final(shash, digest);
-- 
2.17.1


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

* [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
                   ` (3 preceding siblings ...)
  2018-12-13 10:29 ` [PATCH v7 4/5] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
@ 2018-12-13 10:29 ` Roberto Sassu
  2018-12-20 15:21   ` Jarkko Sakkinen
  4 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2018-12-13 10:29 UTC (permalink / raw)
  To: jarkko.sakkinen, zohar, david.safford, monty.wiseman
  Cc: linux-integrity, linux-security-module, linux-kernel,
	silviu.vlasceanu, Roberto Sassu

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

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

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

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

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

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

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index eb7c79ca8a94..911fea19e408 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
  * tpm_pcr_extend - extend a PCR value in SHA1 bank.
  * @chip:	a &struct tpm_chip instance, %NULL for the default chip
  * @pcr_idx:	the PCR to be retrieved
- * @hash:	the hash value used to extend the PCR value
+ * @count:	number of tpm_extend_digest structures
+ * @digests:	array of tpm_extend_digest structures used to extend PCRs
  *
  * Note: with TPM 2.0 extends also those banks for which no digest was
  * specified in order to prevent malicious use of those PCR banks.
  *
  * Return: same as with tpm_transmit_cmd()
  */
-int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
+int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		   const struct tpm_extend_digest *digests)
 {
 	int rc;
-	struct tpm_digest *digest_list;
-	int i;
 
 	chip = tpm_find_get_ops(chip);
 	if (!chip)
 		return -ENODEV;
 
 	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-		digest_list = kcalloc(chip->nr_allocated_banks,
-				      sizeof(*digest_list), GFP_KERNEL);
-		if (!digest_list)
-			return -ENOMEM;
-
-		for (i = 0; i < chip->nr_allocated_banks; i++) {
-			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
-			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
-		}
-
-		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
-				     digest_list);
-		kfree(digest_list);
+		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
 		tpm_put_ops(chip);
 		return rc;
 	}
 
-	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
+	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
 			     "attempting extend a PCR value");
 	tpm_put_ops(chip);
 	return rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 64d93d26087f..6b446504d2fe 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
 int tpm1_do_selftest(struct tpm_chip *chip);
 int tpm1_get_timeouts(struct tpm_chip *chip);
 unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
-int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
+int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		    const struct tpm_extend_digest *digests,
 		    const char *log_msg);
 int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
 ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
@@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
 int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 		  struct tpm_digest *digest, u16 *digest_size_ptr);
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm_digest *digests);
+		    const struct tpm_extend_digest *digests);
 int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
 void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 			    unsigned int flags);
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index 8b70a7f884a7..04ee10284b8c 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
 }
 
 #define TPM_ORD_PCR_EXTEND 20
-int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
+int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+		    const struct tpm_extend_digest *digests,
 		    const char *log_msg)
 {
 	struct tpm_buf buf;
+	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
+	const u8 *hash;
 	int rc;
 
+	hash = dummy_hash;
+	if (count)
+		memcpy(dummy_hash, digests[0].data,
+		       min(digests[0].size, (u16)sizeof(dummy_hash)));
+
 	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
 	if (rc)
 		return rc;
@@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
  */
 int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 {
-	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
 	struct tpm_buf buf;
 	unsigned int try;
 	int rc;
@@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
 
 	/* for buggy tpm, flush pcrs with extend to selected dummy */
 	if (tpm_suspend_pcr)
-		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
+		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
 				     "extending dummy pcr before suspend");
 
 	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 6ce5173cf0e5..77b5808270c6 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
  *
  * @chip:	TPM chip to use.
  * @pcr_idx:	index of the PCR.
- * @count:	number of digests passed.
- * @digests:	list of pcr banks and corresponding digest values to extend.
+ * @count:	number of tpm_extend_digest passed.
+ * @digests:	array of tpm_extend_digest with digest values to extend.
  *
  * Return: Same as with tpm_transmit_cmd.
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
-		    struct tpm_digest *digests)
+		    const struct tpm_extend_digest *digests)
 {
 	struct tpm_buf buf;
 	struct tpm2_null_auth_area auth_area;
+	const struct tpm_extend_digest *digest;
+	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
+	const u8 *hash;
 	int rc;
 	int i;
-
-	if (count > chip->nr_allocated_banks)
-		return -EINVAL;
+	int j;
 
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
 	if (rc)
@@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
 	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
 	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
 		       sizeof(auth_area));
-	tpm_buf_append_u32(&buf, count);
+	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
+
+	if (count)
+		memcpy(dummy_hash, digests[0].data, digests[0].size);
+
+	for (i = 0; i < chip->nr_allocated_banks; i++) {
+		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
+
+		hash = dummy_hash;
+		for (j = 0; j < count; j++) {
+			digest = digests + j;
+
+			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
+				hash = digest->data;
+				break;
+			}
+		}
 
-	for (i = 0; i < count; i++) {
-		tpm_buf_append_u16(&buf, digests[i].alg_id);
-		tpm_buf_append(&buf, (const unsigned char *)&digests[i].digest,
+		tpm_buf_append(&buf, hash,
 			       chip->allocated_banks[i].digest_size);
 	}
 
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 72df8d4252ef..f865bfdc39dc 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -52,6 +52,12 @@ struct tpm_bank_info {
 	u16 crypto_id;
 };
 
+struct tpm_extend_digest {
+	u16 alg_id;
+	u16 size;
+	const u8 *data;
+};
+
 enum TPM_OPS_FLAGS {
 	TPM_OPS_AUTO_STARTUP = BIT(0),
 };
@@ -79,7 +85,8 @@ struct tpm_class_ops {
 extern int tpm_is_tpm2(struct tpm_chip *chip);
 extern int tpm_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
 			struct tpm_digest *digest);
-extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash);
+extern int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+			  const struct tpm_extend_digest *digests);
 extern int tpm_send(struct tpm_chip *chip, void *cmd, size_t buflen);
 extern int tpm_get_random(struct tpm_chip *chip, u8 *data, size_t max);
 extern int tpm_seal_trusted(struct tpm_chip *chip,
@@ -101,8 +108,8 @@ static inline int tpm_pcr_read(struct tpm_chip *chip, int pcr_idx,
 	return -ENODEV;
 }
 
-static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx,
-				 const u8 *hash)
+static inline int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
+				 const struct tpm_extend_digest *digests)
 {
 	return -ENODEV;
 }
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index b186819bd5aa..183733cfb5d2 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -140,12 +140,15 @@ unsigned long ima_get_binary_runtime_size(void)
 
 static int ima_pcr_extend(const u8 *hash, int pcr)
 {
+	struct tpm_extend_digest digest = { .alg_id = TPM_ALG_SHA1,
+					    .size = TPM_DIGEST_SIZE,
+					    .data = hash };
 	int result = 0;
 
 	if (!ima_tpm_chip)
 		return result;
 
-	result = tpm_pcr_extend(ima_tpm_chip, pcr, hash);
+	result = tpm_pcr_extend(ima_tpm_chip, pcr, 1, &digest);
 	if (result != 0)
 		pr_err("Error Communicating to TPM chip, result: %d\n", result);
 	return result;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ff6789365a12..208a64a3fe1f 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -380,6 +380,9 @@ EXPORT_SYMBOL_GPL(trusted_tpm_send);
 static int pcrlock(const int pcrnum)
 {
 	unsigned char hash[SHA1_DIGEST_SIZE];
+	struct tpm_extend_digest digest = { .alg_id = TPM_ALG_SHA1,
+					    .size = sizeof(hash),
+					    .data = hash };
 	int ret;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -387,7 +390,7 @@ static int pcrlock(const int pcrnum)
 	ret = tpm_get_random(NULL, hash, SHA1_DIGEST_SIZE);
 	if (ret != SHA1_DIGEST_SIZE)
 		return ret;
-	return tpm_pcr_extend(NULL, pcrnum, hash) ? -EINVAL : 0;
+	return tpm_pcr_extend(NULL, pcrnum, 1, &digest) ? -EINVAL : 0;
 }
 
 /*
-- 
2.17.1


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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
@ 2018-12-20 14:55   ` Jarkko Sakkinen
  2018-12-21  9:40     ` Roberto Sassu
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2018-12-20 14:55 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
> This patch renames active_banks (member of tpm_chip) to allocated_banks,
> stores the number of allocated PCR banks in nr_allocated_banks (new member
> of tpm_chip), and replaces the static array with a pointer to a dynamically
> allocated array.
> 
> tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
> the mask in the TPML_PCR_SELECTION structure returned by the TPM for
> TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
> bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
> case, the bank is not included in chip->allocated_banks, to avoid that TPM
> driver users unnecessarily calculate a digest for that bank.
> 
> One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
> 
> As a consequence of the introduction of nr_allocated_banks,
> tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
> is equal to zero.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-chip.c      |  1 +
>  drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
>  drivers/char/tpm/tpm.h           |  3 ++-
>  drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
>  drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
>  5 files changed, 47 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 32db84683c40..ce851c62bb68 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>  	kfree(chip->log.bios_event_log);
>  	kfree(chip->work_space.context_buf);
>  	kfree(chip->work_space.session_buf);
> +	kfree(chip->allocated_banks);
>  	kfree(chip);
>  }
>  
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index d9439f9abe78..7b80919228be 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>  int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>  {
>  	int rc;
> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> -	u32 count = 0;
> +	struct tpm2_digest *digest_list;
>  	int i;
>  
>  	chip = tpm_find_get_ops(chip);
> @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>  		return -ENODEV;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		memset(digest_list, 0, sizeof(digest_list));
> +		digest_list = kcalloc(chip->nr_allocated_banks,
> +				      sizeof(*digest_list), GFP_KERNEL);
> +		if (!digest_list)
> +			return -ENOMEM;

You could preallocate digest list and place it to struct tpm_chip
instead of doing it everytime tpm_pcr_extend() called.

>  
> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
> -			digest_list[i].alg_id = chip->active_banks[i];
> +		for (i = 0; i < chip->nr_allocated_banks; i++) {
> +			digest_list[i].alg_id = chip->allocated_banks[i];
>  			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -			count++;
>  		}
>  
> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
> +				     digest_list);
> +		kfree(digest_list);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f27d1f38a93d..6b94306ab7c5 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -257,7 +257,8 @@ struct tpm_chip {
>  	const struct attribute_group *groups[3];
>  	unsigned int groups_cnt;
>  
> -	u16 active_banks[7];
> +	u32 nr_allocated_banks;
> +	u16 *allocated_banks;
>  #ifdef CONFIG_ACPI
>  	acpi_handle acpi_dev_handle;
>  	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index 6f306338953b..0874743ca96d 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -709,6 +709,16 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>  		goto out;
>  	}
>  
> +	chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
> +					GFP_KERNEL);
> +	if (!chip->allocated_banks) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	chip->allocated_banks[0] = TPM2_ALG_SHA1;
> +	chip->nr_allocated_banks = 1;
> +
>  	return rc;
>  out:
>  	if (rc > 0)
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index a6bec13afa69..245669a7aba5 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -234,7 +234,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>  	int i;
>  	int j;
>  
> -	if (count > ARRAY_SIZE(chip->active_banks))
> +	if (count > chip->nr_allocated_banks)
>  		return -EINVAL;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> @@ -825,11 +825,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	void *marker;
>  	void *end;
>  	void *pcr_select_offset;
> -	unsigned int count;
>  	u32 sizeof_pcr_selection;
> +	u32 nr_possible_banks;
> +	u32 nr_alloc_banks = 0;
> +	u16 hash_alg;
>  	u32 rsp_len;
>  	int rc;
>  	int i = 0;
> +	int j;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
>  	if (rc)
> @@ -844,11 +847,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	if (rc)
>  		goto out;
>  
> -	count = be32_to_cpup(
> +	nr_possible_banks = be32_to_cpup(
>  		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>  
> -	if (count > ARRAY_SIZE(chip->active_banks)) {
> -		rc = -ENODEV;
> +	chip->allocated_banks = kcalloc(nr_possible_banks,
> +					sizeof(*chip->allocated_banks),
> +					GFP_KERNEL);
> +	if (!chip->allocated_banks) {
> +		rc = -ENOMEM;
>  		goto out;
>  	}
>  
> @@ -857,7 +863,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>  	end = &buf.data[rsp_len];
>  
> -	for (i = 0; i < count; i++) {
> +	for (i = 0; i < nr_possible_banks; i++) {
>  		pcr_select_offset = marker +
>  			offsetof(struct tpm2_pcr_selection, size_of_select);
>  		if (pcr_select_offset >= end) {
> @@ -866,17 +872,25 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>  		}
>  
>  		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
> -		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
> +		hash_alg = be16_to_cpu(pcr_selection.hash_alg);
> +
> +		for (j = 0; j < pcr_selection.size_of_select; j++)
> +			if (pcr_selection.pcr_select[j])
> +				break;
> +
> +		if (j < pcr_selection.size_of_select) {
> +			chip->allocated_banks[nr_alloc_banks] = hash_alg;
> +			nr_alloc_banks++;
> +		}

The same comment as before: you should use memchr_inv() here.

> +
>  		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
>  			sizeof(pcr_selection.size_of_select) +
>  			pcr_selection.size_of_select;
>  		marker = marker + sizeof_pcr_selection;
>  	}
>  
> +	chip->nr_allocated_banks = nr_alloc_banks;
>  out:
> -	if (i < ARRAY_SIZE(chip->active_banks))
> -		chip->active_banks[i] = TPM2_ALG_ERROR;
> -
>  	tpm_buf_destroy(&buf);
>  
>  	return rc;
> -- 
> 2.17.1
> 

/Jarkko

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2018-12-13 10:29 ` [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend() Roberto Sassu
@ 2018-12-20 15:21   ` Jarkko Sakkinen
  2019-01-17  7:59     ` Roberto Sassu
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2018-12-20 15:21 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> 
> This patch modifies the definition of tpm_pcr_extend() to allow other
> kernel subsystems to pass a digest for each algorithm supported by the TPM.
> All digests are processed by the TPM in one operation.
> 
> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> the TPM driver extends the remaining PCR banks with the first digest
> passed as an argument to the function.
> 
> The new tpm_extend digest structure has been preferred to the tpm_digest
> structure, to let the caller specify the size of the digest (which may be
> unknown to the TPM driver).
> 
> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>  drivers/char/tpm/tpm.h             |  5 +++--
>  drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
>  drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
>  include/linux/tpm.h                | 13 ++++++++---
>  security/integrity/ima/ima_queue.c |  5 ++++-
>  security/keys/trusted.c            |  5 ++++-
>  7 files changed, 62 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index eb7c79ca8a94..911fea19e408 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>   * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>   * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>   * @pcr_idx:	the PCR to be retrieved
> - * @hash:	the hash value used to extend the PCR value
> + * @count:	number of tpm_extend_digest structures
> + * @digests:	array of tpm_extend_digest structures used to extend PCRs
>   *
>   * Note: with TPM 2.0 extends also those banks for which no digest was
>   * specified in order to prevent malicious use of those PCR banks.
>   *
>   * Return: same as with tpm_transmit_cmd()
>   */
> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		   const struct tpm_extend_digest *digests)

Remove const. Document how @digests is used  like the special meaning
of the first index. I faintly remember asking this last time.

>  {
>  	int rc;
> -	struct tpm_digest *digest_list;
> -	int i;
>  
>  	chip = tpm_find_get_ops(chip);
>  	if (!chip)
>  		return -ENODEV;
>  
>  	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> -		digest_list = kcalloc(chip->nr_allocated_banks,
> -				      sizeof(*digest_list), GFP_KERNEL);
> -		if (!digest_list)
> -			return -ENOMEM;
> -
> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> -		}
> -
> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
> -				     digest_list);
> -		kfree(digest_list);
> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
>  		tpm_put_ops(chip);
>  		return rc;
>  	}
>  
> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
>  			     "attempting extend a PCR value");

The validation is missing that the provided array has only one element
and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
but what you are doing to that function does not make any sense so
better to do it here.

>  	tpm_put_ops(chip);
>  	return rc;
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 64d93d26087f..6b446504d2fe 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>  int tpm1_do_selftest(struct tpm_chip *chip);
>  int tpm1_get_timeouts(struct tpm_chip *chip);
>  unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		    const struct tpm_extend_digest *digests,
>  		    const char *log_msg);
>  int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>  ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>  int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>  		  struct tpm_digest *digest, u16 *digest_size_ptr);
>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> -		    struct tpm_digest *digests);
> +		    const struct tpm_extend_digest *digests);
>  int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>  void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>  			    unsigned int flags);
> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> index 8b70a7f884a7..04ee10284b8c 100644
> --- a/drivers/char/tpm/tpm1-cmd.c
> +++ b/drivers/char/tpm/tpm1-cmd.c
> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>  }
>  
>  #define TPM_ORD_PCR_EXTEND 20
> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> +		    const struct tpm_extend_digest *digests,
>  		    const char *log_msg)
>  {
>  	struct tpm_buf buf;
> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> +	const u8 *hash;
>  	int rc;
>  
> +	hash = dummy_hash;
> +	if (count)
> +		memcpy(dummy_hash, digests[0].data,
> +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
> +

You copy memory from one place to another without any good reason to do
so. My suggestion is just not to change tpm1_pcr_extend() at all.

>  	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
>  	if (rc)
>  		return rc;
> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>   */
>  int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>  {
> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>  	struct tpm_buf buf;
>  	unsigned int try;
>  	int rc;
> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>  
>  	/* for buggy tpm, flush pcrs with extend to selected dummy */
>  	if (tpm_suspend_pcr)
> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>  				     "extending dummy pcr before suspend");
>  
>  	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 6ce5173cf0e5..77b5808270c6 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
>   *
>   * @chip:	TPM chip to use.
>   * @pcr_idx:	index of the PCR.
> - * @count:	number of digests passed.
> - * @digests:	list of pcr banks and corresponding digest values to extend.
> + * @count:	number of tpm_extend_digest passed.
> + * @digests:	array of tpm_extend_digest with digest values to extend.
>   *
>   * Return: Same as with tpm_transmit_cmd.
>   */

The documentation about @digests.

>  int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> -		    struct tpm_digest *digests)
> +		    const struct tpm_extend_digest *digests)
>  {
>  	struct tpm_buf buf;
>  	struct tpm2_null_auth_area auth_area;
> +	const struct tpm_extend_digest *digest;
> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
> +	const u8 *hash;
>  	int rc;
>  	int i;
> -
> -	if (count > chip->nr_allocated_banks)
> -		return -EINVAL;
> +	int j;
>  
>  	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>  	if (rc)
> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>  	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>  	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>  		       sizeof(auth_area));
> -	tpm_buf_append_u32(&buf, count);
> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> +
> +	if (count)
> +		memcpy(dummy_hash, digests[0].data, digests[0].size);
> +
> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
> +
> +		hash = dummy_hash;
> +		for (j = 0; j < count; j++) {
> +			digest = digests + j;
> +
> +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
> +				hash = digest->data;
> +				break;

I think the whole design is just wrong. I did re-read your response to
v6 again and I'm very sorry, but I just don't get this. Caller has all
the information (from struct tpm_chip) to give the correct data. This
function should validate that data (check algorithm ID and that's it).

Extending with the dummy hash should be done by the caller when
preparing the array, not baked into this function. This kind of also
makes obvious that we don't need this new struct. There should never be
a local variable (whose size is BTW randomly chosen) called dummy_hash.

/Jarkko

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2018-12-20 14:55   ` Jarkko Sakkinen
@ 2018-12-21  9:40     ` Roberto Sassu
  2018-12-22  0:03       ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2018-12-21  9:40 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
> On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
>> This patch renames active_banks (member of tpm_chip) to allocated_banks,
>> stores the number of allocated PCR banks in nr_allocated_banks (new member
>> of tpm_chip), and replaces the static array with a pointer to a dynamically
>> allocated array.
>>
>> tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
>> the mask in the TPML_PCR_SELECTION structure returned by the TPM for
>> TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
>> bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
>> case, the bank is not included in chip->allocated_banks, to avoid that TPM
>> driver users unnecessarily calculate a digest for that bank.
>>
>> One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
>>
>> As a consequence of the introduction of nr_allocated_banks,
>> tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
>> is equal to zero.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> ---
>>   drivers/char/tpm/tpm-chip.c      |  1 +
>>   drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
>>   drivers/char/tpm/tpm.h           |  3 ++-
>>   drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
>>   drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
>>   5 files changed, 47 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>> index 32db84683c40..ce851c62bb68 100644
>> --- a/drivers/char/tpm/tpm-chip.c
>> +++ b/drivers/char/tpm/tpm-chip.c
>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>   	kfree(chip->log.bios_event_log);
>>   	kfree(chip->work_space.context_buf);
>>   	kfree(chip->work_space.session_buf);
>> +	kfree(chip->allocated_banks);
>>   	kfree(chip);
>>   }
>>   
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index d9439f9abe78..7b80919228be 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>   int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>   {
>>   	int rc;
>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>> -	u32 count = 0;
>> +	struct tpm2_digest *digest_list;
>>   	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>> @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>   		return -ENODEV;
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		memset(digest_list, 0, sizeof(digest_list));
>> +		digest_list = kcalloc(chip->nr_allocated_banks,
>> +				      sizeof(*digest_list), GFP_KERNEL);
>> +		if (!digest_list)
>> +			return -ENOMEM;
> 
> You could preallocate digest list and place it to struct tpm_chip
> instead of doing it everytime tpm_pcr_extend() called.

This part will be removed with patch 5/5.


>> -		for (i = 0; i < ARRAY_SIZE(chip->active_banks) &&
>> -			    chip->active_banks[i] != TPM2_ALG_ERROR; i++) {
>> -			digest_list[i].alg_id = chip->active_banks[i];
>> +		for (i = 0; i < chip->nr_allocated_banks; i++) {
>> +			digest_list[i].alg_id = chip->allocated_banks[i];
>>   			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -			count++;
>>   		}
>>   
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, count, digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>> +				     digest_list);
>> +		kfree(digest_list);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index f27d1f38a93d..6b94306ab7c5 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -257,7 +257,8 @@ struct tpm_chip {
>>   	const struct attribute_group *groups[3];
>>   	unsigned int groups_cnt;
>>   
>> -	u16 active_banks[7];
>> +	u32 nr_allocated_banks;
>> +	u16 *allocated_banks;
>>   #ifdef CONFIG_ACPI
>>   	acpi_handle acpi_dev_handle;
>>   	char ppi_version[TPM_PPI_VERSION_LEN + 1];
>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>> index 6f306338953b..0874743ca96d 100644
>> --- a/drivers/char/tpm/tpm1-cmd.c
>> +++ b/drivers/char/tpm/tpm1-cmd.c
>> @@ -709,6 +709,16 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>   		goto out;
>>   	}
>>   
>> +	chip->allocated_banks = kcalloc(1, sizeof(*chip->allocated_banks),
>> +					GFP_KERNEL);
>> +	if (!chip->allocated_banks) {
>> +		rc = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>> +	chip->allocated_banks[0] = TPM2_ALG_SHA1;
>> +	chip->nr_allocated_banks = 1;
>> +
>>   	return rc;
>>   out:
>>   	if (rc > 0)
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index a6bec13afa69..245669a7aba5 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -234,7 +234,7 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>   	int i;
>>   	int j;
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks))
>> +	if (count > chip->nr_allocated_banks)
>>   		return -EINVAL;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>> @@ -825,11 +825,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	void *marker;
>>   	void *end;
>>   	void *pcr_select_offset;
>> -	unsigned int count;
>>   	u32 sizeof_pcr_selection;
>> +	u32 nr_possible_banks;
>> +	u32 nr_alloc_banks = 0;
>> +	u16 hash_alg;
>>   	u32 rsp_len;
>>   	int rc;
>>   	int i = 0;
>> +	int j;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
>>   	if (rc)
>> @@ -844,11 +847,14 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	if (rc)
>>   		goto out;
>>   
>> -	count = be32_to_cpup(
>> +	nr_possible_banks = be32_to_cpup(
>>   		(__be32 *)&buf.data[TPM_HEADER_SIZE + 5]);
>>   
>> -	if (count > ARRAY_SIZE(chip->active_banks)) {
>> -		rc = -ENODEV;
>> +	chip->allocated_banks = kcalloc(nr_possible_banks,
>> +					sizeof(*chip->allocated_banks),
>> +					GFP_KERNEL);
>> +	if (!chip->allocated_banks) {
>> +		rc = -ENOMEM;
>>   		goto out;
>>   	}
>>   
>> @@ -857,7 +863,7 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   	rsp_len = be32_to_cpup((__be32 *)&buf.data[2]);
>>   	end = &buf.data[rsp_len];
>>   
>> -	for (i = 0; i < count; i++) {
>> +	for (i = 0; i < nr_possible_banks; i++) {
>>   		pcr_select_offset = marker +
>>   			offsetof(struct tpm2_pcr_selection, size_of_select);
>>   		if (pcr_select_offset >= end) {
>> @@ -866,17 +872,25 @@ static ssize_t tpm2_get_pcr_allocation(struct tpm_chip *chip)
>>   		}
>>   
>>   		memcpy(&pcr_selection, marker, sizeof(pcr_selection));
>> -		chip->active_banks[i] = be16_to_cpu(pcr_selection.hash_alg);
>> +		hash_alg = be16_to_cpu(pcr_selection.hash_alg);
>> +
>> +		for (j = 0; j < pcr_selection.size_of_select; j++)
>> +			if (pcr_selection.pcr_select[j])
>> +				break;
>> +
>> +		if (j < pcr_selection.size_of_select) {
>> +			chip->allocated_banks[nr_alloc_banks] = hash_alg;
>> +			nr_alloc_banks++;
>> +		}
> 
> The same comment as before: you should use memchr_inv() here.

Ok.

Roberto


>> +
>>   		sizeof_pcr_selection = sizeof(pcr_selection.hash_alg) +
>>   			sizeof(pcr_selection.size_of_select) +
>>   			pcr_selection.size_of_select;
>>   		marker = marker + sizeof_pcr_selection;
>>   	}
>>   
>> +	chip->nr_allocated_banks = nr_alloc_banks;
>>   out:
>> -	if (i < ARRAY_SIZE(chip->active_banks))
>> -		chip->active_banks[i] = TPM2_ALG_ERROR;
>> -
>>   	tpm_buf_destroy(&buf);
>>   
>>   	return rc;
>> -- 
>> 2.17.1
>>
> 
> /Jarkko
> 

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

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2018-12-21  9:40     ` Roberto Sassu
@ 2018-12-22  0:03       ` Jarkko Sakkinen
  2019-01-07 10:06         ` Roberto Sassu
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2018-12-22  0:03 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Fri, Dec 21, 2018 at 10:40:09AM +0100, Roberto Sassu wrote:
> On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
> > On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
> > > This patch renames active_banks (member of tpm_chip) to allocated_banks,
> > > stores the number of allocated PCR banks in nr_allocated_banks (new member
> > > of tpm_chip), and replaces the static array with a pointer to a dynamically
> > > allocated array.
> > > 
> > > tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
> > > the mask in the TPML_PCR_SELECTION structure returned by the TPM for
> > > TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
> > > bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
> > > case, the bank is not included in chip->allocated_banks, to avoid that TPM
> > > driver users unnecessarily calculate a digest for that bank.
> > > 
> > > One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
> > > 
> > > As a consequence of the introduction of nr_allocated_banks,
> > > tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
> > > is equal to zero.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >   drivers/char/tpm/tpm-chip.c      |  1 +
> > >   drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
> > >   drivers/char/tpm/tpm.h           |  3 ++-
> > >   drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
> > >   drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
> > >   5 files changed, 47 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > index 32db84683c40..ce851c62bb68 100644
> > > --- a/drivers/char/tpm/tpm-chip.c
> > > +++ b/drivers/char/tpm/tpm-chip.c
> > > @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
> > >   	kfree(chip->log.bios_event_log);
> > >   	kfree(chip->work_space.context_buf);
> > >   	kfree(chip->work_space.session_buf);
> > > +	kfree(chip->allocated_banks);
> > >   	kfree(chip);
> > >   }
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index d9439f9abe78..7b80919228be 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> > >   int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > >   {
> > >   	int rc;
> > > -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> > > -	u32 count = 0;
> > > +	struct tpm2_digest *digest_list;
> > >   	int i;
> > >   	chip = tpm_find_get_ops(chip);
> > > @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > >   		return -ENODEV;
> > >   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > -		memset(digest_list, 0, sizeof(digest_list));
> > > +		digest_list = kcalloc(chip->nr_allocated_banks,
> > > +				      sizeof(*digest_list), GFP_KERNEL);
> > > +		if (!digest_list)
> > > +			return -ENOMEM;
> > 
> > You could preallocate digest list and place it to struct tpm_chip
> > instead of doing it everytime tpm_pcr_extend() called.
> 
> This part will be removed with patch 5/5.

Even if it did, it does not make this patch unbroken.

/Jarkko

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2018-12-22  0:03       ` Jarkko Sakkinen
@ 2019-01-07 10:06         ` Roberto Sassu
  2019-01-10 17:38           ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-07 10:06 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On 12/22/2018 1:03 AM, Jarkko Sakkinen wrote:
> On Fri, Dec 21, 2018 at 10:40:09AM +0100, Roberto Sassu wrote:
>> On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
>>> On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
>>>> This patch renames active_banks (member of tpm_chip) to allocated_banks,
>>>> stores the number of allocated PCR banks in nr_allocated_banks (new member
>>>> of tpm_chip), and replaces the static array with a pointer to a dynamically
>>>> allocated array.
>>>>
>>>> tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
>>>> the mask in the TPML_PCR_SELECTION structure returned by the TPM for
>>>> TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
>>>> bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
>>>> case, the bank is not included in chip->allocated_banks, to avoid that TPM
>>>> driver users unnecessarily calculate a digest for that bank.
>>>>
>>>> One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
>>>>
>>>> As a consequence of the introduction of nr_allocated_banks,
>>>> tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
>>>> is equal to zero.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> ---
>>>>    drivers/char/tpm/tpm-chip.c      |  1 +
>>>>    drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
>>>>    drivers/char/tpm/tpm.h           |  3 ++-
>>>>    drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
>>>>    drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
>>>>    5 files changed, 47 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>>> index 32db84683c40..ce851c62bb68 100644
>>>> --- a/drivers/char/tpm/tpm-chip.c
>>>> +++ b/drivers/char/tpm/tpm-chip.c
>>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>>    	kfree(chip->log.bios_event_log);
>>>>    	kfree(chip->work_space.context_buf);
>>>>    	kfree(chip->work_space.session_buf);
>>>> +	kfree(chip->allocated_banks);
>>>>    	kfree(chip);
>>>>    }
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index d9439f9abe78..7b80919228be 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>>>    int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>>    {
>>>>    	int rc;
>>>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>>> -	u32 count = 0;
>>>> +	struct tpm2_digest *digest_list;
>>>>    	int i;
>>>>    	chip = tpm_find_get_ops(chip);
>>>> @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>>    		return -ENODEV;
>>>>    	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> -		memset(digest_list, 0, sizeof(digest_list));
>>>> +		digest_list = kcalloc(chip->nr_allocated_banks,
>>>> +				      sizeof(*digest_list), GFP_KERNEL);
>>>> +		if (!digest_list)
>>>> +			return -ENOMEM;
>>>
>>> You could preallocate digest list and place it to struct tpm_chip
>>> instead of doing it everytime tpm_pcr_extend() called.
>>
>> This part will be removed with patch 5/5.
> 
> Even if it did, it does not make this patch unbroken.

Can two calls to tpm_pcr_extend() be executed at the same time?

If yes, the digest list should be protected by a mutex.

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2019-01-07 10:06         ` Roberto Sassu
@ 2019-01-10 17:38           ` Jarkko Sakkinen
  2019-01-11  7:53             ` Roberto Sassu
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-10 17:38 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Mon, Jan 07, 2019 at 11:06:33AM +0100, Roberto Sassu wrote:
> On 12/22/2018 1:03 AM, Jarkko Sakkinen wrote:
> > On Fri, Dec 21, 2018 at 10:40:09AM +0100, Roberto Sassu wrote:
> > > On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
> > > > On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
> > > > > This patch renames active_banks (member of tpm_chip) to allocated_banks,
> > > > > stores the number of allocated PCR banks in nr_allocated_banks (new member
> > > > > of tpm_chip), and replaces the static array with a pointer to a dynamically
> > > > > allocated array.
> > > > > 
> > > > > tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
> > > > > the mask in the TPML_PCR_SELECTION structure returned by the TPM for
> > > > > TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
> > > > > bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
> > > > > case, the bank is not included in chip->allocated_banks, to avoid that TPM
> > > > > driver users unnecessarily calculate a digest for that bank.
> > > > > 
> > > > > One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
> > > > > 
> > > > > As a consequence of the introduction of nr_allocated_banks,
> > > > > tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
> > > > > is equal to zero.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > ---
> > > > >    drivers/char/tpm/tpm-chip.c      |  1 +
> > > > >    drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
> > > > >    drivers/char/tpm/tpm.h           |  3 ++-
> > > > >    drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
> > > > >    drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
> > > > >    5 files changed, 47 insertions(+), 19 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > > > index 32db84683c40..ce851c62bb68 100644
> > > > > --- a/drivers/char/tpm/tpm-chip.c
> > > > > +++ b/drivers/char/tpm/tpm-chip.c
> > > > > @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
> > > > >    	kfree(chip->log.bios_event_log);
> > > > >    	kfree(chip->work_space.context_buf);
> > > > >    	kfree(chip->work_space.session_buf);
> > > > > +	kfree(chip->allocated_banks);
> > > > >    	kfree(chip);
> > > > >    }
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > > index d9439f9abe78..7b80919228be 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> > > > >    int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > > >    {
> > > > >    	int rc;
> > > > > -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> > > > > -	u32 count = 0;
> > > > > +	struct tpm2_digest *digest_list;
> > > > >    	int i;
> > > > >    	chip = tpm_find_get_ops(chip);
> > > > > @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > > >    		return -ENODEV;
> > > > >    	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > > > -		memset(digest_list, 0, sizeof(digest_list));
> > > > > +		digest_list = kcalloc(chip->nr_allocated_banks,
> > > > > +				      sizeof(*digest_list), GFP_KERNEL);
> > > > > +		if (!digest_list)
> > > > > +			return -ENOMEM;
> > > > 
> > > > You could preallocate digest list and place it to struct tpm_chip
> > > > instead of doing it everytime tpm_pcr_extend() called.
> > > 
> > > This part will be removed with patch 5/5.
> > 
> > Even if it did, it does not make this patch unbroken.
> 
> Can two calls to tpm_pcr_extend() be executed at the same time?
> 
> If yes, the digest list should be protected by a mutex.

Good question: the answer is no. Mutex locking is done inside the
transmit flow ATM.

/Jarkko

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2019-01-10 17:38           ` Jarkko Sakkinen
@ 2019-01-11  7:53             ` Roberto Sassu
  2019-01-11 16:35               ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-11  7:53 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On 1/10/2019 6:38 PM, Jarkko Sakkinen wrote:
> On Mon, Jan 07, 2019 at 11:06:33AM +0100, Roberto Sassu wrote:
>> On 12/22/2018 1:03 AM, Jarkko Sakkinen wrote:
>>> On Fri, Dec 21, 2018 at 10:40:09AM +0100, Roberto Sassu wrote:
>>>> On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
>>>>> On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
>>>>>> This patch renames active_banks (member of tpm_chip) to allocated_banks,
>>>>>> stores the number of allocated PCR banks in nr_allocated_banks (new member
>>>>>> of tpm_chip), and replaces the static array with a pointer to a dynamically
>>>>>> allocated array.
>>>>>>
>>>>>> tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
>>>>>> the mask in the TPML_PCR_SELECTION structure returned by the TPM for
>>>>>> TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
>>>>>> bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
>>>>>> case, the bank is not included in chip->allocated_banks, to avoid that TPM
>>>>>> driver users unnecessarily calculate a digest for that bank.
>>>>>>
>>>>>> One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
>>>>>>
>>>>>> As a consequence of the introduction of nr_allocated_banks,
>>>>>> tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
>>>>>> is equal to zero.
>>>>>>
>>>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>>>> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/char/tpm/tpm-chip.c      |  1 +
>>>>>>     drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
>>>>>>     drivers/char/tpm/tpm.h           |  3 ++-
>>>>>>     drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
>>>>>>     drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
>>>>>>     5 files changed, 47 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
>>>>>> index 32db84683c40..ce851c62bb68 100644
>>>>>> --- a/drivers/char/tpm/tpm-chip.c
>>>>>> +++ b/drivers/char/tpm/tpm-chip.c
>>>>>> @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
>>>>>>     	kfree(chip->log.bios_event_log);
>>>>>>     	kfree(chip->work_space.context_buf);
>>>>>>     	kfree(chip->work_space.session_buf);
>>>>>> +	kfree(chip->allocated_banks);
>>>>>>     	kfree(chip);
>>>>>>     }
>>>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>>>> index d9439f9abe78..7b80919228be 100644
>>>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>>>> @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>>>>>     int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>>>>     {
>>>>>>     	int rc;
>>>>>> -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
>>>>>> -	u32 count = 0;
>>>>>> +	struct tpm2_digest *digest_list;
>>>>>>     	int i;
>>>>>>     	chip = tpm_find_get_ops(chip);
>>>>>> @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>>>>     		return -ENODEV;
>>>>>>     	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>>>> -		memset(digest_list, 0, sizeof(digest_list));
>>>>>> +		digest_list = kcalloc(chip->nr_allocated_banks,
>>>>>> +				      sizeof(*digest_list), GFP_KERNEL);
>>>>>> +		if (!digest_list)
>>>>>> +			return -ENOMEM;
>>>>>
>>>>> You could preallocate digest list and place it to struct tpm_chip
>>>>> instead of doing it everytime tpm_pcr_extend() called.
>>>>
>>>> This part will be removed with patch 5/5.
>>>
>>> Even if it did, it does not make this patch unbroken.
>>
>> Can two calls to tpm_pcr_extend() be executed at the same time?
>>
>> If yes, the digest list should be protected by a mutex.
> 
> Good question: the answer is no. Mutex locking is done inside the
> transmit flow ATM.

But data are copied before the mutex is locked. Can't a second call
overwrite chip->preallocated_digest_list while the first call is still
writing it?

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array
  2019-01-11  7:53             ` Roberto Sassu
@ 2019-01-11 16:35               ` Jarkko Sakkinen
  0 siblings, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-11 16:35 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Fri, Jan 11, 2019 at 08:53:00AM +0100, Roberto Sassu wrote:
> On 1/10/2019 6:38 PM, Jarkko Sakkinen wrote:
> > On Mon, Jan 07, 2019 at 11:06:33AM +0100, Roberto Sassu wrote:
> > > On 12/22/2018 1:03 AM, Jarkko Sakkinen wrote:
> > > > On Fri, Dec 21, 2018 at 10:40:09AM +0100, Roberto Sassu wrote:
> > > > > On 12/20/2018 3:55 PM, Jarkko Sakkinen wrote:
> > > > > > On Thu, Dec 13, 2018 at 11:29:41AM +0100, Roberto Sassu wrote:
> > > > > > > This patch renames active_banks (member of tpm_chip) to allocated_banks,
> > > > > > > stores the number of allocated PCR banks in nr_allocated_banks (new member
> > > > > > > of tpm_chip), and replaces the static array with a pointer to a dynamically
> > > > > > > allocated array.
> > > > > > > 
> > > > > > > tpm2_get_pcr_allocation() determines if a PCR bank is allocated by checking
> > > > > > > the mask in the TPML_PCR_SELECTION structure returned by the TPM for
> > > > > > > TPM2_Get_Capability(). If a bank is not allocated, the TPM returns that
> > > > > > > bank in TPML_PCR_SELECTION, with all bits in the mask set to zero. In this
> > > > > > > case, the bank is not included in chip->allocated_banks, to avoid that TPM
> > > > > > > driver users unnecessarily calculate a digest for that bank.
> > > > > > > 
> > > > > > > One PCR bank with algorithm set to SHA1 is always allocated for TPM 1.x.
> > > > > > > 
> > > > > > > As a consequence of the introduction of nr_allocated_banks,
> > > > > > > tpm_pcr_extend() does not check anymore if the algorithm stored in tpm_chip
> > > > > > > is equal to zero.
> > > > > > > 
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > > > ---
> > > > > > >     drivers/char/tpm/tpm-chip.c      |  1 +
> > > > > > >     drivers/char/tpm/tpm-interface.c | 18 +++++++++--------
> > > > > > >     drivers/char/tpm/tpm.h           |  3 ++-
> > > > > > >     drivers/char/tpm/tpm1-cmd.c      | 10 ++++++++++
> > > > > > >     drivers/char/tpm/tpm2-cmd.c      | 34 ++++++++++++++++++++++----------
> > > > > > >     5 files changed, 47 insertions(+), 19 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> > > > > > > index 32db84683c40..ce851c62bb68 100644
> > > > > > > --- a/drivers/char/tpm/tpm-chip.c
> > > > > > > +++ b/drivers/char/tpm/tpm-chip.c
> > > > > > > @@ -160,6 +160,7 @@ static void tpm_dev_release(struct device *dev)
> > > > > > >     	kfree(chip->log.bios_event_log);
> > > > > > >     	kfree(chip->work_space.context_buf);
> > > > > > >     	kfree(chip->work_space.session_buf);
> > > > > > > +	kfree(chip->allocated_banks);
> > > > > > >     	kfree(chip);
> > > > > > >     }
> > > > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > > > > index d9439f9abe78..7b80919228be 100644
> > > > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > > > @@ -488,8 +488,7 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> > > > > > >     int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > > > > >     {
> > > > > > >     	int rc;
> > > > > > > -	struct tpm2_digest digest_list[ARRAY_SIZE(chip->active_banks)];
> > > > > > > -	u32 count = 0;
> > > > > > > +	struct tpm2_digest *digest_list;
> > > > > > >     	int i;
> > > > > > >     	chip = tpm_find_get_ops(chip);
> > > > > > > @@ -497,16 +496,19 @@ int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > > > > >     		return -ENODEV;
> > > > > > >     	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > > > > > -		memset(digest_list, 0, sizeof(digest_list));
> > > > > > > +		digest_list = kcalloc(chip->nr_allocated_banks,
> > > > > > > +				      sizeof(*digest_list), GFP_KERNEL);
> > > > > > > +		if (!digest_list)
> > > > > > > +			return -ENOMEM;
> > > > > > 
> > > > > > You could preallocate digest list and place it to struct tpm_chip
> > > > > > instead of doing it everytime tpm_pcr_extend() called.
> > > > > 
> > > > > This part will be removed with patch 5/5.
> > > > 
> > > > Even if it did, it does not make this patch unbroken.
> > > 
> > > Can two calls to tpm_pcr_extend() be executed at the same time?
> > > 
> > > If yes, the digest list should be protected by a mutex.
> > 
> > Good question: the answer is no. Mutex locking is done inside the
> > transmit flow ATM.
> 
> But data are copied before the mutex is locked. Can't a second call
> overwrite chip->preallocated_digest_list while the first call is still
> writing it?

Now I see what you mean. I have patch set on review that would make this
more natural to do. Locking can be done now too so that it would work
but that would counter-productive, so lets keep the approach that you
have.

/Jarkko

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2018-12-20 15:21   ` Jarkko Sakkinen
@ 2019-01-17  7:59     ` Roberto Sassu
  2019-01-18 15:12       ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-17  7:59 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>
>> This patch modifies the definition of tpm_pcr_extend() to allow other
>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>> All digests are processed by the TPM in one operation.
>>
>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>> the TPM driver extends the remaining PCR banks with the first digest
>> passed as an argument to the function.
>>
>> The new tpm_extend digest structure has been preferred to the tpm_digest
>> structure, to let the caller specify the size of the digest (which may be
>> unknown to the TPM driver).
>>
>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>> ---
>>   drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>>   drivers/char/tpm/tpm.h             |  5 +++--
>>   drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
>>   drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
>>   include/linux/tpm.h                | 13 ++++++++---
>>   security/integrity/ima/ima_queue.c |  5 ++++-
>>   security/keys/trusted.c            |  5 ++++-
>>   7 files changed, 62 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> index eb7c79ca8a94..911fea19e408 100644
>> --- a/drivers/char/tpm/tpm-interface.c
>> +++ b/drivers/char/tpm/tpm-interface.c
>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>    * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>>    * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>>    * @pcr_idx:	the PCR to be retrieved
>> - * @hash:	the hash value used to extend the PCR value
>> + * @count:	number of tpm_extend_digest structures
>> + * @digests:	array of tpm_extend_digest structures used to extend PCRs
>>    *
>>    * Note: with TPM 2.0 extends also those banks for which no digest was
>>    * specified in order to prevent malicious use of those PCR banks.
>>    *
>>    * Return: same as with tpm_transmit_cmd()
>>    */
>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		   const struct tpm_extend_digest *digests)
> 
> Remove const. Document how @digests is used  like the special meaning
> of the first index. I faintly remember asking this last time.
> 
>>   {
>>   	int rc;
>> -	struct tpm_digest *digest_list;
>> -	int i;
>>   
>>   	chip = tpm_find_get_ops(chip);
>>   	if (!chip)
>>   		return -ENODEV;
>>   
>>   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>> -		digest_list = kcalloc(chip->nr_allocated_banks,
>> -				      sizeof(*digest_list), GFP_KERNEL);
>> -		if (!digest_list)
>> -			return -ENOMEM;
>> -
>> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
>> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
>> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>> -		}
>> -
>> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>> -				     digest_list);
>> -		kfree(digest_list);
>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
>>   		tpm_put_ops(chip);
>>   		return rc;
>>   	}
>>   
>> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
>> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
>>   			     "attempting extend a PCR value");
> 
> The validation is missing that the provided array has only one element
> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
> but what you are doing to that function does not make any sense so
> better to do it here.
> 
>>   	tpm_put_ops(chip);
>>   	return rc;
>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>> index 64d93d26087f..6b446504d2fe 100644
>> --- a/drivers/char/tpm/tpm.h
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>>   int tpm1_do_selftest(struct tpm_chip *chip);
>>   int tpm1_get_timeouts(struct tpm_chip *chip);
>>   unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		    const struct tpm_extend_digest *digests,
>>   		    const char *log_msg);
>>   int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>>   ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>>   int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>   		  struct tpm_digest *digest, u16 *digest_size_ptr);
>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> -		    struct tpm_digest *digests);
>> +		    const struct tpm_extend_digest *digests);
>>   int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>>   void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>>   			    unsigned int flags);
>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>> index 8b70a7f884a7..04ee10284b8c 100644
>> --- a/drivers/char/tpm/tpm1-cmd.c
>> +++ b/drivers/char/tpm/tpm1-cmd.c
>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>>   }
>>   
>>   #define TPM_ORD_PCR_EXTEND 20
>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> +		    const struct tpm_extend_digest *digests,
>>   		    const char *log_msg)
>>   {
>>   	struct tpm_buf buf;
>> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>> +	const u8 *hash;
>>   	int rc;
>>   
>> +	hash = dummy_hash;
>> +	if (count)
>> +		memcpy(dummy_hash, digests[0].data,
>> +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
>> +
> 
> You copy memory from one place to another without any good reason to do
> so. My suggestion is just not to change tpm1_pcr_extend() at all.
> 
>>   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
>>   	if (rc)
>>   		return rc;
>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>    */
>>   int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>   {
>> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>   	struct tpm_buf buf;
>>   	unsigned int try;
>>   	int rc;
>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>   
>>   	/* for buggy tpm, flush pcrs with extend to selected dummy */
>>   	if (tpm_suspend_pcr)
>> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
>> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>>   				     "extending dummy pcr before suspend");
>>   
>>   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 6ce5173cf0e5..77b5808270c6 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
>>    *
>>    * @chip:	TPM chip to use.
>>    * @pcr_idx:	index of the PCR.
>> - * @count:	number of digests passed.
>> - * @digests:	list of pcr banks and corresponding digest values to extend.
>> + * @count:	number of tpm_extend_digest passed.
>> + * @digests:	array of tpm_extend_digest with digest values to extend.
>>    *
>>    * Return: Same as with tpm_transmit_cmd.
>>    */
> 
> The documentation about @digests.
> 
>>   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>> -		    struct tpm_digest *digests)
>> +		    const struct tpm_extend_digest *digests)
>>   {
>>   	struct tpm_buf buf;
>>   	struct tpm2_null_auth_area auth_area;
>> +	const struct tpm_extend_digest *digest;
>> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
>> +	const u8 *hash;
>>   	int rc;
>>   	int i;
>> -
>> -	if (count > chip->nr_allocated_banks)
>> -		return -EINVAL;
>> +	int j;
>>   
>>   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>   	if (rc)
>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>   	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>>   	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>>   		       sizeof(auth_area));
>> -	tpm_buf_append_u32(&buf, count);
>> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>> +
>> +	if (count)
>> +		memcpy(dummy_hash, digests[0].data, digests[0].size);
>> +
>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
>> +
>> +		hash = dummy_hash;
>> +		for (j = 0; j < count; j++) {
>> +			digest = digests + j;
>> +
>> +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
>> +				hash = digest->data;
>> +				break;
> 
> I think the whole design is just wrong. I did re-read your response to
> v6 again and I'm very sorry, but I just don't get this. Caller has all
> the information (from struct tpm_chip) to give the correct data. This
> function should validate that data (check algorithm ID and that's it).

The question is if checking tpm->allocated_banks is a strict
requirement, or we can allow callers to use the algorithm they are
currently using, without further modifications.


> Extending with the dummy hash should be done by the caller when
> preparing the array, not baked into this function. This kind of also
> makes obvious that we don't need this new struct. There should never be
> a local variable (whose size is BTW randomly chosen) called dummy_hash.

This means duplicating the code for each caller. Currently, this work is
done by the TPM driver.

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-17  7:59     ` Roberto Sassu
@ 2019-01-18 15:12       ` Jarkko Sakkinen
  2019-01-21  8:11         ` Roberto Sassu
  2019-01-21  9:58         ` Roberto Sassu
  0 siblings, 2 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-18 15:12 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu

On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote:
> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
> > On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
> > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> > > 
> > > This patch modifies the definition of tpm_pcr_extend() to allow other
> > > kernel subsystems to pass a digest for each algorithm supported by the TPM.
> > > All digests are processed by the TPM in one operation.
> > > 
> > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> > > the TPM driver extends the remaining PCR banks with the first digest
> > > passed as an argument to the function.
> > > 
> > > The new tpm_extend digest structure has been preferred to the tpm_digest
> > > structure, to let the caller specify the size of the digest (which may be
> > > unknown to the TPM driver).
> > > 
> > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
> > > 
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >   drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
> > >   drivers/char/tpm/tpm.h             |  5 +++--
> > >   drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
> > >   drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
> > >   include/linux/tpm.h                | 13 ++++++++---
> > >   security/integrity/ima/ima_queue.c |  5 ++++-
> > >   security/keys/trusted.c            |  5 ++++-
> > >   7 files changed, 62 insertions(+), 38 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index eb7c79ca8a94..911fea19e408 100644
> > > --- a/drivers/char/tpm/tpm-interface.c
> > > +++ b/drivers/char/tpm/tpm-interface.c
> > > @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> > >    * tpm_pcr_extend - extend a PCR value in SHA1 bank.
> > >    * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> > >    * @pcr_idx:	the PCR to be retrieved
> > > - * @hash:	the hash value used to extend the PCR value
> > > + * @count:	number of tpm_extend_digest structures
> > > + * @digests:	array of tpm_extend_digest structures used to extend PCRs
> > >    *
> > >    * Note: with TPM 2.0 extends also those banks for which no digest was
> > >    * specified in order to prevent malicious use of those PCR banks.
> > >    *
> > >    * Return: same as with tpm_transmit_cmd()
> > >    */
> > > -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > +		   const struct tpm_extend_digest *digests)
> > 
> > Remove const. Document how @digests is used  like the special meaning
> > of the first index. I faintly remember asking this last time.
> > 
> > >   {
> > >   	int rc;
> > > -	struct tpm_digest *digest_list;
> > > -	int i;
> > >   	chip = tpm_find_get_ops(chip);
> > >   	if (!chip)
> > >   		return -ENODEV;
> > >   	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > -		digest_list = kcalloc(chip->nr_allocated_banks,
> > > -				      sizeof(*digest_list), GFP_KERNEL);
> > > -		if (!digest_list)
> > > -			return -ENOMEM;
> > > -
> > > -		for (i = 0; i < chip->nr_allocated_banks; i++) {
> > > -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
> > > -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> > > -		}
> > > -
> > > -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
> > > -				     digest_list);
> > > -		kfree(digest_list);
> > > +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
> > >   		tpm_put_ops(chip);
> > >   		return rc;
> > >   	}
> > > -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
> > > +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
> > >   			     "attempting extend a PCR value");
> > 
> > The validation is missing that the provided array has only one element
> > and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
> > but what you are doing to that function does not make any sense so
> > better to do it here.
> > 
> > >   	tpm_put_ops(chip);
> > >   	return rc;
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index 64d93d26087f..6b446504d2fe 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
> > >   int tpm1_do_selftest(struct tpm_chip *chip);
> > >   int tpm1_get_timeouts(struct tpm_chip *chip);
> > >   unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > +		    const struct tpm_extend_digest *digests,
> > >   		    const char *log_msg);
> > >   int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
> > >   ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> > > @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
> > >   int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> > >   		  struct tpm_digest *digest, u16 *digest_size_ptr);
> > >   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > -		    struct tpm_digest *digests);
> > > +		    const struct tpm_extend_digest *digests);
> > >   int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
> > >   void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
> > >   			    unsigned int flags);
> > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> > > index 8b70a7f884a7..04ee10284b8c 100644
> > > --- a/drivers/char/tpm/tpm1-cmd.c
> > > +++ b/drivers/char/tpm/tpm1-cmd.c
> > > @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
> > >   }
> > >   #define TPM_ORD_PCR_EXTEND 20
> > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > +		    const struct tpm_extend_digest *digests,
> > >   		    const char *log_msg)
> > >   {
> > >   	struct tpm_buf buf;
> > > +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> > > +	const u8 *hash;
> > >   	int rc;
> > > +	hash = dummy_hash;
> > > +	if (count)
> > > +		memcpy(dummy_hash, digests[0].data,
> > > +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
> > > +
> > 
> > You copy memory from one place to another without any good reason to do
> > so. My suggestion is just not to change tpm1_pcr_extend() at all.
> > 
> > >   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
> > >   	if (rc)
> > >   		return rc;
> > > @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
> > >    */
> > >   int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
> > >   {
> > > -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> > >   	struct tpm_buf buf;
> > >   	unsigned int try;
> > >   	int rc;
> > > @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
> > >   	/* for buggy tpm, flush pcrs with extend to selected dummy */
> > >   	if (tpm_suspend_pcr)
> > > -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
> > > +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
> > >   				     "extending dummy pcr before suspend");
> > >   	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
> > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > > index 6ce5173cf0e5..77b5808270c6 100644
> > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
> > >    *
> > >    * @chip:	TPM chip to use.
> > >    * @pcr_idx:	index of the PCR.
> > > - * @count:	number of digests passed.
> > > - * @digests:	list of pcr banks and corresponding digest values to extend.
> > > + * @count:	number of tpm_extend_digest passed.
> > > + * @digests:	array of tpm_extend_digest with digest values to extend.
> > >    *
> > >    * Return: Same as with tpm_transmit_cmd.
> > >    */
> > 
> > The documentation about @digests.
> > 
> > >   int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > -		    struct tpm_digest *digests)
> > > +		    const struct tpm_extend_digest *digests)
> > >   {
> > >   	struct tpm_buf buf;
> > >   	struct tpm2_null_auth_area auth_area;
> > > +	const struct tpm_extend_digest *digest;
> > > +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
> > > +	const u8 *hash;
> > >   	int rc;
> > >   	int i;
> > > -
> > > -	if (count > chip->nr_allocated_banks)
> > > -		return -EINVAL;
> > > +	int j;
> > >   	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> > >   	if (rc)
> > > @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > >   	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
> > >   	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
> > >   		       sizeof(auth_area));
> > > -	tpm_buf_append_u32(&buf, count);
> > > +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> > > +
> > > +	if (count)
> > > +		memcpy(dummy_hash, digests[0].data, digests[0].size);
> > > +
> > > +	for (i = 0; i < chip->nr_allocated_banks; i++) {
> > > +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
> > > +
> > > +		hash = dummy_hash;
> > > +		for (j = 0; j < count; j++) {
> > > +			digest = digests + j;
> > > +
> > > +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
> > > +				hash = digest->data;
> > > +				break;
> > 
> > I think the whole design is just wrong. I did re-read your response to
> > v6 again and I'm very sorry, but I just don't get this. Caller has all
> > the information (from struct tpm_chip) to give the correct data. This
> > function should validate that data (check algorithm ID and that's it).
> 
> The question is if checking tpm->allocated_banks is a strict
> requirement, or we can allow callers to use the algorithm they are
> currently using, without further modifications.

If you want to know what is available, you can use that array.

> > Extending with the dummy hash should be done by the caller when
> > preparing the array, not baked into this function. This kind of also
> > makes obvious that we don't need this new struct. There should never be
> > a local variable (whose size is BTW randomly chosen) called dummy_hash.
> 
> This means duplicating the code for each caller. Currently, this work is
> done by the TPM driver.

It is better than fix the usage pattern like this. Not only that, but
this somewhat complicated behavior now completely undocumented in the
function description.

You are making strong assumptions how some other subsystem might use
extend operation. I will rather accept redundancy and consider
consolidation later when there are two clients. Rather keep the TPM
provided function simple and stupid.

/Jarkko

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-18 15:12       ` Jarkko Sakkinen
@ 2019-01-21  8:11         ` Roberto Sassu
  2019-01-21 12:30           ` Jarkko Sakkinen
  2019-01-21  9:58         ` Roberto Sassu
  1 sibling, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-21  8:11 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote:
> On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote:
>> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
>>> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
>>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>>>
>>>> This patch modifies the definition of tpm_pcr_extend() to allow other
>>>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>>>> All digests are processed by the TPM in one operation.
>>>>
>>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>>>> the TPM driver extends the remaining PCR banks with the first digest
>>>> passed as an argument to the function.
>>>>
>>>> The new tpm_extend digest structure has been preferred to the tpm_digest
>>>> structure, to let the caller specify the size of the digest (which may be
>>>> unknown to the TPM driver).
>>>>
>>>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>>    drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>>>>    drivers/char/tpm/tpm.h             |  5 +++--
>>>>    drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
>>>>    drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
>>>>    include/linux/tpm.h                | 13 ++++++++---
>>>>    security/integrity/ima/ima_queue.c |  5 ++++-
>>>>    security/keys/trusted.c            |  5 ++++-
>>>>    7 files changed, 62 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index eb7c79ca8a94..911fea19e408 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>>>     * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>>>>     * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>>>>     * @pcr_idx:	the PCR to be retrieved
>>>> - * @hash:	the hash value used to extend the PCR value
>>>> + * @count:	number of tpm_extend_digest structures
>>>> + * @digests:	array of tpm_extend_digest structures used to extend PCRs
>>>>     *
>>>>     * Note: with TPM 2.0 extends also those banks for which no digest was
>>>>     * specified in order to prevent malicious use of those PCR banks.
>>>>     *
>>>>     * Return: same as with tpm_transmit_cmd()
>>>>     */
>>>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		   const struct tpm_extend_digest *digests)
>>>
>>> Remove const. Document how @digests is used  like the special meaning
>>> of the first index. I faintly remember asking this last time.
>>>
>>>>    {
>>>>    	int rc;
>>>> -	struct tpm_digest *digest_list;
>>>> -	int i;
>>>>    	chip = tpm_find_get_ops(chip);
>>>>    	if (!chip)
>>>>    		return -ENODEV;
>>>>    	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> -		digest_list = kcalloc(chip->nr_allocated_banks,
>>>> -				      sizeof(*digest_list), GFP_KERNEL);
>>>> -		if (!digest_list)
>>>> -			return -ENOMEM;
>>>> -
>>>> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
>>>> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>> -		}
>>>> -
>>>> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>>>> -				     digest_list);
>>>> -		kfree(digest_list);
>>>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
>>>>    		tpm_put_ops(chip);
>>>>    		return rc;
>>>>    	}
>>>> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
>>>> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
>>>>    			     "attempting extend a PCR value");
>>>
>>> The validation is missing that the provided array has only one element
>>> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
>>> but what you are doing to that function does not make any sense so
>>> better to do it here.
>>>
>>>>    	tpm_put_ops(chip);
>>>>    	return rc;
>>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>>> index 64d93d26087f..6b446504d2fe 100644
>>>> --- a/drivers/char/tpm/tpm.h
>>>> +++ b/drivers/char/tpm/tpm.h
>>>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>>>>    int tpm1_do_selftest(struct tpm_chip *chip);
>>>>    int tpm1_get_timeouts(struct tpm_chip *chip);
>>>>    unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		    const struct tpm_extend_digest *digests,
>>>>    		    const char *log_msg);
>>>>    int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>>>>    ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>>>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>>>>    int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>>>    		  struct tpm_digest *digest, u16 *digest_size_ptr);
>>>>    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> -		    struct tpm_digest *digests);
>>>> +		    const struct tpm_extend_digest *digests);
>>>>    int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>>>>    void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>>>>    			    unsigned int flags);
>>>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>>>> index 8b70a7f884a7..04ee10284b8c 100644
>>>> --- a/drivers/char/tpm/tpm1-cmd.c
>>>> +++ b/drivers/char/tpm/tpm1-cmd.c
>>>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>>>>    }
>>>>    #define TPM_ORD_PCR_EXTEND 20
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		    const struct tpm_extend_digest *digests,
>>>>    		    const char *log_msg)
>>>>    {
>>>>    	struct tpm_buf buf;
>>>> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>> +	const u8 *hash;
>>>>    	int rc;
>>>> +	hash = dummy_hash;
>>>> +	if (count)
>>>> +		memcpy(dummy_hash, digests[0].data,
>>>> +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
>>>> +
>>>
>>> You copy memory from one place to another without any good reason to do
>>> so. My suggestion is just not to change tpm1_pcr_extend() at all.
>>>
>>>>    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
>>>>    	if (rc)
>>>>    		return rc;
>>>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>>>     */
>>>>    int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>>    {
>>>> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>>    	struct tpm_buf buf;
>>>>    	unsigned int try;
>>>>    	int rc;
>>>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>>    	/* for buggy tpm, flush pcrs with extend to selected dummy */
>>>>    	if (tpm_suspend_pcr)
>>>> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
>>>> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>>>>    				     "extending dummy pcr before suspend");
>>>>    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index 6ce5173cf0e5..77b5808270c6 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
>>>>     *
>>>>     * @chip:	TPM chip to use.
>>>>     * @pcr_idx:	index of the PCR.
>>>> - * @count:	number of digests passed.
>>>> - * @digests:	list of pcr banks and corresponding digest values to extend.
>>>> + * @count:	number of tpm_extend_digest passed.
>>>> + * @digests:	array of tpm_extend_digest with digest values to extend.
>>>>     *
>>>>     * Return: Same as with tpm_transmit_cmd.
>>>>     */
>>>
>>> The documentation about @digests.
>>>
>>>>    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> -		    struct tpm_digest *digests)
>>>> +		    const struct tpm_extend_digest *digests)
>>>>    {
>>>>    	struct tpm_buf buf;
>>>>    	struct tpm2_null_auth_area auth_area;
>>>> +	const struct tpm_extend_digest *digest;
>>>> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
>>>> +	const u8 *hash;
>>>>    	int rc;
>>>>    	int i;
>>>> -
>>>> -	if (count > chip->nr_allocated_banks)
>>>> -		return -EINVAL;
>>>> +	int j;
>>>>    	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>>>    	if (rc)
>>>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>>    	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>>>>    	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>>>>    		       sizeof(auth_area));
>>>> -	tpm_buf_append_u32(&buf, count);
>>>> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>>>> +
>>>> +	if (count)
>>>> +		memcpy(dummy_hash, digests[0].data, digests[0].size);
>>>> +
>>>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
>>>> +
>>>> +		hash = dummy_hash;
>>>> +		for (j = 0; j < count; j++) {
>>>> +			digest = digests + j;
>>>> +
>>>> +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
>>>> +				hash = digest->data;
>>>> +				break;
>>>
>>> I think the whole design is just wrong. I did re-read your response to
>>> v6 again and I'm very sorry, but I just don't get this. Caller has all
>>> the information (from struct tpm_chip) to give the correct data. This
>>> function should validate that data (check algorithm ID and that's it).
>>
>> The question is if checking tpm->allocated_banks is a strict
>> requirement, or we can allow callers to use the algorithm they are
>> currently using, without further modifications.
> 
> If you want to know what is available, you can use that array.
> 
>>> Extending with the dummy hash should be done by the caller when
>>> preparing the array, not baked into this function. This kind of also
>>> makes obvious that we don't need this new struct. There should never be
>>> a local variable (whose size is BTW randomly chosen) called dummy_hash.
>>
>> This means duplicating the code for each caller. Currently, this work is
>> done by the TPM driver.
> 
> It is better than fix the usage pattern like this. Not only that, but
> this somewhat complicated behavior now completely undocumented in the
> function description.
> 
> You are making strong assumptions how some other subsystem might use
> extend operation. I will rather accept redundancy and consider
> consolidation later when there are two clients. Rather keep the TPM
> provided function simple and stupid.

Ok, if I understood it correctly, every caller of tpm_pcr_extend() will 
pass an array of tpm_digest structures.

I will not introduce a new structure, as the caller has to use
algorithms from chip->allocated_banks and passing the digest size is not
necessary.

tpm_pcr_extend() will reject incomplete arrays (one or more algorithms
are missing). To simplify even more tpm_pcr_extend(), I assume and check
that passed algorithms are in the same order as those in
chip->allocated_banks.

I will remove the 'count' parameter from tpm_pcr_extend(), as the number
of tpm_digest structures will be always chip->nr_allocated_banks.

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-18 15:12       ` Jarkko Sakkinen
  2019-01-21  8:11         ` Roberto Sassu
@ 2019-01-21  9:58         ` Roberto Sassu
  2019-01-21 12:37           ` Jarkko Sakkinen
  1 sibling, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-21  9:58 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote:
> On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote:
>> On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
>>> On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
>>>> Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
>>>>
>>>> This patch modifies the definition of tpm_pcr_extend() to allow other
>>>> kernel subsystems to pass a digest for each algorithm supported by the TPM.
>>>> All digests are processed by the TPM in one operation.
>>>>
>>>> If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
>>>> the TPM driver extends the remaining PCR banks with the first digest
>>>> passed as an argument to the function.
>>>>
>>>> The new tpm_extend digest structure has been preferred to the tpm_digest
>>>> structure, to let the caller specify the size of the digest (which may be
>>>> unknown to the TPM driver).
>>>>
>>>> Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
>>>>
>>>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
>>>> ---
>>>>    drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
>>>>    drivers/char/tpm/tpm.h             |  5 +++--
>>>>    drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
>>>>    drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
>>>>    include/linux/tpm.h                | 13 ++++++++---
>>>>    security/integrity/ima/ima_queue.c |  5 ++++-
>>>>    security/keys/trusted.c            |  5 ++++-
>>>>    7 files changed, 62 insertions(+), 38 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>>>> index eb7c79ca8a94..911fea19e408 100644
>>>> --- a/drivers/char/tpm/tpm-interface.c
>>>> +++ b/drivers/char/tpm/tpm-interface.c
>>>> @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
>>>>     * tpm_pcr_extend - extend a PCR value in SHA1 bank.
>>>>     * @chip:	a &struct tpm_chip instance, %NULL for the default chip
>>>>     * @pcr_idx:	the PCR to be retrieved
>>>> - * @hash:	the hash value used to extend the PCR value
>>>> + * @count:	number of tpm_extend_digest structures
>>>> + * @digests:	array of tpm_extend_digest structures used to extend PCRs
>>>>     *
>>>>     * Note: with TPM 2.0 extends also those banks for which no digest was
>>>>     * specified in order to prevent malicious use of those PCR banks.
>>>>     *
>>>>     * Return: same as with tpm_transmit_cmd()
>>>>     */
>>>> -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
>>>> +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		   const struct tpm_extend_digest *digests)
>>>
>>> Remove const. Document how @digests is used  like the special meaning
>>> of the first index. I faintly remember asking this last time.
>>>
>>>>    {
>>>>    	int rc;
>>>> -	struct tpm_digest *digest_list;
>>>> -	int i;
>>>>    	chip = tpm_find_get_ops(chip);
>>>>    	if (!chip)
>>>>    		return -ENODEV;
>>>>    	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
>>>> -		digest_list = kcalloc(chip->nr_allocated_banks,
>>>> -				      sizeof(*digest_list), GFP_KERNEL);
>>>> -		if (!digest_list)
>>>> -			return -ENOMEM;
>>>> -
>>>> -		for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
>>>> -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
>>>> -		}
>>>> -
>>>> -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
>>>> -				     digest_list);
>>>> -		kfree(digest_list);
>>>> +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
>>>>    		tpm_put_ops(chip);
>>>>    		return rc;
>>>>    	}
>>>> -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
>>>> +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
>>>>    			     "attempting extend a PCR value");
>>>
>>> The validation is missing that the provided array has only one element
>>> and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
>>> but what you are doing to that function does not make any sense so
>>> better to do it here.
>>>
>>>>    	tpm_put_ops(chip);
>>>>    	return rc;
>>>> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
>>>> index 64d93d26087f..6b446504d2fe 100644
>>>> --- a/drivers/char/tpm/tpm.h
>>>> +++ b/drivers/char/tpm/tpm.h
>>>> @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
>>>>    int tpm1_do_selftest(struct tpm_chip *chip);
>>>>    int tpm1_get_timeouts(struct tpm_chip *chip);
>>>>    unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		    const struct tpm_extend_digest *digests,
>>>>    		    const char *log_msg);
>>>>    int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
>>>>    ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
>>>> @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
>>>>    int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
>>>>    		  struct tpm_digest *digest, u16 *digest_size_ptr);
>>>>    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> -		    struct tpm_digest *digests);
>>>> +		    const struct tpm_extend_digest *digests);
>>>>    int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
>>>>    void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>>>>    			    unsigned int flags);
>>>> diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
>>>> index 8b70a7f884a7..04ee10284b8c 100644
>>>> --- a/drivers/char/tpm/tpm1-cmd.c
>>>> +++ b/drivers/char/tpm/tpm1-cmd.c
>>>> @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
>>>>    }
>>>>    #define TPM_ORD_PCR_EXTEND 20
>>>> -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
>>>> +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> +		    const struct tpm_extend_digest *digests,
>>>>    		    const char *log_msg)
>>>>    {
>>>>    	struct tpm_buf buf;
>>>> +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>> +	const u8 *hash;
>>>>    	int rc;
>>>> +	hash = dummy_hash;
>>>> +	if (count)
>>>> +		memcpy(dummy_hash, digests[0].data,
>>>> +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
>>>> +
>>>
>>> You copy memory from one place to another without any good reason to do
>>> so. My suggestion is just not to change tpm1_pcr_extend() at all.
>>>
>>>>    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
>>>>    	if (rc)
>>>>    		return rc;
>>>> @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
>>>>     */
>>>>    int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>>    {
>>>> -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
>>>>    	struct tpm_buf buf;
>>>>    	unsigned int try;
>>>>    	int rc;
>>>> @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
>>>>    	/* for buggy tpm, flush pcrs with extend to selected dummy */
>>>>    	if (tpm_suspend_pcr)
>>>> -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
>>>> +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
>>>>    				     "extending dummy pcr before suspend");
>>>>    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
>>>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>>>> index 6ce5173cf0e5..77b5808270c6 100644
>>>> --- a/drivers/char/tpm/tpm2-cmd.c
>>>> +++ b/drivers/char/tpm/tpm2-cmd.c
>>>> @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
>>>>     *
>>>>     * @chip:	TPM chip to use.
>>>>     * @pcr_idx:	index of the PCR.
>>>> - * @count:	number of digests passed.
>>>> - * @digests:	list of pcr banks and corresponding digest values to extend.
>>>> + * @count:	number of tpm_extend_digest passed.
>>>> + * @digests:	array of tpm_extend_digest with digest values to extend.
>>>>     *
>>>>     * Return: Same as with tpm_transmit_cmd.
>>>>     */
>>>
>>> The documentation about @digests.
>>>
>>>>    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>> -		    struct tpm_digest *digests)
>>>> +		    const struct tpm_extend_digest *digests)
>>>>    {
>>>>    	struct tpm_buf buf;
>>>>    	struct tpm2_null_auth_area auth_area;
>>>> +	const struct tpm_extend_digest *digest;
>>>> +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
>>>> +	const u8 *hash;
>>>>    	int rc;
>>>>    	int i;
>>>> -
>>>> -	if (count > chip->nr_allocated_banks)
>>>> -		return -EINVAL;
>>>> +	int j;
>>>>    	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
>>>>    	if (rc)
>>>> @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
>>>>    	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
>>>>    	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
>>>>    		       sizeof(auth_area));
>>>> -	tpm_buf_append_u32(&buf, count);
>>>> +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
>>>> +
>>>> +	if (count)
>>>> +		memcpy(dummy_hash, digests[0].data, digests[0].size);
>>>> +
>>>> +	for (i = 0; i < chip->nr_allocated_banks; i++) {
>>>> +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
>>>> +
>>>> +		hash = dummy_hash;
>>>> +		for (j = 0; j < count; j++) {
>>>> +			digest = digests + j;
>>>> +
>>>> +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
>>>> +				hash = digest->data;
>>>> +				break;
>>>
>>> I think the whole design is just wrong. I did re-read your response to
>>> v6 again and I'm very sorry, but I just don't get this. Caller has all
>>> the information (from struct tpm_chip) to give the correct data. This
>>> function should validate that data (check algorithm ID and that's it).
>>
>> The question is if checking tpm->allocated_banks is a strict
>> requirement, or we can allow callers to use the algorithm they are
>> currently using, without further modifications.
> 
> If you want to know what is available, you can use that array.
> 
>>> Extending with the dummy hash should be done by the caller when
>>> preparing the array, not baked into this function. This kind of also
>>> makes obvious that we don't need this new struct. There should never be
>>> a local variable (whose size is BTW randomly chosen) called dummy_hash.
>>
>> This means duplicating the code for each caller. Currently, this work is
>> done by the TPM driver.
> 
> It is better than fix the usage pattern like this. Not only that, but
> this somewhat complicated behavior now completely undocumented in the
> function description.
> 
> You are making strong assumptions how some other subsystem might use
> extend operation. I will rather accept redundancy and consider
> consolidation later when there are two clients. Rather keep the TPM
> provided function simple and stupid.

I just found that your requirement does not match Mimi's requirements:

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

Agreed, there should be a clear division.

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

See https://lkml.org/lkml/2018/11/19/603.


Also, if callers should be able to retrieve the list of supported
algorithms from the TPM, I should move the tpm_chip definition to
include/linux/tpm.h or introduce a new function that returns them.

However, you said that this shouldn't be done now:

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

See https://lkml.org/lkml/2018/11/13/1059.

How we should proceed?

Thanks

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-21  8:11         ` Roberto Sassu
@ 2019-01-21 12:30           ` Jarkko Sakkinen
  0 siblings, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-21 12:30 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On Mon, Jan 21, 2019 at 09:11:21AM +0100, Roberto Sassu wrote:
> On 1/18/2019 4:12 PM, Jarkko Sakkinen wrote:
> > On Thu, Jan 17, 2019 at 08:59:00AM +0100, Roberto Sassu wrote:
> > > On 12/20/2018 4:21 PM, Jarkko Sakkinen wrote:
> > > > On Thu, Dec 13, 2018 at 11:29:45AM +0100, Roberto Sassu wrote:
> > > > > Currently, tpm_pcr_extend() accepts as an input only a SHA1 digest.
> > > > > 
> > > > > This patch modifies the definition of tpm_pcr_extend() to allow other
> > > > > kernel subsystems to pass a digest for each algorithm supported by the TPM.
> > > > > All digests are processed by the TPM in one operation.
> > > > > 
> > > > > If a tpm_pcr_extend() caller provides a subset of the supported algorithms,
> > > > > the TPM driver extends the remaining PCR banks with the first digest
> > > > > passed as an argument to the function.
> > > > > 
> > > > > The new tpm_extend digest structure has been preferred to the tpm_digest
> > > > > structure, to let the caller specify the size of the digest (which may be
> > > > > unknown to the TPM driver).
> > > > > 
> > > > > Due to the API change, ima_pcr_extend() and pcrlock() have been modified.
> > > > > 
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >    drivers/char/tpm/tpm-interface.c   | 24 +++++---------------
> > > > >    drivers/char/tpm/tpm.h             |  5 +++--
> > > > >    drivers/char/tpm/tpm1-cmd.c        | 13 ++++++++---
> > > > >    drivers/char/tpm/tpm2-cmd.c        | 35 +++++++++++++++++++++---------
> > > > >    include/linux/tpm.h                | 13 ++++++++---
> > > > >    security/integrity/ima/ima_queue.c |  5 ++++-
> > > > >    security/keys/trusted.c            |  5 ++++-
> > > > >    7 files changed, 62 insertions(+), 38 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > > > index eb7c79ca8a94..911fea19e408 100644
> > > > > --- a/drivers/char/tpm/tpm-interface.c
> > > > > +++ b/drivers/char/tpm/tpm-interface.c
> > > > > @@ -478,42 +478,30 @@ EXPORT_SYMBOL_GPL(tpm_pcr_read);
> > > > >     * tpm_pcr_extend - extend a PCR value in SHA1 bank.
> > > > >     * @chip:	a &struct tpm_chip instance, %NULL for the default chip
> > > > >     * @pcr_idx:	the PCR to be retrieved
> > > > > - * @hash:	the hash value used to extend the PCR value
> > > > > + * @count:	number of tpm_extend_digest structures
> > > > > + * @digests:	array of tpm_extend_digest structures used to extend PCRs
> > > > >     *
> > > > >     * Note: with TPM 2.0 extends also those banks for which no digest was
> > > > >     * specified in order to prevent malicious use of those PCR banks.
> > > > >     *
> > > > >     * Return: same as with tpm_transmit_cmd()
> > > > >     */
> > > > > -int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash)
> > > > > +int tpm_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > > +		   const struct tpm_extend_digest *digests)
> > > > 
> > > > Remove const. Document how @digests is used  like the special meaning
> > > > of the first index. I faintly remember asking this last time.
> > > > 
> > > > >    {
> > > > >    	int rc;
> > > > > -	struct tpm_digest *digest_list;
> > > > > -	int i;
> > > > >    	chip = tpm_find_get_ops(chip);
> > > > >    	if (!chip)
> > > > >    		return -ENODEV;
> > > > >    	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> > > > > -		digest_list = kcalloc(chip->nr_allocated_banks,
> > > > > -				      sizeof(*digest_list), GFP_KERNEL);
> > > > > -		if (!digest_list)
> > > > > -			return -ENOMEM;
> > > > > -
> > > > > -		for (i = 0; i < chip->nr_allocated_banks; i++) {
> > > > > -			digest_list[i].alg_id = chip->allocated_banks[i].alg_id;
> > > > > -			memcpy(digest_list[i].digest, hash, TPM_DIGEST_SIZE);
> > > > > -		}
> > > > > -
> > > > > -		rc = tpm2_pcr_extend(chip, pcr_idx, chip->nr_allocated_banks,
> > > > > -				     digest_list);
> > > > > -		kfree(digest_list);
> > > > > +		rc = tpm2_pcr_extend(chip, pcr_idx, count, digests);
> > > > >    		tpm_put_ops(chip);
> > > > >    		return rc;
> > > > >    	}
> > > > > -	rc = tpm1_pcr_extend(chip, pcr_idx, hash,
> > > > > +	rc = tpm1_pcr_extend(chip, pcr_idx, count, digests,
> > > > >    			     "attempting extend a PCR value");
> > > > 
> > > > The validation is missing that the provided array has only one element
> > > > and the algorithm is SHA1. Could be done also inside tpm1_pcr_extend()
> > > > but what you are doing to that function does not make any sense so
> > > > better to do it here.
> > > > 
> > > > >    	tpm_put_ops(chip);
> > > > >    	return rc;
> > > > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > > > index 64d93d26087f..6b446504d2fe 100644
> > > > > --- a/drivers/char/tpm/tpm.h
> > > > > +++ b/drivers/char/tpm/tpm.h
> > > > > @@ -504,7 +504,8 @@ int tpm1_auto_startup(struct tpm_chip *chip);
> > > > >    int tpm1_do_selftest(struct tpm_chip *chip);
> > > > >    int tpm1_get_timeouts(struct tpm_chip *chip);
> > > > >    unsigned long tpm1_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal);
> > > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> > > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > > +		    const struct tpm_extend_digest *digests,
> > > > >    		    const char *log_msg);
> > > > >    int tpm1_pcr_read(struct tpm_chip *chip, u32 pcr_idx, u8 *res_buf);
> > > > >    ssize_t tpm1_getcap(struct tpm_chip *chip, u32 subcap_id, cap_t *cap,
> > > > > @@ -551,7 +552,7 @@ int tpm2_get_timeouts(struct tpm_chip *chip);
> > > > >    int tpm2_pcr_read(struct tpm_chip *chip, u32 pcr_idx,
> > > > >    		  struct tpm_digest *digest, u16 *digest_size_ptr);
> > > > >    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > > -		    struct tpm_digest *digests);
> > > > > +		    const struct tpm_extend_digest *digests);
> > > > >    int tpm2_get_random(struct tpm_chip *chip, u8 *dest, size_t max);
> > > > >    void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
> > > > >    			    unsigned int flags);
> > > > > diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
> > > > > index 8b70a7f884a7..04ee10284b8c 100644
> > > > > --- a/drivers/char/tpm/tpm1-cmd.c
> > > > > +++ b/drivers/char/tpm/tpm1-cmd.c
> > > > > @@ -449,12 +449,20 @@ int tpm1_get_timeouts(struct tpm_chip *chip)
> > > > >    }
> > > > >    #define TPM_ORD_PCR_EXTEND 20
> > > > > -int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, const u8 *hash,
> > > > > +int tpm1_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > > +		    const struct tpm_extend_digest *digests,
> > > > >    		    const char *log_msg)
> > > > >    {
> > > > >    	struct tpm_buf buf;
> > > > > +	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> > > > > +	const u8 *hash;
> > > > >    	int rc;
> > > > > +	hash = dummy_hash;
> > > > > +	if (count)
> > > > > +		memcpy(dummy_hash, digests[0].data,
> > > > > +		       min(digests[0].size, (u16)sizeof(dummy_hash)));
> > > > > +
> > > > 
> > > > You copy memory from one place to another without any good reason to do
> > > > so. My suggestion is just not to change tpm1_pcr_extend() at all.
> > > > 
> > > > >    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_PCR_EXTEND);
> > > > >    	if (rc)
> > > > >    		return rc;
> > > > > @@ -743,7 +751,6 @@ int tpm1_auto_startup(struct tpm_chip *chip)
> > > > >     */
> > > > >    int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
> > > > >    {
> > > > > -	u8 dummy_hash[TPM_DIGEST_SIZE] = { 0 };
> > > > >    	struct tpm_buf buf;
> > > > >    	unsigned int try;
> > > > >    	int rc;
> > > > > @@ -751,7 +758,7 @@ int tpm1_pm_suspend(struct tpm_chip *chip, u32 tpm_suspend_pcr)
> > > > >    	/* for buggy tpm, flush pcrs with extend to selected dummy */
> > > > >    	if (tpm_suspend_pcr)
> > > > > -		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, dummy_hash,
> > > > > +		rc = tpm1_pcr_extend(chip, tpm_suspend_pcr, 0, NULL,
> > > > >    				     "extending dummy pcr before suspend");
> > > > >    	rc = tpm_buf_init(&buf, TPM_TAG_RQU_COMMAND, TPM_ORD_SAVESTATE);
> > > > > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> > > > > index 6ce5173cf0e5..77b5808270c6 100644
> > > > > --- a/drivers/char/tpm/tpm2-cmd.c
> > > > > +++ b/drivers/char/tpm/tpm2-cmd.c
> > > > > @@ -247,21 +247,22 @@ struct tpm2_null_auth_area {
> > > > >     *
> > > > >     * @chip:	TPM chip to use.
> > > > >     * @pcr_idx:	index of the PCR.
> > > > > - * @count:	number of digests passed.
> > > > > - * @digests:	list of pcr banks and corresponding digest values to extend.
> > > > > + * @count:	number of tpm_extend_digest passed.
> > > > > + * @digests:	array of tpm_extend_digest with digest values to extend.
> > > > >     *
> > > > >     * Return: Same as with tpm_transmit_cmd.
> > > > >     */
> > > > 
> > > > The documentation about @digests.
> > > > 
> > > > >    int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > > -		    struct tpm_digest *digests)
> > > > > +		    const struct tpm_extend_digest *digests)
> > > > >    {
> > > > >    	struct tpm_buf buf;
> > > > >    	struct tpm2_null_auth_area auth_area;
> > > > > +	const struct tpm_extend_digest *digest;
> > > > > +	u8 dummy_hash[SHA512_DIGEST_SIZE] = { 0 };
> > > > > +	const u8 *hash;
> > > > >    	int rc;
> > > > >    	int i;
> > > > > -
> > > > > -	if (count > chip->nr_allocated_banks)
> > > > > -		return -EINVAL;
> > > > > +	int j;
> > > > >    	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
> > > > >    	if (rc)
> > > > > @@ -277,11 +278,25 @@ int tpm2_pcr_extend(struct tpm_chip *chip, u32 pcr_idx, u32 count,
> > > > >    	tpm_buf_append_u32(&buf, sizeof(struct tpm2_null_auth_area));
> > > > >    	tpm_buf_append(&buf, (const unsigned char *)&auth_area,
> > > > >    		       sizeof(auth_area));
> > > > > -	tpm_buf_append_u32(&buf, count);
> > > > > +	tpm_buf_append_u32(&buf, chip->nr_allocated_banks);
> > > > > +
> > > > > +	if (count)
> > > > > +		memcpy(dummy_hash, digests[0].data, digests[0].size);
> > > > > +
> > > > > +	for (i = 0; i < chip->nr_allocated_banks; i++) {
> > > > > +		tpm_buf_append_u16(&buf, chip->allocated_banks[i].alg_id);
> > > > > +
> > > > > +		hash = dummy_hash;
> > > > > +		for (j = 0; j < count; j++) {
> > > > > +			digest = digests + j;
> > > > > +
> > > > > +			if (digest->alg_id == chip->allocated_banks[i].alg_id) {
> > > > > +				hash = digest->data;
> > > > > +				break;
> > > > 
> > > > I think the whole design is just wrong. I did re-read your response to
> > > > v6 again and I'm very sorry, but I just don't get this. Caller has all
> > > > the information (from struct tpm_chip) to give the correct data. This
> > > > function should validate that data (check algorithm ID and that's it).
> > > 
> > > The question is if checking tpm->allocated_banks is a strict
> > > requirement, or we can allow callers to use the algorithm they are
> > > currently using, without further modifications.
> > 
> > If you want to know what is available, you can use that array.
> > 
> > > > Extending with the dummy hash should be done by the caller when
> > > > preparing the array, not baked into this function. This kind of also
> > > > makes obvious that we don't need this new struct. There should never be
> > > > a local variable (whose size is BTW randomly chosen) called dummy_hash.
> > > 
> > > This means duplicating the code for each caller. Currently, this work is
> > > done by the TPM driver.
> > 
> > It is better than fix the usage pattern like this. Not only that, but
> > this somewhat complicated behavior now completely undocumented in the
> > function description.
> > 
> > You are making strong assumptions how some other subsystem might use
> > extend operation. I will rather accept redundancy and consider
> > consolidation later when there are two clients. Rather keep the TPM
> > provided function simple and stupid.
> 
> Ok, if I understood it correctly, every caller of tpm_pcr_extend() will pass
> an array of tpm_digest structures.
> 
> I will not introduce a new structure, as the caller has to use
> algorithms from chip->allocated_banks and passing the digest size is not
> necessary.
> 
> tpm_pcr_extend() will reject incomplete arrays (one or more algorithms
> are missing). To simplify even more tpm_pcr_extend(), I assume and check
> that passed algorithms are in the same order as those in
> chip->allocated_banks.
> 
> I will remove the 'count' parameter from tpm_pcr_extend(), as the number
> of tpm_digest structures will be always chip->nr_allocated_banks.

Sounds like a plan. I think some redundancy is almost always better than
hard to undestand API.

/Jarkko

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-21  9:58         ` Roberto Sassu
@ 2019-01-21 12:37           ` Jarkko Sakkinen
  2019-01-21 13:50             ` Roberto Sassu
  0 siblings, 1 reply; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-21 12:37 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On Mon, Jan 21, 2019 at 10:58:22AM +0100, Roberto Sassu wrote:
> Agreed, there should be a clear division.
> 
> 1) The caller shouldn't need to know anything about the chip->info.
> 2) The TPM driver should not rely on the caller to supply all the
> hashes, but verify that all allocated banks are being extended.

1. It is just plain wrong if the basic extend function
   invents its own dummy hashes when it doesn't get one. In the end,
   this is what matters to me, and I'm not going to accept anything that
   tries to do that. That is something that caller should prepare.

   You even left undocumented this very awkward and unguessable
   behaviour. I think redundancy is better than doing guesswork what
   a function might possibly do other than its basic task, which is
   constructing the extend command.
2. The driver should return -EINVAL, if it doesn't get all the hashes.
3. The would be nothing wrong exposing struct tpm_chip in
   include/linux/tpm.h. I would be totally fine with that.

/Jarkko

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-21 12:37           ` Jarkko Sakkinen
@ 2019-01-21 13:50             ` Roberto Sassu
  2019-01-22 16:55               ` Jarkko Sakkinen
  0 siblings, 1 reply; 22+ messages in thread
From: Roberto Sassu @ 2019-01-21 13:50 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On 1/21/2019 1:37 PM, Jarkko Sakkinen wrote:
> 3. The would be nothing wrong exposing struct tpm_chip in
>     include/linux/tpm.h. I would be totally fine with that.

Should I do it in a separate patch (before 5/5)?

Is it fine to call tpm_default_chip() only in pcrlock() for trusted
keys?

Thanks

Roberto


> /Jarkko
> 

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

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

* Re: [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend()
  2019-01-21 13:50             ` Roberto Sassu
@ 2019-01-22 16:55               ` Jarkko Sakkinen
  0 siblings, 0 replies; 22+ messages in thread
From: Jarkko Sakkinen @ 2019-01-22 16:55 UTC (permalink / raw)
  To: Roberto Sassu
  Cc: zohar, david.safford, monty.wiseman, linux-integrity,
	linux-security-module, linux-kernel, silviu.vlasceanu,
	Matthew Garrett

On Mon, Jan 21, 2019 at 02:50:49PM +0100, Roberto Sassu wrote:
> On 1/21/2019 1:37 PM, Jarkko Sakkinen wrote:
> > 3. The would be nothing wrong exposing struct tpm_chip in
> >     include/linux/tpm.h. I would be totally fine with that.
> 
> Should I do it in a separate patch (before 5/5)?

Yes.

> Is it fine to call tpm_default_chip() only in pcrlock() for trusted
> keys?

I think you should get the reference in init_trusted() (similar pattern
as in IMA).

/Jarkko

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

end of thread, other threads:[~2019-01-22 16:55 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-13 10:29 [PATCH v7 0/5] tpm: retrieve digest size of unknown algorithms from TPM Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 1/5] tpm: dynamically allocate the allocated_banks array Roberto Sassu
2018-12-20 14:55   ` Jarkko Sakkinen
2018-12-21  9:40     ` Roberto Sassu
2018-12-22  0:03       ` Jarkko Sakkinen
2019-01-07 10:06         ` Roberto Sassu
2019-01-10 17:38           ` Jarkko Sakkinen
2019-01-11  7:53             ` Roberto Sassu
2019-01-11 16:35               ` Jarkko Sakkinen
2018-12-13 10:29 ` [PATCH v7 2/5] tpm: add _head suffix to tcg_efi_specid_event and tcg_pcr_event2 Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 3/5] tpm: rename and export tpm2_digest and tpm2_algorithms Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 4/5] tpm: retrieve digest size of unknown algorithms with PCR read Roberto Sassu
2018-12-13 10:29 ` [PATCH v7 5/5] tpm: pass an array of tpm_extend_digest structures to tpm_pcr_extend() Roberto Sassu
2018-12-20 15:21   ` Jarkko Sakkinen
2019-01-17  7:59     ` Roberto Sassu
2019-01-18 15:12       ` Jarkko Sakkinen
2019-01-21  8:11         ` Roberto Sassu
2019-01-21 12:30           ` Jarkko Sakkinen
2019-01-21  9:58         ` Roberto Sassu
2019-01-21 12:37           ` Jarkko Sakkinen
2019-01-21 13:50             ` Roberto Sassu
2019-01-22 16:55               ` Jarkko Sakkinen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).