[PATCH v6 1/3] efi_loader: Add check for event log passed from firmware

[PATCH v6 1/3] efi_loader: Add check for event log passed from firmware

[Ruchika Gupta]

Platforms may have support to measure their initial firmware components
and pass the event log to u-boot. The event log address can be passed
in property tpm_event_log_addr and tpm_event_log_size of the tpm node.
Platforms may choose their own specific mechanism to do so. A weak
function is added to check if even log has been passed to u-boot
from earlier firmware components. If available, the eventlog is parsed
to check for its correctness and further event logs are appended to the
passed log.

Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v6: No change

v5:
Shift the efi_init_event_log() to a different location in the file.
This help fixes compilation issue introduced by calling efi_append_scrtm_version()
from it.

v4:
Add SCRTM version to log only if previous firmware doesn't pass the eventlog

v3:
Return as soon as you detect error

v2:
Moved firmware eventlog code parsing to tcg2_get_fw_eventlog()

 lib/efi_loader/efi_tcg2.c | 438 ++++++++++++++++++++++++++++++++------
 1 file changed, 369 insertions(+), 69 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 8c1f22e337..a789c44660 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -324,6 +324,45 @@ __weak efi_status_t platform_get_tpm2_device(struct udevice **dev)
 	return EFI_NOT_FOUND;
 }
 
+/**
+ * platform_get_eventlog() - retrieve the eventlog address and size
+ *
+ * This function retrieves the eventlog address and size if the underlying
+ * firmware has done some measurements and passed them.
+ *
+ * This function may be overridden based on platform specific method of
+ * passing the eventlog address and size.
+ *
+ * @dev:	udevice
+ * @addr:	eventlog address
+ * @sz:		eventlog size
+ * Return:	status code
+ */
+__weak efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr,
+					  u32 *sz)
+{
+	const u64 *basep;
+	const u32 *sizep;
+
+	basep = dev_read_prop(dev, "tpm_event_log_addr", NULL);
+	if (!basep)
+		return EFI_NOT_FOUND;
+
+	*addr = be64_to_cpup((__force __be64 *)basep);
+
+	sizep = dev_read_prop(dev, "tpm_event_log_size", NULL);
+	if (!sizep)
+		return EFI_NOT_FOUND;
+
+	*sz = be32_to_cpup((__force __be32 *)sizep);
+	if (*sz == 0) {
+		log_debug("event log empty\n");
+		return EFI_NOT_FOUND;
+	}
+
+	return EFI_SUCCESS;
+}
+
 /**
  * tpm2_get_max_command_size() - get the supported max command size
  *
@@ -1181,6 +1220,250 @@ static const struct efi_tcg2_protocol efi_tcg2_protocol = {
 	.get_result_of_set_active_pcr_banks = efi_tcg2_get_result_of_set_active_pcr_banks,
 };
 
+/**
+ * parse_event_log_header() -  Parse and verify the event log header fields
+ *
+ * @buffer:			Pointer to the event header
+ * @size:			Size of the eventlog
+ * @pos:			Position in buffer after event log header
+ *
+ * Return:	status code
+ */
+efi_status_t parse_event_log_header(void *buffer, u32 size, u32 *pos)
+{
+	struct tcg_pcr_event *event_header = (struct tcg_pcr_event *)buffer;
+	int i = 0;
+
+	if (size < sizeof(*event_header))
+		return EFI_COMPROMISED_DATA;
+
+	if (get_unaligned_le32(&event_header->pcr_index) != 0 ||
+	    get_unaligned_le32(&event_header->event_type) != EV_NO_ACTION)
+		return EFI_COMPROMISED_DATA;
+
+	for (i = 0; i < sizeof(event_header->digest); i++) {
+		if (event_header->digest[i] != 0)
+			return EFI_COMPROMISED_DATA;
+	}
+
+	*pos += sizeof(*event_header);
+
+	return EFI_SUCCESS;
+}
+
+/**
+ * parse_specid_event() -  Parse and verify the specID Event in the eventlog
+ *
+ * @dev:		udevice
+ * @buffer:		Pointer to the start of the eventlog
+ * @log_size:		Size of the eventlog
+ * @pos:		Offset in the evenlog where specID event starts
+ *
+ * Return:		status code
+ * @pos			Offset in the eventlog where the specID event ends
+ * @digest_list:	list of digests in the event
+ */
+efi_status_t parse_specid_event(struct udevice *dev, void *buffer, u32 log_size,
+				u32 *pos,
+				struct tpml_digest_values *digest_list)
+{
+	struct tcg_efi_spec_id_event *spec_event;
+	struct tcg_pcr_event *event_header = (struct tcg_pcr_event *)buffer;
+	size_t spec_event_size;
+	u32 active = 0, supported = 0, pcr_count = 0, alg_count = 0;
+	u32 spec_active = 0;
+	u16 hash_alg, hash_sz;
+	u8 vendor_sz;
+	int err, i;
+
+	/* Check specID event data */
+	spec_event = (struct tcg_efi_spec_id_event *)((uintptr_t)buffer + *pos);
+	/* Check for signature */
+	if (memcmp(spec_event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
+		   sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03))) {
+		log_err("specID Event: Signature mismatch\n");
+		return EFI_COMPROMISED_DATA;
+	}
+
+	if (spec_event->spec_version_minor !=
+			TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
+	    spec_event->spec_version_major !=
+			TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
+		return EFI_COMPROMISED_DATA;
+
+	if (spec_event->number_of_algorithms > MAX_HASH_COUNT ||
+	    spec_event->number_of_algorithms < 1) {
+		log_err("specID Event: Number of algorithms incorrect\n");
+		return EFI_COMPROMISED_DATA;
+	}
+
+	alg_count = spec_event->number_of_algorithms;
+
+	err = tpm2_get_pcr_info(dev, &supported, &active, &pcr_count);
+	if (err)
+		return EFI_DEVICE_ERROR;
+
+	digest_list->count = 0;
+	/*
+	 * We may need to worry about the order of algs in this structure as
+	 * subsequent entries in event should be in same order
+	 */
+	for (i = 0; i < alg_count; i++) {
+		hash_alg =
+		  get_unaligned_le16(&spec_event->digest_sizes[i].algorithm_id);
+		hash_sz =
+		   get_unaligned_le16(&spec_event->digest_sizes[i].digest_size);
+
+		if (!(supported & alg_to_mask(hash_alg))) {
+			log_err("specID Event: Unsupported algorithm\n");
+			return EFI_COMPROMISED_DATA;
+		}
+		digest_list->digests[digest_list->count++].hash_alg = hash_alg;
+
+		spec_active |= alg_to_mask(hash_alg);
+	}
+
+	/* TCG spec expects the event log to have hashes for all active PCR's */
+	if (spec_active != active) {
+		/*
+		 * Previous stage bootloader should know all the active PCR's
+		 * and use them in the Eventlog.
+		 */
+		log_err("specID Event: All active hash alg not present\n");
+		return EFI_COMPROMISED_DATA;
+	}
+
+	/*
+	 * the size of the spec event and placement of vendor_info_size
+	 * depends on supported algoriths
+	 */
+	spec_event_size =
+		offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
+		alg_count * sizeof(spec_event->digest_sizes[0]);
+
+	vendor_sz = *(uint8_t *)((uintptr_t)buffer + *pos + spec_event_size);
+
+	spec_event_size += sizeof(vendor_sz) + vendor_sz;
+	*pos += spec_event_size;
+
+	if (get_unaligned_le32(&event_header->event_size) != spec_event_size) {
+		log_err("specID event: header event size mismatch\n");
+		/* Right way to handle this can be to call SetActive PCR's */
+		return EFI_COMPROMISED_DATA;
+	}
+
+	return EFI_SUCCESS;
+}
+
+efi_status_t tcg2_parse_event(struct udevice *dev, void *buffer, u32 log_size,
+			      u32 *offset, struct tpml_digest_values *digest_list,
+			      u32 *pcr)
+{
+	struct tcg_pcr_event2 *event = NULL;
+	u32 event_type, count, size, event_size;
+	size_t pos;
+
+	if (*offset > log_size)
+		return EFI_COMPROMISED_DATA;
+
+	event = (struct tcg_pcr_event2 *)((uintptr_t)buffer + *offset);
+
+	*pcr = get_unaligned_le32(&event->pcr_index);
+
+	event_size = tcg_event_final_size(digest_list);
+
+	if (*offset + event_size > log_size) {
+		log_err("Event exceeds log size\n");
+		return EFI_COMPROMISED_DATA;
+	}
+
+	event_type = get_unaligned_le32(&event->event_type);
+
+	/* get the count */
+	count = get_unaligned_le32(&event->digests.count);
+	if (count != digest_list->count)
+		return EFI_COMPROMISED_DATA;
+
+	pos = offsetof(struct tcg_pcr_event2, digests);
+	pos += offsetof(struct tpml_digest_values, digests);
+
+	for (int i = 0; i < digest_list->count; i++) {
+		u16 alg;
+		u16 hash_alg = digest_list->digests[i].hash_alg;
+		u8 *digest = (u8 *)&digest_list->digests[i].digest;
+
+		alg = get_unaligned_le16((void *)((uintptr_t)event + pos));
+
+		if (alg != hash_alg)
+			return EFI_COMPROMISED_DATA;
+
+		pos += offsetof(struct tpmt_ha, digest);
+		memcpy(digest, (void *)((uintptr_t)event + pos), alg_to_len(hash_alg));
+		pos += alg_to_len(hash_alg);
+	}
+
+	size = get_unaligned_le32((void *)((uintptr_t)event + pos));
+	event_size += size;
+	pos += sizeof(u32); /* tcg_pcr_event2 event_size*/
+	pos += size;
+
+	/* make sure the calculated buffer is what we checked against */
+	if (pos != event_size)
+		return EFI_COMPROMISED_DATA;
+
+	if (pos > log_size)
+		return EFI_COMPROMISED_DATA;
+
+	*offset += pos;
+
+	return EFI_SUCCESS;
+}
+
+efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
+				  size_t *log_sz)
+{
+	struct tpml_digest_values digest_list;
+	void *buffer;
+	efi_status_t ret;
+	u32 pcr, pos;
+	u64 base;
+	u32 sz;
+
+	ret = platform_get_eventlog(dev, &base, &sz);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	if (sz > TPM2_EVENT_LOG_SIZE)
+		return EFI_VOLUME_FULL;
+
+	buffer = (void *)base;
+	pos = 0;
+	/* Parse the eventlog to check for its validity */
+	ret = parse_event_log_header(buffer, sz, &pos);
+	if (ret || pos > sz)
+		return EFI_COMPROMISED_DATA;
+
+	ret = parse_specid_event(dev, buffer, sz, &pos, &digest_list);
+	if (ret || pos > sz) {
+		log_err("Error parsing SPEC ID Event\n");
+		return EFI_COMPROMISED_DATA;
+	}
+
+	while (pos < sz) {
+		ret = tcg2_parse_event(dev, buffer, sz, &pos, &digest_list,
+				       &pcr);
+		if (ret) {
+			log_err("Error parsing event\n");
+			return ret;
+		}
+	}
+
+	memcpy(log_buffer, buffer, sz);
+	*log_sz = sz;
+
+	return ret;
+}
+
 /**
  * create_specid_event() - Create the first event in the eventlog
  *
@@ -1312,69 +1595,6 @@ out:
 	return ret;
 }
 
-/**
- * efi_init_event_log() - initialize an eventlog
- */
-static efi_status_t efi_init_event_log(void)
-{
-	/*
-	 * vendor_info_size is currently set to 0, we need to change the length
-	 * and allocate the flexible array member if this changes
-	 */
-	struct tcg_pcr_event *event_header = NULL;
-	struct udevice *dev;
-	size_t spec_event_size;
-	efi_status_t ret;
-
-	ret = platform_get_tpm2_device(&dev);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, TPM2_EVENT_LOG_SIZE,
-				(void **)&event_log.buffer);
-	if (ret != EFI_SUCCESS)
-		goto out;
-
-	/*
-	 * initialize log area as 0xff so the OS can easily figure out the
-	 * last log entry
-	 */
-	memset(event_log.buffer, 0xff, TPM2_EVENT_LOG_SIZE);
-	event_log.pos = 0;
-	event_log.last_event_size = 0;
-	event_log.get_event_called = false;
-	event_log.ebs_called = false;
-	event_log.truncated = false;
-
-	/*
-	 * The log header is defined to be in SHA1 event log entry format.
-	 * Setup event header
-	 */
-	event_header =  (struct tcg_pcr_event *)event_log.buffer;
-	put_unaligned_le32(0, &event_header->pcr_index);
-	put_unaligned_le32(EV_NO_ACTION, &event_header->event_type);
-	memset(&event_header->digest, 0, sizeof(event_header->digest));
-	ret = create_specid_event(dev, (void *)((uintptr_t)event_log.buffer + sizeof(*event_header)),
-				  &spec_event_size);
-	if (ret != EFI_SUCCESS)
-		goto free_pool;
-	put_unaligned_le32(spec_event_size, &event_header->event_size);
-	event_log.pos = spec_event_size + sizeof(*event_header);
-	event_log.last_event_size = event_log.pos;
-
-	ret = create_final_event();
-	if (ret != EFI_SUCCESS)
-		goto free_pool;
-
-out:
-	return ret;
-
-free_pool:
-	efi_free_pool(event_log.buffer);
-	event_log.buffer = NULL;
-	return ret;
-}
-
 /**
  * tcg2_measure_event() - common function to add event log and extend PCR
  *
@@ -1427,6 +1647,92 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev)
 	return ret;
 }
 
+/**
+ * efi_init_event_log() - initialize an eventlog
+ */
+static efi_status_t efi_init_event_log(void)
+{
+	/*
+	 * vendor_info_size is currently set to 0, we need to change the length
+	 * and allocate the flexible array member if this changes
+	 */
+	struct tcg_pcr_event *event_header = NULL;
+	struct udevice *dev;
+	size_t spec_event_size;
+	efi_status_t ret;
+
+	ret = platform_get_tpm2_device(&dev);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, TPM2_EVENT_LOG_SIZE,
+				(void **)&event_log.buffer);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	/*
+	 * initialize log area as 0xff so the OS can easily figure out the
+	 * last log entry
+	 */
+	memset(event_log.buffer, 0xff, TPM2_EVENT_LOG_SIZE);
+
+	/*
+	 * The log header is defined to be in SHA1 event log entry format.
+	 * Setup event header
+	 */
+	event_header =  (struct tcg_pcr_event *)event_log.buffer;
+	event_log.pos = 0;
+	event_log.last_event_size = 0;
+	event_log.get_event_called = false;
+	event_log.ebs_called = false;
+	event_log.truncated = false;
+
+	/*
+	 * Check if earlier firmware have passed any eventlog. Different
+	 * platforms can use different ways to do so
+	 */
+	ret = tcg2_get_fw_eventlog(dev, event_log.buffer, &event_log.pos);
+	/*
+	 * If earlier firmware hasn't passed any eventlog, go ahead and
+	 * create the eventlog header
+	 */
+	if (ret == EFI_NOT_FOUND) {
+		put_unaligned_le32(0, &event_header->pcr_index);
+		put_unaligned_le32(EV_NO_ACTION, &event_header->event_type);
+		memset(&event_header->digest, 0, sizeof(event_header->digest));
+		ret = create_specid_event(dev,
+					  (void *)((uintptr_t)event_log.buffer +
+						   sizeof(*event_header)),
+					  &spec_event_size);
+		if (ret != EFI_SUCCESS)
+			goto free_pool;
+		put_unaligned_le32(spec_event_size, &event_header->event_size);
+		event_log.pos = spec_event_size + sizeof(*event_header);
+		event_log.last_event_size = event_log.pos;
+
+		/*
+		 * Add SCRTM version to the log if previous firmmware
+		 * doesn't pass an eventlog
+		 */
+		ret = efi_append_scrtm_version(dev);
+	}
+
+	if (ret != EFI_SUCCESS)
+		goto free_pool;
+
+	ret = create_final_event();
+	if (ret != EFI_SUCCESS)
+		goto free_pool;
+
+out:
+	return ret;
+
+free_pool:
+	efi_free_pool(event_log.buffer);
+	event_log.buffer = NULL;
+	return ret;
+}
+
 /**
  * tcg2_measure_variable() - add variable event log and extend PCR
  *
@@ -1963,12 +2269,6 @@ efi_status_t efi_tcg2_register(void)
 	if (ret != EFI_SUCCESS)
 		goto fail;
 
-	ret = efi_append_scrtm_version(dev);
-	if (ret != EFI_SUCCESS) {
-		tcg2_uninit();
-		goto fail;
-	}
-
 	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
 			       (void *)&efi_tcg2_protocol);
 	if (ret != EFI_SUCCESS) {
-- 
2.25.1

[PATCH v6 2/3] tpm: use more algorithms than sha256 on pcr_read

[Ruchika Gupta]

The current tpm2_pcr_read is hardcoded using SHA256. Make the
actual command to TPM configurable to use wider range of algorithms.
The current command line is kept as is i.e limited to SHA-256 only.

Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v6: No change

v5: No change

v4: No change

v3: No change

v2:
Change algorithm from u32 to u16
Add parameter description in function declaration

 cmd/tpm-v2.c     |  3 ++-
 include/tpm-v2.h |  5 ++++-
 lib/tpm-v2.c     | 12 ++++++++----
 3 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/cmd/tpm-v2.c b/cmd/tpm-v2.c
index daae91100a..4ea5f9f094 100644
--- a/cmd/tpm-v2.c
+++ b/cmd/tpm-v2.c
@@ -151,7 +151,8 @@ static int do_tpm_pcr_read(struct cmd_tbl *cmdtp, int flag, int argc,
 
 	data = map_sysmem(simple_strtoul(argv[2], NULL, 0), 0);
 
-	rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, data, &updates);
+	rc = tpm2_pcr_read(dev, index, priv->pcr_select_min, TPM2_ALG_SHA256,
+			   data, TPM2_DIGEST_LEN, &updates);
 	if (!rc) {
 		printf("PCR #%u content (%u known updates):\n", index, updates);
 		print_byte_string(data, TPM2_DIGEST_LEN);
diff --git a/include/tpm-v2.h b/include/tpm-v2.h
index ceff7d245e..4e9dd52cb6 100644
--- a/include/tpm-v2.h
+++ b/include/tpm-v2.h
@@ -512,13 +512,16 @@ u32 tpm2_nv_write_value(struct udevice *dev, u32 index, const void *data,
  * @dev		TPM device
  * @idx		Index of the PCR
  * @idx_min_sz	Minimum size in bytes of the pcrSelect array
+ * @algorithm	Algorithm used, defined in 'enum tpm2_algorithms'
  * @data	Output buffer for contents of the named PCR
+ * @digest_len  len of the data
  * @updates	Optional out parameter: number of updates for this PCR
  *
  * @return code of the operation
  */
 u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz,
-		  void *data, unsigned int *updates);
+		  u16 algorithm, void *data, u32 digest_len,
+		  unsigned int *updates);
 
 /**
  * Issue a TPM2_GetCapability command.  This implementation is limited
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 2e7b27bd6b..1bf627853a 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -254,7 +254,8 @@ u32 tpm2_nv_write_value(struct udevice *dev, u32 index, const void *data,
 }
 
 u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz,
-		  void *data, unsigned int *updates)
+		  u16 algorithm, void *data, u32 digest_len,
+		  unsigned int *updates)
 {
 	u8 idx_array_sz = max(idx_min_sz, DIV_ROUND_UP(idx, 8));
 	u8 command_v2[COMMAND_BUFFER_SIZE] = {
@@ -264,7 +265,7 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz,
 
 		/* TPML_PCR_SELECTION */
 		tpm_u32(1),			/* Number of selections */
-		tpm_u16(TPM2_ALG_SHA256),	/* Algorithm of the hash */
+		tpm_u16(algorithm),		/* Algorithm of the hash */
 		idx_array_sz,			/* Array size for selection */
 		/* bitmap(idx)			   Selected PCR bitmap */
 	};
@@ -283,10 +284,13 @@ u32 tpm2_pcr_read(struct udevice *dev, u32 idx, unsigned int idx_min_sz,
 	if (ret)
 		return ret;
 
+	if (digest_len > response_len)
+		return TPM_LIB_ERROR;
+
 	if (unpack_byte_string(response, response_len, "ds",
 			       10, &counter,
-			       response_len - TPM2_DIGEST_LEN, data,
-			       TPM2_DIGEST_LEN))
+			       response_len - digest_len, data,
+			       digest_len))
 		return TPM_LIB_ERROR;
 
 	if (updates)
-- 
2.25.1

[PATCH v6 3/3] efi_loader: Extend PCR's for firmware measurements

[Ruchika Gupta]

Firmwares before U-Boot may be capable of doing tpm measurements
and passing them to U-Boot in the form of eventlog. However there
may be scenarios where the firmwares don't have TPM driver and
are not capable of extending the measurements in the PCRs.
Based on TCG spec, if previous firnware has extended PCR's, PCR0
would not be 0. So, read the PCR0 to determine if the PCR's need
to be extended as eventlog is parsed or not.

Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
v6: Changed TPM2_DIGEST_LEN to TPM2_SHA512_DIGEST_SIZE

v5 : No change

v4 : No change

v3 : 
Rebase changes on top of changes made in first patch series

v2 : 
Removed check for PCR0 in eventlog

 lib/efi_loader/efi_tcg2.c | 75 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index a789c44660..295070f3d8 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -199,6 +199,43 @@ static efi_status_t tcg2_pcr_extend(struct udevice *dev, u32 pcr_index,
 	return EFI_SUCCESS;
 }
 
+/* tcg2_pcr_read - Read PCRs for a TPM2 device for a given tpml_digest_values
+ *
+ * @dev:		device
+ * @digest_list:	list of digest algorithms to extend
+ *
+ * @Return: status code
+ */
+static efi_status_t tcg2_pcr_read(struct udevice *dev, u32 pcr_index,
+				  struct tpml_digest_values *digest_list)
+{
+	struct tpm_chip_priv *priv;
+	unsigned int updates, pcr_select_min;
+	u32 rc;
+	size_t i;
+
+	priv = dev_get_uclass_priv(dev);
+	if (!priv)
+		return EFI_DEVICE_ERROR;
+
+	pcr_select_min = priv->pcr_select_min;
+
+	for (i = 0; i < digest_list->count; i++) {
+		u16 hash_alg = digest_list->digests[i].hash_alg;
+		u8 *digest = (u8 *)&digest_list->digests[i].digest;
+
+		rc = tpm2_pcr_read(dev, pcr_index, pcr_select_min,
+				   hash_alg, digest, alg_to_len(hash_alg),
+				   &updates);
+		if (rc) {
+			EFI_PRINT("Failed to read PCR\n");
+			return EFI_DEVICE_ERROR;
+		}
+	}
+
+	return EFI_SUCCESS;
+}
+
 /* put_event - Append an agile event to an eventlog
  *
  * @pcr_index:		PCR index
@@ -1428,6 +1465,8 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
 	u32 pcr, pos;
 	u64 base;
 	u32 sz;
+	bool extend_pcr = false;
+	int i;
 
 	ret = platform_get_eventlog(dev, &base, &sz);
 	if (ret != EFI_SUCCESS)
@@ -1449,6 +1488,26 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
 		return EFI_COMPROMISED_DATA;
 	}
 
+	ret = tcg2_pcr_read(dev, 0, &digest_list);
+	if (ret) {
+		log_err("Error reading PCR 0\n");
+		return ret;
+	}
+
+	/*
+	 * If PCR0 is 0, previous firmware didn't have the capability
+	 * to extend the PCR. In this scenario, extend the PCR as
+	 * the eventlog is parsed.
+	 */
+	for (i = 0; i < digest_list.count; i++) {
+		u8 buffer[TPM2_SHA512_DIGEST_SIZE] =  { 0 };
+		u16 hash_alg = digest_list.digests[i].hash_alg;
+
+		if (!memcmp((u8 *)&digest_list.digests[i].digest, buffer,
+			    alg_to_len(hash_alg)))
+			extend_pcr = true;
+	}
+
 	while (pos < sz) {
 		ret = tcg2_parse_event(dev, buffer, sz, &pos, &digest_list,
 				       &pcr);
@@ -1456,6 +1515,22 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
 			log_err("Error parsing event\n");
 			return ret;
 		}
+		if (extend_pcr) {
+			ret = tcg2_pcr_extend(dev, pcr, &digest_list);
+			if (ret != EFI_SUCCESS) {
+				log_err("Error in extending PCR\n");
+				return ret;
+			}
+
+			/* Clear the digest for next event */
+			for (i = 0; i < digest_list.count; i++) {
+				u16 hash_alg = digest_list.digests[i].hash_alg;
+				u8 *digest =
+				   (u8 *)&digest_list.digests[i].digest;
+
+				memset(digest, 0, alg_to_len(hash_alg));
+			}
+		}
 	}
 
 	memcpy(log_buffer, buffer, sz);
-- 
2.25.1

Re: [PATCH v6 1/3] efi_loader: Add check for event log passed from firmware

[Heinrich Schuchardt]

On 11/26/21 06:00, Ruchika Gupta wrote:
> Platforms may have support to measure their initial firmware components
> and pass the event log to u-boot. The event log address can be passed
> in property tpm_event_log_addr and tpm_event_log_size of the tpm node.
> Platforms may choose their own specific mechanism to do so. A weak
> function is added to check if even log has been passed to u-boot
> from earlier firmware components. If available, the eventlog is parsed
> to check for its correctness and further event logs are appended to the
> passed log.
>
> Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
> Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> v6: No change
>
> v5:
> Shift the efi_init_event_log() to a different location in the file.
> This help fixes compilation issue introduced by calling efi_append_scrtm_version()
> from it.
>
> v4:
> Add SCRTM version to log only if previous firmware doesn't pass the eventlog
>
> v3:
> Return as soon as you detect error
>
> v2:
> Moved firmware eventlog code parsing to tcg2_get_fw_eventlog()
>
>   lib/efi_loader/efi_tcg2.c | 438 ++++++++++++++++++++++++++++++++------
>   1 file changed, 369 insertions(+), 69 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 8c1f22e337..a789c44660 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -324,6 +324,45 @@ __weak efi_status_t platform_get_tpm2_device(struct udevice **dev)
>   	return EFI_NOT_FOUND;
>   }
>
> +/**
> + * platform_get_eventlog() - retrieve the eventlog address and size
> + *
> + * This function retrieves the eventlog address and size if the underlying
> + * firmware has done some measurements and passed them.
> + *
> + * This function may be overridden based on platform specific method of
> + * passing the eventlog address and size.
> + *
> + * @dev:	udevice
> + * @addr:	eventlog address
> + * @sz:		eventlog size
> + * Return:	status code
> + */
> +__weak efi_status_t platform_get_eventlog(struct udevice *dev, u64 *addr,
> +					  u32 *sz)

This function must be declared in a header to be overridden.

> +{
> +	const u64 *basep;
> +	const u32 *sizep;
> +
> +	basep = dev_read_prop(dev, "tpm_event_log_addr", NULL);
> +	if (!basep)
> +		return EFI_NOT_FOUND;
> +
> +	*addr = be64_to_cpup((__force __be64 *)basep);
> +
> +	sizep = dev_read_prop(dev, "tpm_event_log_size", NULL);
> +	if (!sizep)
> +		return EFI_NOT_FOUND;
> +
> +	*sz = be32_to_cpup((__force __be32 *)sizep);
> +	if (*sz == 0) {
> +		log_debug("event log empty\n");
> +		return EFI_NOT_FOUND;
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +
>   /**
>    * tpm2_get_max_command_size() - get the supported max command size
>    *
> @@ -1181,6 +1220,250 @@ static const struct efi_tcg2_protocol efi_tcg2_protocol = {
>   	.get_result_of_set_active_pcr_banks = efi_tcg2_get_result_of_set_active_pcr_banks,
>   };
>
> +/**
> + * parse_event_log_header() -  Parse and verify the event log header fields
> + *
> + * @buffer:			Pointer to the event header
> + * @size:			Size of the eventlog
> + * @pos:			Position in buffer after event log header
> + *
> + * Return:	status code
> + */
> +efi_status_t parse_event_log_header(void *buffer, u32 size, u32 *pos)

This function should be declared in a header or be static.

Should buffer have type struct tcg_pcr_event *?

> +{
> +	struct tcg_pcr_event *event_header = (struct tcg_pcr_event *)buffer;
> +	int i = 0;
> +
> +	if (size < sizeof(*event_header))
> +		return EFI_COMPROMISED_DATA;
> +
> +	if (get_unaligned_le32(&event_header->pcr_index) != 0 ||
> +	    get_unaligned_le32(&event_header->event_type) != EV_NO_ACTION)
> +		return EFI_COMPROMISED_DATA;
> +
> +	for (i = 0; i < sizeof(event_header->digest); i++) {
> +		if (event_header->digest[i] != 0)

if (event_header->digest[i])

> +			return EFI_COMPROMISED_DATA;
> +	}
> +
> +	*pos += sizeof(*event_header);

Do you re

> +
> +	return EFI_SUCCESS;
> +}
> +
> +/**
> + * parse_specid_event() -  Parse and verify the specID Event in the eventlog
> + *
> + * @dev:		udevice
> + * @buffer:		Pointer to the start of the eventlog
> + * @log_size:		Size of the eventlog
> + * @pos:		Offset in the evenlog where specID event starts
> + *
> + * Return:		status code
> + * @pos			Offset in the eventlog where the specID event ends

Duplicate line

> + * @digest_list:	list of digests in the event

Misplaced line

> + */
> +efi_status_t parse_specid_event(struct udevice *dev, void *buffer, u32 log_size,
> +				u32 *pos,
> +				struct tpml_digest_values *digest_list)
> +{

This function should be declared in a header or be static.

> +	struct tcg_efi_spec_id_event *spec_event;
> +	struct tcg_pcr_event *event_header = (struct tcg_pcr_event *)buffer;
> +	size_t spec_event_size;
> +	u32 active = 0, supported = 0, pcr_count = 0, alg_count = 0;
> +	u32 spec_active = 0;
> +	u16 hash_alg, hash_sz;
> +	u8 vendor_sz;
> +	int err, i;
> +
> +	/* Check specID event data */
> +	spec_event = (struct tcg_efi_spec_id_event *)((uintptr_t)buffer + *pos);
> +	/* Check for signature */
> +	if (memcmp(spec_event->signature, TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
> +		   sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03))) {
> +		log_err("specID Event: Signature mismatch\n");
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	if (spec_event->spec_version_minor !=
> +			TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
> +	    spec_event->spec_version_major !=
> +			TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
> +		return EFI_COMPROMISED_DATA;
> +
> +	if (spec_event->number_of_algorithms > MAX_HASH_COUNT ||
> +	    spec_event->number_of_algorithms < 1) {
> +		log_err("specID Event: Number of algorithms incorrect\n");
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	alg_count = spec_event->number_of_algorithms;
> +
> +	err = tpm2_get_pcr_info(dev, &supported, &active, &pcr_count);
> +	if (err)
> +		return EFI_DEVICE_ERROR;
> +
> +	digest_list->count = 0;
> +	/*
> +	 * We may need to worry about the order of algs in this structure as

%s/algs/algorithms/

> +	 * subsequent entries in event should be in same order

"We need to worry" sounds like a FIXME. Is there anything to fix or do
you mean:

We have to take care that the sequence of algorithms that we record in
digest_list matches the sequence in the event log.

> +	 */
> +	for (i = 0; i < alg_count; i++) {
> +		hash_alg =
> +		  get_unaligned_le16(&spec_event->digest_sizes[i].algorithm_id);
> +		hash_sz =
> +		   get_unaligned_le16(&spec_event->digest_sizes[i].digest_size);
> +
> +		if (!(supported & alg_to_mask(hash_alg))) {
> +			log_err("specID Event: Unsupported algorithm\n");
> +			return EFI_COMPROMISED_DATA;
> +		}
> +		digest_list->digests[digest_list->count++].hash_alg = hash_alg;
> +
> +		spec_active |= alg_to_mask(hash_alg);
> +	}
> +
> +	/* TCG spec expects the event log to have hashes for all active PCR's */

The TCG specification

> +	if (spec_active != active) {
> +		/*
> +		 * Previous stage bootloader should know all the active PCR's
> +		 * and use them in the Eventlog.
> +		 */
> +		log_err("specID Event: All active hash alg not present\n");
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	/*
> +	 * the size of the spec event and placement of vendor_info_size
> +	 * depends on supported algoriths
> +	 */
> +	spec_event_size =
> +		offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
> +		alg_count * sizeof(spec_event->digest_sizes[0]);
> +
> +	vendor_sz = *(uint8_t *)((uintptr_t)buffer + *pos + spec_event_size);
> +
> +	spec_event_size += sizeof(vendor_sz) + vendor_sz;
> +	*pos += spec_event_size;
> +
> +	if (get_unaligned_le32(&event_header->event_size) != spec_event_size) {
> +		log_err("specID event: header event size mismatch\n");
> +		/* Right way to handle this can be to call SetActive PCR's */
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +

Missing function description

> +efi_status_t tcg2_parse_event(struct udevice *dev, void *buffer, u32 log_size,
> +			      u32 *offset, struct tpml_digest_values *digest_list,
> +			      u32 *pcr)

Exported functions should be declared in a header.
Or should this function be static?

> +{
> +	struct tcg_pcr_event2 *event = NULL;
> +	u32 event_type, count, size, event_size;
> +	size_t pos;
> +
> +	if (*offset > log_size)
> +		return EFI_COMPROMISED_DATA;
> +
> +	event = (struct tcg_pcr_event2 *)((uintptr_t)buffer + *offset);
> +
> +	*pcr = get_unaligned_le32(&event->pcr_index);
> +
> +	event_size = tcg_event_final_size(digest_list);
> +
> +	if (*offset + event_size > log_size) {
> +		log_err("Event exceeds log size\n");
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	event_type = get_unaligned_le32(&event->event_type);
> +
> +	/* get the count */
> +	count = get_unaligned_le32(&event->digests.count);
> +	if (count != digest_list->count)
> +		return EFI_COMPROMISED_DATA;
> +
> +	pos = offsetof(struct tcg_pcr_event2, digests);
> +	pos += offsetof(struct tpml_digest_values, digests);
> +
> +	for (int i = 0; i < digest_list->count; i++) {
> +		u16 alg;
> +		u16 hash_alg = digest_list->digests[i].hash_alg;
> +		u8 *digest = (u8 *)&digest_list->digests[i].digest;
> +
> +		alg = get_unaligned_le16((void *)((uintptr_t)event + pos));
> +
> +		if (alg != hash_alg)
> +			return EFI_COMPROMISED_DATA;
> +
> +		pos += offsetof(struct tpmt_ha, digest);
> +		memcpy(digest, (void *)((uintptr_t)event + pos), alg_to_len(hash_alg));
> +		pos += alg_to_len(hash_alg);
> +	}
> +
> +	size = get_unaligned_le32((void *)((uintptr_t)event + pos));
> +	event_size += size;
> +	pos += sizeof(u32); /* tcg_pcr_event2 event_size*/
> +	pos += size;
> +
> +	/* make sure the calculated buffer is what we checked against */
> +	if (pos != event_size)
> +		return EFI_COMPROMISED_DATA;
> +
> +	if (pos > log_size)
> +		return EFI_COMPROMISED_DATA;
> +
> +	*offset += pos;
> +
> +	return EFI_SUCCESS;
> +}
> +

Missing function description.

> +efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
> +				  size_t *log_sz)

This function should be declared in a header or should be static.

> +{
> +	struct tpml_digest_values digest_list;
> +	void *buffer;
> +	efi_status_t ret;
> +	u32 pcr, pos;
> +	u64 base;
> +	u32 sz;
> +
> +	ret = platform_get_eventlog(dev, &base, &sz);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	if (sz > TPM2_EVENT_LOG_SIZE)
> +		return EFI_VOLUME_FULL;
> +
> +	buffer = (void *)base;

Superfluous conversion.

> +	pos = 0;
> +	/* Parse the eventlog to check for its validity */
> +	ret = parse_event_log_header(buffer, sz, &pos);
> +	if (ret || pos > sz)

if (ret != EFI_SUCCESS

The check pos > sz should be done in the called function to avoid a
buffer overlow.

> +		return EFI_COMPROMISED_DATA;

Simply return ret.

> +
> +	ret = parse_specid_event(dev, buffer, sz, &pos, &digest_list);
> +	if (ret || pos > sz) {

ditto

> +		log_err("Error parsing SPEC ID Event\n");
> +		return EFI_COMPROMISED_DATA;
> +	}
> +
> +	while (pos < sz) {
> +		ret = tcg2_parse_event(dev, buffer, sz, &pos, &digest_list,
> +				       &pcr);
> +		if (ret) {
> +			log_err("Error parsing event\n");
> +			return ret;
> +		}
> +	}
> +
> +	memcpy(log_buffer, buffer, sz);
> +	*log_sz = sz;
> +
> +	return ret;
> +}
> +
>   /**
>    * create_specid_event() - Create the first event in the eventlog
>    *
> @@ -1312,69 +1595,6 @@ out:
>   	return ret;
>   }
>
> -/**
> - * efi_init_event_log() - initialize an eventlog
> - */
> -static efi_status_t efi_init_event_log(void)
> -{
> -	/*
> -	 * vendor_info_size is currently set to 0, we need to change the length
> -	 * and allocate the flexible array member if this changes
> -	 */
> -	struct tcg_pcr_event *event_header = NULL;
> -	struct udevice *dev;
> -	size_t spec_event_size;
> -	efi_status_t ret;
> -
> -	ret = platform_get_tpm2_device(&dev);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, TPM2_EVENT_LOG_SIZE,
> -				(void **)&event_log.buffer);
> -	if (ret != EFI_SUCCESS)
> -		goto out;
> -
> -	/*
> -	 * initialize log area as 0xff so the OS can easily figure out the
> -	 * last log entry
> -	 */
> -	memset(event_log.buffer, 0xff, TPM2_EVENT_LOG_SIZE);
> -	event_log.pos = 0;
> -	event_log.last_event_size = 0;
> -	event_log.get_event_called = false;
> -	event_log.ebs_called = false;
> -	event_log.truncated = false;
> -
> -	/*
> -	 * The log header is defined to be in SHA1 event log entry format.
> -	 * Setup event header
> -	 */
> -	event_header =  (struct tcg_pcr_event *)event_log.buffer;
> -	put_unaligned_le32(0, &event_header->pcr_index);
> -	put_unaligned_le32(EV_NO_ACTION, &event_header->event_type);
> -	memset(&event_header->digest, 0, sizeof(event_header->digest));
> -	ret = create_specid_event(dev, (void *)((uintptr_t)event_log.buffer + sizeof(*event_header)),
> -				  &spec_event_size);
> -	if (ret != EFI_SUCCESS)
> -		goto free_pool;
> -	put_unaligned_le32(spec_event_size, &event_header->event_size);
> -	event_log.pos = spec_event_size + sizeof(*event_header);
> -	event_log.last_event_size = event_log.pos;
> -
> -	ret = create_final_event();
> -	if (ret != EFI_SUCCESS)
> -		goto free_pool;
> -
> -out:
> -	return ret;
> -
> -free_pool:
> -	efi_free_pool(event_log.buffer);
> -	event_log.buffer = NULL;
> -	return ret;
> -}
> -
>   /**
>    * tcg2_measure_event() - common function to add event log and extend PCR
>    *
> @@ -1427,6 +1647,92 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev)
>   	return ret;
>   }
>
> +/**
> + * efi_init_event_log() - initialize an eventlog

Please, add a line describing the return value.

> + */
> +static efi_status_t efi_init_event_log(void)
> +{
> +	/*
> +	 * vendor_info_size is currently set to 0, we need to change the length
> +	 * and allocate the flexible array member if this changes
> +	 */
> +	struct tcg_pcr_event *event_header = NULL;
> +	struct udevice *dev;
> +	size_t spec_event_size;
> +	efi_status_t ret;
> +
> +	ret = platform_get_tpm2_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		goto out;

return ret;

> +
> +	ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, TPM2_EVENT_LOG_SIZE,
> +				(void **)&event_log.buffer);
> +	if (ret != EFI_SUCCESS)
> +		goto out;

return ret;

> +
> +	/*
> +	 * initialize log area as 0xff so the OS can easily figure out the
> +	 * last log entry

Where is this standardized?

EFI_TCG2_PROTOCOL.GetEventLog() returns EventLogLastEntry to indicate
the last log entry.

> +	 */
> +	memset(event_log.buffer, 0xff, TPM2_EVENT_LOG_SIZE);
> +
> +	/*
> +	 * The log header is defined to be in SHA1 event log entry format.
> +	 * Setup event header
> +	 */
> +	event_header =  (struct tcg_pcr_event *)event_log.buffer;
> +	event_log.pos = 0;
> +	event_log.last_event_size = 0;
> +	event_log.get_event_called = false;
> +	event_log.ebs_called = false;
> +	event_log.truncated = false;
> +
> +	/*
> +	 * Check if earlier firmware have passed any eventlog. Different
> +	 * platforms can use different ways to do so
> +	 */
> +	ret = tcg2_get_fw_eventlog(dev, event_log.buffer, &event_log.pos);
> +	/*
> +	 * If earlier firmware hasn't passed any eventlog, go ahead and
> +	 * create the eventlog header

%s/header/header./

> +	 */
> +	if (ret == EFI_NOT_FOUND) {
> +		put_unaligned_le32(0, &event_header->pcr_index);
> +		put_unaligned_le32(EV_NO_ACTION, &event_header->event_type);
> +		memset(&event_header->digest, 0, sizeof(event_header->digest));
> +		ret = create_specid_event(dev,
> +					  (void *)((uintptr_t)event_log.buffer +
> +						   sizeof(*event_header)),
> +					  &spec_event_size);
> +		if (ret != EFI_SUCCESS)
> +			goto free_pool;
> +		put_unaligned_le32(spec_event_size, &event_header->event_size);
> +		event_log.pos = spec_event_size + sizeof(*event_header);
> +		event_log.last_event_size = event_log.pos;
> +
> +		/*
> +		 * Add SCRTM version to the log if previous firmmware
> +		 * doesn't pass an eventlog
> +		 */
> +		ret = efi_append_scrtm_version(dev);
> +	}
> +
> +	if (ret != EFI_SUCCESS)
> +		goto free_pool;
> +
> +	ret = create_final_event();
> +	if (ret != EFI_SUCCESS)
> +		goto free_pool;
> +
> +out:
> +	return ret;

As you don't do anything here I would prefer removing the label out:

Best regards

Heinrich

> +
> +free_pool:
> +	efi_free_pool(event_log.buffer);
> +	event_log.buffer = NULL;
> +	return ret;
> +}
> +
>   /**
>    * tcg2_measure_variable() - add variable event log and extend PCR
>    *
> @@ -1963,12 +2269,6 @@ efi_status_t efi_tcg2_register(void)
>   	if (ret != EFI_SUCCESS)
>   		goto fail;
>
> -	ret = efi_append_scrtm_version(dev);
> -	if (ret != EFI_SUCCESS) {
> -		tcg2_uninit();
> -		goto fail;
> -	}
> -
>   	ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol,
>   			       (void *)&efi_tcg2_protocol);
>   	if (ret != EFI_SUCCESS) {
>

Re: [PATCH v6 3/3] efi_loader: Extend PCR's for firmware measurements

[Heinrich Schuchardt]

On 11/26/21 06:00, Ruchika Gupta wrote:
> Firmwares before U-Boot may be capable of doing tpm measurements
> and passing them to U-Boot in the form of eventlog. However there
> may be scenarios where the firmwares don't have TPM driver and
> are not capable of extending the measurements in the PCRs.
> Based on TCG spec, if previous firnware has extended PCR's, PCR0
> would not be 0. So, read the PCR0 to determine if the PCR's need
> to be extended as eventlog is parsed or not.
>
> Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> v6: Changed TPM2_DIGEST_LEN to TPM2_SHA512_DIGEST_SIZE
>
> v5 : No change
>
> v4 : No change
>
> v3 :
> Rebase changes on top of changes made in first patch series
>
> v2 :
> Removed check for PCR0 in eventlog
>
>   lib/efi_loader/efi_tcg2.c | 75 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index a789c44660..295070f3d8 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -199,6 +199,43 @@ static efi_status_t tcg2_pcr_extend(struct udevice *dev, u32 pcr_index,
>   	return EFI_SUCCESS;
>   }
>
> +/* tcg2_pcr_read - Read PCRs for a TPM2 device for a given tpml_digest_values
> + *
> + * @dev:		device

pcr_index is missing

Best regards

Heinrich

> + * @digest_list:	list of digest algorithms to extend
> + *
> + * @Return: status code
> + */
> +static efi_status_t tcg2_pcr_read(struct udevice *dev, u32 pcr_index,
> +				  struct tpml_digest_values *digest_list)
> +{
> +	struct tpm_chip_priv *priv;
> +	unsigned int updates, pcr_select_min;
> +	u32 rc;
> +	size_t i;
> +
> +	priv = dev_get_uclass_priv(dev);
> +	if (!priv)
> +		return EFI_DEVICE_ERROR;
> +
> +	pcr_select_min = priv->pcr_select_min;
> +
> +	for (i = 0; i < digest_list->count; i++) {
> +		u16 hash_alg = digest_list->digests[i].hash_alg;
> +		u8 *digest = (u8 *)&digest_list->digests[i].digest;
> +
> +		rc = tpm2_pcr_read(dev, pcr_index, pcr_select_min,
> +				   hash_alg, digest, alg_to_len(hash_alg),
> +				   &updates);
> +		if (rc) {
> +			EFI_PRINT("Failed to read PCR\n");
> +			return EFI_DEVICE_ERROR;
> +		}
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +
>   /* put_event - Append an agile event to an eventlog
>    *
>    * @pcr_index:		PCR index
> @@ -1428,6 +1465,8 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>   	u32 pcr, pos;
>   	u64 base;
>   	u32 sz;
> +	bool extend_pcr = false;
> +	int i;
>
>   	ret = platform_get_eventlog(dev, &base, &sz);
>   	if (ret != EFI_SUCCESS)
> @@ -1449,6 +1488,26 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>   		return EFI_COMPROMISED_DATA;
>   	}
>
> +	ret = tcg2_pcr_read(dev, 0, &digest_list);
> +	if (ret) {
> +		log_err("Error reading PCR 0\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * If PCR0 is 0, previous firmware didn't have the capability
> +	 * to extend the PCR. In this scenario, extend the PCR as
> +	 * the eventlog is parsed.
> +	 */
> +	for (i = 0; i < digest_list.count; i++) {
> +		u8 buffer[TPM2_SHA512_DIGEST_SIZE] =  { 0 };
> +		u16 hash_alg = digest_list.digests[i].hash_alg;
> +
> +		if (!memcmp((u8 *)&digest_list.digests[i].digest, buffer,
> +			    alg_to_len(hash_alg)))
> +			extend_pcr = true;
> +	}
> +
>   	while (pos < sz) {
>   		ret = tcg2_parse_event(dev, buffer, sz, &pos, &digest_list,
>   				       &pcr);
> @@ -1456,6 +1515,22 @@ efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>   			log_err("Error parsing event\n");
>   			return ret;
>   		}
> +		if (extend_pcr) {
> +			ret = tcg2_pcr_extend(dev, pcr, &digest_list);
> +			if (ret != EFI_SUCCESS) {
> +				log_err("Error in extending PCR\n");
> +				return ret;
> +			}
> +
> +			/* Clear the digest for next event */
> +			for (i = 0; i < digest_list.count; i++) {
> +				u16 hash_alg = digest_list.digests[i].hash_alg;
> +				u8 *digest =
> +				   (u8 *)&digest_list.digests[i].digest;
> +
> +				memset(digest, 0, alg_to_len(hash_alg));
> +			}
> +		}
>   	}
>
>   	memcpy(log_buffer, buffer, sz);
>

Re: [PATCH v6 1/3] efi_loader: Add check for event log passed from firmware

[Ilias Apalodimas]

Hi Heinrich,

> 
> > +
> > +	/*
> > +	 * initialize log area as 0xff so the OS can easily figure out the
> > +	 * last log entry
> 
> Where is this standardized?
> 
> EFI_TCG2_PROTOCOL.GetEventLog() returns EventLogLastEntry to indicate
> the last log entry.
> 

Indeed.  However while I was looking into EDK2 code I found a similar
comment [1].  Since EDK2 is used to boot windows as well I assumed that was
a good policy to keep.  


> > +	 */
> > +	memset(event_log.buffer, 0xff, TPM2_EVENT_LOG_SIZE);
> > +
> > +	/*
> > +	 * The log header is defined to be in SHA1 event log entry format.
> > +	 * Setup event header
> > +	 */

[...]

[1] https://github.com/tianocore/edk2/blob/master/SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c#L1578

Cheers
/Ilias

Re: [PATCH v6 1/3] efi_loader: Add check for event log passed from firmware

[Ruchika Gupta]

Hi Heinrich,

On Fri, 26 Nov 2021 at 13:01, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 11/26/21 06:00, Ruchika Gupta wrote:
> > Platforms may have support to measure their initial firmware components
> > and pass the event log to u-boot. The event log address can be passed
> > in property tpm_event_log_addr and tpm_event_log_size of the tpm node.
> > Platforms may choose their own specific mechanism to do so. A weak
> > function is added to check if even log has been passed to u-boot
> > from earlier firmware components. If available, the eventlog is parsed
> > to check for its correctness and further event logs are appended to the
> > passed log.
> >
> > Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
> > Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > v6: No change
> >
> > v5:
> > Shift the efi_init_event_log() to a different location in the file.
> > This help fixes compilation issue introduced by calling
> efi_append_scrtm_version()
> > from it.
> >
> > v4:
> > Add SCRTM version to log only if previous firmware doesn't pass the
> eventlog
> >
> > v3:
> > Return as soon as you detect error
> >
> > v2:
> > Moved firmware eventlog code parsing to tcg2_get_fw_eventlog()
> >
> >   lib/efi_loader/efi_tcg2.c | 438 ++++++++++++++++++++++++++++++++------
> >   1 file changed, 369 insertions(+), 69 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 8c1f22e337..a789c44660 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -324,6 +324,45 @@ __weak efi_status_t platform_get_tpm2_device(struct
> udevice **dev)
> >       return EFI_NOT_FOUND;
> >   }
> >
> > +/**
> > + * platform_get_eventlog() - retrieve the eventlog address and size
> > + *
> > + * This function retrieves the eventlog address and size if the
> underlying
> > + * firmware has done some measurements and passed them.
> > + *
> > + * This function may be overridden based on platform specific method of
> > + * passing the eventlog address and size.
> > + *
> > + * @dev:     udevice
> > + * @addr:    eventlog address
> > + * @sz:              eventlog size
> > + * Return:   status code
> > + */
> > +__weak efi_status_t platform_get_eventlog(struct udevice *dev, u64
> *addr,
> > +                                       u32 *sz)
>
> This function must be declared in a header to be overridden.
>
> > +{
> > +     const u64 *basep;
> > +     const u32 *sizep;
> > +
> > +     basep = dev_read_prop(dev, "tpm_event_log_addr", NULL);
> > +     if (!basep)
> > +             return EFI_NOT_FOUND;
> > +
> > +     *addr = be64_to_cpup((__force __be64 *)basep);
> > +
> > +     sizep = dev_read_prop(dev, "tpm_event_log_size", NULL);
> > +     if (!sizep)
> > +             return EFI_NOT_FOUND;
> > +
> > +     *sz = be32_to_cpup((__force __be32 *)sizep);
> > +     if (*sz == 0) {
> > +             log_debug("event log empty\n");
> > +             return EFI_NOT_FOUND;
> > +     }
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> >   /**
> >    * tpm2_get_max_command_size() - get the supported max command size
> >    *
> > @@ -1181,6 +1220,250 @@ static const struct efi_tcg2_protocol
> efi_tcg2_protocol = {
> >       .get_result_of_set_active_pcr_banks =
> efi_tcg2_get_result_of_set_active_pcr_banks,
> >   };
> >
> > +/**
> > + * parse_event_log_header() -  Parse and verify the event log header
> fields
> > + *
> > + * @buffer:                  Pointer to the event header
> > + * @size:                    Size of the eventlog
> > + * @pos:                     Position in buffer after event log header
> > + *
> > + * Return:   status code
> > + */
> > +efi_status_t parse_event_log_header(void *buffer, u32 size, u32 *pos)
>
> This function should be declared in a header or be static.
>
> Should buffer have type struct tcg_pcr_event *?
>

Since buffer points to the complete eventlog, it would be probably better
to keep it as it is i.e void *.
I will correct the description of this parameter in the function
description to avoid confusion.


> > +{
> > +     struct tcg_pcr_event *event_header = (struct tcg_pcr_event
> *)buffer;
> > +     int i = 0;
> > +
> > +     if (size < sizeof(*event_header))
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     if (get_unaligned_le32(&event_header->pcr_index) != 0 ||
> > +         get_unaligned_le32(&event_header->event_type) != EV_NO_ACTION)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     for (i = 0; i < sizeof(event_header->digest); i++) {
> > +             if (event_header->digest[i] != 0)
>
> if (event_header->digest[i])
>
> > +                     return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     *pos += sizeof(*event_header);
>
> Do you re
>

Probably you are asking about parameter pos here. pos points to the offset
in the buffer where the next event starts (after the event header)


>
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +/**
> > + * parse_specid_event() -  Parse and verify the specID Event in the
> eventlog
> > + *
> > + * @dev:             udevice
> > + * @buffer:          Pointer to the start of the eventlog
> > + * @log_size:                Size of the eventlog
> > + * @pos:             Offset in the evenlog where specID event starts
> > + *
> > + * Return:           status code
> > + * @pos                      Offset in the eventlog where the specID
> event ends
>
> Duplicate line
>
> > + * @digest_list:     list of digests in the event
>
> Misplaced line
>
> > + */
> > +efi_status_t parse_specid_event(struct udevice *dev, void *buffer, u32
> log_size,
> > +                             u32 *pos,
> > +                             struct tpml_digest_values *digest_list)
> > +{
>
> This function should be declared in a header or be static.
>
> > +     struct tcg_efi_spec_id_event *spec_event;
> > +     struct tcg_pcr_event *event_header = (struct tcg_pcr_event
> *)buffer;
> > +     size_t spec_event_size;
> > +     u32 active = 0, supported = 0, pcr_count = 0, alg_count = 0;
> > +     u32 spec_active = 0;
> > +     u16 hash_alg, hash_sz;
> > +     u8 vendor_sz;
> > +     int err, i;
> > +
> > +     /* Check specID event data */
> > +     spec_event = (struct tcg_efi_spec_id_event *)((uintptr_t)buffer +
> *pos);
> > +     /* Check for signature */
> > +     if (memcmp(spec_event->signature,
> TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03,
> > +                sizeof(TCG_EFI_SPEC_ID_EVENT_SIGNATURE_03))) {
> > +             log_err("specID Event: Signature mismatch\n");
> > +             return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     if (spec_event->spec_version_minor !=
> > +                     TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MINOR_TPM2 ||
> > +         spec_event->spec_version_major !=
> > +                     TCG_EFI_SPEC_ID_EVENT_SPEC_VERSION_MAJOR_TPM2)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     if (spec_event->number_of_algorithms > MAX_HASH_COUNT ||
> > +         spec_event->number_of_algorithms < 1) {
> > +             log_err("specID Event: Number of algorithms incorrect\n");
> > +             return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     alg_count = spec_event->number_of_algorithms;
> > +
> > +     err = tpm2_get_pcr_info(dev, &supported, &active, &pcr_count);
> > +     if (err)
> > +             return EFI_DEVICE_ERROR;
> > +
> > +     digest_list->count = 0;
> > +     /*
> > +      * We may need to worry about the order of algs in this structure
> as
>
> %s/algs/algorithms/
>
> > +      * subsequent entries in event should be in same order
>
> "We need to worry" sounds like a FIXME. Is there anything to fix or do
> you mean:
>
> We have to take care that the sequence of algorithms that we record in
> digest_list matches the sequence in the event log.
>
> > +      */
> > +     for (i = 0; i < alg_count; i++) {
> > +             hash_alg =
> > +
>  get_unaligned_le16(&spec_event->digest_sizes[i].algorithm_id);
> > +             hash_sz =
> > +
> get_unaligned_le16(&spec_event->digest_sizes[i].digest_size);
> > +
> > +             if (!(supported & alg_to_mask(hash_alg))) {
> > +                     log_err("specID Event: Unsupported algorithm\n");
> > +                     return EFI_COMPROMISED_DATA;
> > +             }
> > +             digest_list->digests[digest_list->count++].hash_alg =
> hash_alg;
> > +
> > +             spec_active |= alg_to_mask(hash_alg);
> > +     }
> > +
> > +     /* TCG spec expects the event log to have hashes for all active
> PCR's */
>
> The TCG specification
>
> > +     if (spec_active != active) {
> > +             /*
> > +              * Previous stage bootloader should know all the active
> PCR's
> > +              * and use them in the Eventlog.
> > +              */
> > +             log_err("specID Event: All active hash alg not present\n");
> > +             return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     /*
> > +      * the size of the spec event and placement of vendor_info_size
> > +      * depends on supported algoriths
> > +      */
> > +     spec_event_size =
> > +             offsetof(struct tcg_efi_spec_id_event, digest_sizes) +
> > +             alg_count * sizeof(spec_event->digest_sizes[0]);
> > +
> > +     vendor_sz = *(uint8_t *)((uintptr_t)buffer + *pos +
> spec_event_size);
> > +
> > +     spec_event_size += sizeof(vendor_sz) + vendor_sz;
> > +     *pos += spec_event_size;
> > +
> > +     if (get_unaligned_le32(&event_header->event_size) !=
> spec_event_size) {
> > +             log_err("specID event: header event size mismatch\n");
> > +             /* Right way to handle this can be to call SetActive PCR's
> */
> > +             return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
>
> Missing function description
>
> > +efi_status_t tcg2_parse_event(struct udevice *dev, void *buffer, u32
> log_size,
> > +                           u32 *offset, struct tpml_digest_values
> *digest_list,
> > +                           u32 *pcr)
>
> Exported functions should be declared in a header.
> Or should this function be static?
>
> > +{
> > +     struct tcg_pcr_event2 *event = NULL;
> > +     u32 event_type, count, size, event_size;
> > +     size_t pos;
> > +
> > +     if (*offset > log_size)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     event = (struct tcg_pcr_event2 *)((uintptr_t)buffer + *offset);
> > +
> > +     *pcr = get_unaligned_le32(&event->pcr_index);
> > +
> > +     event_size = tcg_event_final_size(digest_list);
> > +
> > +     if (*offset + event_size > log_size) {
> > +             log_err("Event exceeds log size\n");
> > +             return EFI_COMPROMISED_DATA;
> > +     }
> > +
> > +     event_type = get_unaligned_le32(&event->event_type);
> > +
> > +     /* get the count */
> > +     count = get_unaligned_le32(&event->digests.count);
> > +     if (count != digest_list->count)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     pos = offsetof(struct tcg_pcr_event2, digests);
> > +     pos += offsetof(struct tpml_digest_values, digests);
> > +
> > +     for (int i = 0; i < digest_list->count; i++) {
> > +             u16 alg;
> > +             u16 hash_alg = digest_list->digests[i].hash_alg;
> > +             u8 *digest = (u8 *)&digest_list->digests[i].digest;
> > +
> > +             alg = get_unaligned_le16((void *)((uintptr_t)event + pos));
> > +
> > +             if (alg != hash_alg)
> > +                     return EFI_COMPROMISED_DATA;
> > +
> > +             pos += offsetof(struct tpmt_ha, digest);
> > +             memcpy(digest, (void *)((uintptr_t)event + pos),
> alg_to_len(hash_alg));
> > +             pos += alg_to_len(hash_alg);
> > +     }
> > +
> > +     size = get_unaligned_le32((void *)((uintptr_t)event + pos));
> > +     event_size += size;
> > +     pos += sizeof(u32); /* tcg_pcr_event2 event_size*/
> > +     pos += size;
> > +
> > +     /* make sure the calculated buffer is what we checked against */
> > +     if (pos != event_size)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     if (pos > log_size)
> > +             return EFI_COMPROMISED_DATA;
> > +
> > +     *offset += pos;
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
>
> Missing function description.
>
> > +efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
> > +                               size_t *log_sz)
>
> This function should be declared in a header or should be static.
>
> > +{
> > +     struct tpml_digest_values digest_list;
> > +     void *buffer;
> > +     efi_status_t ret;
> > +     u32 pcr, pos;
> > +     u64 base;
> > +     u32 sz;
> > +
> > +     ret = platform_get_eventlog(dev, &base, &sz);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     if (sz > TPM2_EVENT_LOG_SIZE)
> > +             return EFI_VOLUME_FULL;
> > +
> > +     buffer = (void *)base;
>
> Superfluous conversion.
>
Sorry, I didn't get this comment.
If you meant removing the typecast, base is u64 so when assigning it to a
void pointer, cast would be needed.

On removing the case, I get this warning
buffer = base;
lib/efi_loader/efi_tcg2.c:1518:9: warning: assignment to ‘void *’ from
‘u64’ {aka ‘long long unsigned int’} makes pointer from integer without a
cast [-Wint-conversion]
 1518 |  buffer = base;

[...]

Regards,
Ruchika