[PATCH v2 0/3] Enhance Measured Boot

[PATCH v2 0/3] Enhance Measured Boot

[Masahisa Kojima]

This patch series add the following measurement mandated in the
TCG PC Client PFP Specification.
 - SMBIOS tables
 - GPT disk partition topology
 - AuditMode and Deployed mode

Masahisa Kojima (3):
  efi_loader: add SMBIOS table measurement
  efi_loader: add UEFI GPT measurement
  efi_loader: add DeployedMode and AuditMode variable measurement

 include/blk.h                 |   3 +
 include/efi_loader.h          |   4 +-
 include/efi_tcg2.h            |  27 +++
 include/smbios.h              |  17 +-
 lib/efi_loader/Kconfig        |   1 +
 lib/efi_loader/efi_boottime.c |   4 +-
 lib/efi_loader/efi_smbios.c   |   2 -
 lib/efi_loader/efi_tcg2.c     | 306 +++++++++++++++++++++++++++++++++-
 lib/smbios-parser.c           | 152 +++++++++++++++--
 9 files changed, 499 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Masahisa Kojima]

TCG PC Client spec requires to measure the SMBIOS
table that contain static configuration information
(e.g. Platform Manufacturer Enterprise Number assigned by IANA,
platform model number, Vendor and Device IDs for each SMBIOS table).

The device and environment dependent information such as
serial number is cleared to zero or space character for
the measurement.

Existing smbios_string() function returns pointer to the string
with const qualifier, but exisintg use case is updating version
string and const qualifier must be removed.
This commit removes const qualifier from smbios_string()
return value and reuses to clear the strings for the measurement.

This commit also fixes the following compiler warning:

lib/smbios-parser.c:59:39: warning: cast to pointer from integer of
different size [-Wint-to-pointer-cast]
  const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---

Changes in v2:
- use flexible array for table_entry field
- modify funtion name to find_smbios_table()
- remove unnecessary const qualifier from smbios_string()
- create non-const version of next_header()

 include/efi_loader.h          |   2 +
 include/efi_tcg2.h            |  15 ++++
 include/smbios.h              |  17 +++-
 lib/efi_loader/Kconfig        |   1 +
 lib/efi_loader/efi_boottime.c |   2 +
 lib/efi_loader/efi_smbios.c   |   2 -
 lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
 lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
 8 files changed, 261 insertions(+), 14 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c440962fe5..13f0c24058 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -308,6 +308,8 @@ extern const efi_guid_t efi_guid_capsule_report;
 extern const efi_guid_t efi_guid_firmware_management_protocol;
 /* GUID for the ESRT */
 extern const efi_guid_t efi_esrt_guid;
+/* GUID of the SMBIOS table */
+extern const efi_guid_t smbios_guid;
 
 extern char __efi_runtime_start[], __efi_runtime_stop[];
 extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[];
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index 5a1a36212e..85a032dbbd 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -215,6 +215,21 @@ struct efi_tcg2_uefi_variable_data {
 	u8 variable_data[1];
 };
 
+/**
+ * struct tdUEFI_HANDOFF_TABLE_POINTERS2 - event log structure of SMBOIS tables
+ * @table_description_size:	size of table description
+ * @table_description:		table description
+ * @number_of_tables:		number of uefi configuration table
+ * @table_entry:		uefi configuration table entry
+ */
+#define SMBIOS_HANDOFF_TABLE_DESC  "SmbiosTable"
+struct smbios_handoff_table_pointers2 {
+	u8 table_description_size;
+	u8 table_description[sizeof(SMBIOS_HANDOFF_TABLE_DESC)];
+	u64 number_of_tables;
+	struct efi_configuration_table table_entry[];
+} __packed;
+
 struct efi_tcg2_protocol {
 	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
 					       struct efi_tcg2_boot_service_capability *capability);
diff --git a/include/smbios.h b/include/smbios.h
index aa6b6f3849..acfcbfe2ca 100644
--- a/include/smbios.h
+++ b/include/smbios.h
@@ -260,9 +260,9 @@ const struct smbios_header *smbios_header(const struct smbios_entry *entry, int
  *
  * @header:    pointer to struct smbios_header
  * @index:     string index
- * @return:    NULL or a valid const char pointer
+ * @return:    NULL or a valid char pointer
  */
-const char *smbios_string(const struct smbios_header *header, int index);
+char *smbios_string(const struct smbios_header *header, int index);
 
 /**
  * smbios_update_version() - Update the version string
@@ -292,4 +292,17 @@ int smbios_update_version(const char *version);
  */
 int smbios_update_version_full(void *smbios_tab, const char *version);
 
+/**
+ * smbios_prepare_measurement() - Update smbios table for the measurement
+ *
+ * TCG specification requires to measure static configuration information.
+ * This function clear the device dependent parameters such as
+ * serial number for the measurement.
+ *
+ * @entry: pointer to a struct smbios_entry
+ * @header: pointer to a struct smbios_header
+ */
+void smbios_prepare_measurement(const struct smbios_entry *entry,
+				struct smbios_header *header);
+
 #endif /* _SMBIOS_H_ */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index f4e9129a39..da68a219a3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -328,6 +328,7 @@ config EFI_TCG2_PROTOCOL
 	select SHA384
 	select SHA512
 	select HASH
+	select SMBIOS_PARSER
 	help
 	  Provide a EFI_TCG2_PROTOCOL implementation using the TPM hardware
 	  of the platform.
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index f0283b539e..701e2212c8 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -86,6 +86,8 @@ const efi_guid_t efi_guid_event_group_reset_system =
 /* GUIDs of the Load File and Load File2 protocols */
 const efi_guid_t efi_guid_load_file_protocol = EFI_LOAD_FILE_PROTOCOL_GUID;
 const efi_guid_t efi_guid_load_file2_protocol = EFI_LOAD_FILE2_PROTOCOL_GUID;
+/* GUID of the SMBIOS table */
+const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
 
 static efi_status_t EFIAPI efi_disconnect_controller(
 					efi_handle_t controller_handle,
diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c
index 2eb4cb1c1a..fc0b23397c 100644
--- a/lib/efi_loader/efi_smbios.c
+++ b/lib/efi_loader/efi_smbios.c
@@ -13,8 +13,6 @@
 #include <mapmem.h>
 #include <smbios.h>
 
-static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID;
-
 /*
  * Install the SMBIOS table as a configuration table.
  *
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index cb48919223..4f68f6dfd5 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -14,6 +14,7 @@
 #include <efi_tcg2.h>
 #include <log.h>
 #include <malloc.h>
+#include <smbios.h>
 #include <version.h>
 #include <tpm-v2.h>
 #include <u-boot/hash-checksum.h>
@@ -1449,6 +1450,81 @@ error:
 	return ret;
 }
 
+/**
+ * tcg2_measure_smbios() - measure smbios table
+ *
+ * @dev:	TPM device
+ * @entry:	pointer to the smbios_entry structure
+ *
+ * Return:	status code
+ */
+static efi_status_t
+tcg2_measure_smbios(struct udevice *dev,
+		    const struct smbios_entry *entry)
+{
+	efi_status_t ret;
+	struct smbios_header *smbios_copy;
+	struct smbios_handoff_table_pointers2 *event = NULL;
+	u32 event_size;
+
+	/*
+	 * TCG PC Client PFP Spec says
+	 * "SMBIOS structures that contain static configuration information
+	 * (e.g. Platform Manufacturer Enterprise Number assigned by IANA,
+	 * platform model number, Vendor and Device IDs for each SMBIOS table)
+	 * that is relevant to the security of the platform MUST be measured".
+	 * Device dependent parameters such as serial number are cleared to
+	 * zero or spaces for the measurement.
+	 */
+	event_size = sizeof(struct smbios_handoff_table_pointers2) +
+		     FIELD_SIZEOF(struct efi_configuration_table, guid) +
+		     entry->struct_table_length;
+	event = calloc(1, event_size);
+	if (!event) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	event->table_description_size = sizeof(SMBIOS_HANDOFF_TABLE_DESC);
+	memcpy(event->table_description, SMBIOS_HANDOFF_TABLE_DESC,
+	       sizeof(SMBIOS_HANDOFF_TABLE_DESC));
+	put_unaligned_le64(1, &event->number_of_tables);
+	guidcpy(&event->table_entry[0].guid, &smbios_guid);
+	smbios_copy = (struct smbios_header *)((uintptr_t)&event->table_entry[0].table);
+	memcpy(&event->table_entry[0].table,
+	       (void *)((uintptr_t)entry->struct_table_address),
+	       entry->struct_table_length);
+
+	smbios_prepare_measurement(entry, smbios_copy);
+
+	ret = tcg2_measure_event(dev, 1, EV_EFI_HANDOFF_TABLES2, event_size,
+				 (u8 *)event);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+out:
+	free(event);
+
+	return ret;
+}
+
+/**
+ * find_smbios_table() - find smbios table
+ *
+ * Return:	pointer to the smbios table
+ */
+static void *find_smbios_table(void)
+{
+	u32 i;
+
+	for (i = 0; i < systab.nr_tables; i++) {
+		if (!guidcmp(&smbios_guid, &systab.tables[i].guid))
+			return systab.tables[i].table;
+	}
+
+	return NULL;
+}
+
 /**
  * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
  *
@@ -1460,6 +1536,7 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
 	u32 pcr_index;
 	struct udevice *dev;
 	u32 event = 0;
+	struct smbios_entry *entry;
 
 	if (tcg2_efi_app_invoked)
 		return EFI_SUCCESS;
@@ -1485,6 +1562,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
 			goto out;
 	}
 
+	entry = (struct smbios_entry *)find_smbios_table();
+	if (entry) {
+		ret = tcg2_measure_smbios(dev, entry);
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
+
 	tcg2_efi_app_invoked = true;
 out:
 	return ret;
diff --git a/lib/smbios-parser.c b/lib/smbios-parser.c
index 34203f952c..596a967302 100644
--- a/lib/smbios-parser.c
+++ b/lib/smbios-parser.c
@@ -39,10 +39,8 @@ const struct smbios_entry *smbios_entry(u64 address, u32 size)
 	return entry;
 }
 
-static const struct smbios_header *next_header(const struct smbios_header *curr)
+static u8 *find_next_header(u8 *pos)
 {
-	u8 *pos = ((u8 *)curr) + curr->length;
-
 	/* search for _double_ NULL bytes */
 	while (!((*pos == 0) && (*(pos + 1) == 0)))
 		pos++;
@@ -50,13 +48,27 @@ static const struct smbios_header *next_header(const struct smbios_header *curr)
 	/* step behind the double NULL bytes */
 	pos += 2;
 
-	return (struct smbios_header *)pos;
+	return pos;
+}
+
+static struct smbios_header *get_next_header(struct smbios_header *curr)
+{
+	u8 *pos = ((u8 *)curr) + curr->length;
+
+	return (struct smbios_header *)find_next_header(pos);
+}
+
+static const struct smbios_header *next_header(const struct smbios_header *curr)
+{
+	u8 *pos = ((u8 *)curr) + curr->length;
+
+	return (struct smbios_header *)find_next_header(pos);
 }
 
 const struct smbios_header *smbios_header(const struct smbios_entry *entry, int type)
 {
 	const unsigned int num_header = entry->struct_count;
-	const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;
+	const struct smbios_header *header = (struct smbios_header *)((uintptr_t)entry->struct_table_address);
 
 	for (unsigned int i = 0; i < num_header; i++) {
 		if (header->type == type)
@@ -68,8 +80,8 @@ const struct smbios_header *smbios_header(const struct smbios_entry *entry, int
 	return NULL;
 }
 
-static const char *string_from_smbios_table(const struct smbios_header *header,
-                                           int idx)
+static char *string_from_smbios_table(const struct smbios_header *header,
+				      int idx)
 {
 	unsigned int i = 1;
 	u8 *pos;
@@ -86,10 +98,10 @@ static const char *string_from_smbios_table(const struct smbios_header *header,
 		pos++;
 	}
 
-	return (const char *)pos;
+	return (char *)pos;
 }
 
-const char *smbios_string(const struct smbios_header *header, int index)
+char *smbios_string(const struct smbios_header *header, int index)
 {
 	if (!header)
 		return NULL;
@@ -109,7 +121,7 @@ int smbios_update_version_full(void *smbios_tab, const char *version)
 	if (!hdr)
 		return log_msg_ret("tab", -ENOENT);
 	bios = (struct smbios_type0 *)hdr;
-	ptr = (char *)smbios_string(hdr, bios->bios_ver);
+	ptr = smbios_string(hdr, bios->bios_ver);
 	if (!ptr)
 		return log_msg_ret("str", -ENOMEDIUM);
 
@@ -132,3 +144,123 @@ int smbios_update_version_full(void *smbios_tab, const char *version)
 
 	return 0;
 }
+
+struct smbios_filter_param {
+	u32 offset;
+	u32 size;
+	bool is_string;
+};
+
+struct smbios_filter_table {
+	int type;
+	struct smbios_filter_param *params;
+	u32 count;
+};
+
+struct smbios_filter_param smbios_type1_filter_params[] = {
+	{offsetof(struct smbios_type1, serial_number),
+	 FIELD_SIZEOF(struct smbios_type1, serial_number), true},
+	{offsetof(struct smbios_type1, uuid),
+	 FIELD_SIZEOF(struct smbios_type1, uuid), false},
+	{offsetof(struct smbios_type1, wakeup_type),
+	 FIELD_SIZEOF(struct smbios_type1, wakeup_type), false},
+};
+
+struct smbios_filter_param smbios_type2_filter_params[] = {
+	{offsetof(struct smbios_type2, serial_number),
+	 FIELD_SIZEOF(struct smbios_type2, serial_number), true},
+	{offsetof(struct smbios_type2, chassis_location),
+	 FIELD_SIZEOF(struct smbios_type2, chassis_location), false},
+};
+
+struct smbios_filter_param smbios_type3_filter_params[] = {
+	{offsetof(struct smbios_type3, serial_number),
+	 FIELD_SIZEOF(struct smbios_type3, serial_number), true},
+	{offsetof(struct smbios_type3, asset_tag_number),
+	 FIELD_SIZEOF(struct smbios_type3, asset_tag_number), true},
+};
+
+struct smbios_filter_param smbios_type4_filter_params[] = {
+	{offsetof(struct smbios_type4, serial_number),
+	 FIELD_SIZEOF(struct smbios_type4, serial_number), true},
+	{offsetof(struct smbios_type4, asset_tag),
+	 FIELD_SIZEOF(struct smbios_type4, asset_tag), true},
+	{offsetof(struct smbios_type4, part_number),
+	 FIELD_SIZEOF(struct smbios_type4, part_number), true},
+	{offsetof(struct smbios_type4, core_count),
+	 FIELD_SIZEOF(struct smbios_type4, core_count), false},
+	{offsetof(struct smbios_type4, core_enabled),
+	 FIELD_SIZEOF(struct smbios_type4, core_enabled), false},
+	{offsetof(struct smbios_type4, thread_count),
+	 FIELD_SIZEOF(struct smbios_type4, thread_count), false},
+	{offsetof(struct smbios_type4, core_count2),
+	 FIELD_SIZEOF(struct smbios_type4, core_count2), false},
+	{offsetof(struct smbios_type4, core_enabled2),
+	 FIELD_SIZEOF(struct smbios_type4, core_enabled2), false},
+	{offsetof(struct smbios_type4, thread_count),
+	 FIELD_SIZEOF(struct smbios_type4, thread_count2), false},
+	{offsetof(struct smbios_type4, voltage),
+	 FIELD_SIZEOF(struct smbios_type4, voltage), false},
+};
+
+struct smbios_filter_table smbios_filter_tables[] = {
+	{SMBIOS_SYSTEM_INFORMATION, smbios_type1_filter_params,
+	 ARRAY_SIZE(smbios_type1_filter_params)},
+	{SMBIOS_BOARD_INFORMATION, smbios_type2_filter_params,
+	 ARRAY_SIZE(smbios_type2_filter_params)},
+	{SMBIOS_SYSTEM_ENCLOSURE, smbios_type3_filter_params,
+	 ARRAY_SIZE(smbios_type3_filter_params)},
+	{SMBIOS_PROCESSOR_INFORMATION, smbios_type4_filter_params,
+	 ARRAY_SIZE(smbios_type4_filter_params)},
+};
+
+static void clear_smbios_table(struct smbios_header *header,
+			       struct smbios_filter_param *filter,
+			       u32 count)
+{
+	u32 i;
+	char *str;
+	u8 string_id;
+
+	for (i = 0; i < count; i++) {
+		if (filter[i].is_string) {
+			string_id = *((u8 *)header + filter[i].offset);
+			if (string_id == 0) /* string is empty */
+				continue;
+
+			str = smbios_string(header, string_id);
+			if (!str)
+				continue;
+
+			/* string is cleared to space, keep '\0' terminator */
+			memset(str, ' ', strlen(str));
+
+		} else {
+			memset((void *)((u8 *)header + filter[i].offset),
+			       0, filter[i].size);
+		}
+	}
+}
+
+void smbios_prepare_measurement(const struct smbios_entry *entry,
+				struct smbios_header *smbios_copy)
+{
+	u32 i, j;
+	struct smbios_header *header;
+
+	for (i = 0; i < ARRAY_SIZE(smbios_filter_tables); i++) {
+		header = smbios_copy;
+		for (j = 0; j < entry->struct_count; j++) {
+			if (header->type == smbios_filter_tables[i].type)
+				break;
+
+			header = get_next_header(header);
+		}
+		if (j >= entry->struct_count)
+			continue;
+
+		clear_smbios_table(header,
+				   smbios_filter_tables[i].params,
+				   smbios_filter_tables[i].count);
+	}
+}
-- 
2.17.1

[PATCH v2 2/3] efi_loader: add UEFI GPT measurement

[Masahisa Kojima]

This commit adds the UEFI GPT disk partition topology
measurement required in TCG PC Client PFP Spec.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---

(no changes since v1)

 include/blk.h                 |   3 +
 include/efi_loader.h          |   2 +-
 include/efi_tcg2.h            |  12 +++
 lib/efi_loader/efi_boottime.c |   2 +-
 lib/efi_loader/efi_tcg2.c     | 175 +++++++++++++++++++++++++++++++++-
 5 files changed, 191 insertions(+), 3 deletions(-)

diff --git a/include/blk.h b/include/blk.h
index 19bab081c2..f0cc7ca1a2 100644
--- a/include/blk.h
+++ b/include/blk.h
@@ -45,6 +45,9 @@ enum if_type {
 #define BLK_PRD_SIZE		20
 #define BLK_REV_SIZE		8
 
+#define PART_FORMAT_PCAT	0x1
+#define PART_FORMAT_GPT		0x2
+
 /*
  * Identifies the partition table type (ie. MBR vs GPT GUID) signature
  */
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 13f0c24058..dbcc296e01 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -503,7 +503,7 @@ efi_status_t efi_init_variables(void);
 void efi_variables_boot_exit_notify(void);
 efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
 /* Measure efi application invocation */
-efi_status_t efi_tcg2_measure_efi_app_invocation(void);
+efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle);
 /* Measure efi application exit */
 efi_status_t efi_tcg2_measure_efi_app_exit(void);
 /* Called by bootefi to initialize root node */
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index 85a032dbbd..0ecc7f99d7 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -230,6 +230,18 @@ struct smbios_handoff_table_pointers2 {
 	struct efi_configuration_table table_entry[];
 } __packed;
 
+/**
+ * struct tdUEFI_GPT_DATA - event log structure of industry standard tables
+ * @uefi_partition_header:	gpt partition header
+ * @number_of_partitions:	the number of partition
+ * @partitions:			partition entries
+ */
+struct efi_gpt_data {
+	gpt_header uefi_partition_header;
+	u64 number_of_partitions;
+	gpt_entry partitions[];
+} __packed;
+
 struct efi_tcg2_protocol {
 	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
 					       struct efi_tcg2_boot_service_capability *capability);
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 701e2212c8..bf5661e1ee 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -3003,7 +3003,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 
 	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
 		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
-			ret = efi_tcg2_measure_efi_app_invocation();
+			ret = efi_tcg2_measure_efi_app_invocation(image_obj);
 			if (ret != EFI_SUCCESS) {
 				log_warning("tcg2 measurement fails(0x%lx)\n",
 					    ret);
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 4f68f6dfd5..ea2c1ead03 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1525,12 +1525,181 @@ static void *find_smbios_table(void)
 	return NULL;
 }
 
+/**
+ * search_gpt_dp_node() - search gpt device path node
+ *
+ * @device_path:	device path
+ *
+ * Return:	pointer to the gpt device path node
+ */
+static struct
+efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
+{
+	struct efi_device_path *dp = device_path;
+
+	while (dp) {
+		if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
+		    dp->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
+			struct efi_device_path_hard_drive_path *hd_dp =
+				(struct efi_device_path_hard_drive_path *)dp;
+
+			if (hd_dp->partmap_type == PART_FORMAT_GPT &&
+			    hd_dp->signature_type == SIG_TYPE_GUID)
+				return dp;
+		}
+		dp = efi_dp_next(dp);
+	}
+
+	return NULL;
+}
+
+/**
+ * tcg2_measure_gpt_table() - measure gpt table
+ *
+ * @dev:		TPM device
+ * @loaded_image:	handle to the loaded image
+ *
+ * Return:	status code
+ */
+static efi_status_t
+tcg2_measure_gpt_data(struct udevice *dev,
+		      struct efi_loaded_image_obj *loaded_image)
+{
+	efi_status_t ret;
+	efi_handle_t handle;
+	struct efi_handler *dp_handler;
+	struct efi_device_path *orig_device_path;
+	struct efi_device_path *device_path;
+	struct efi_device_path *dp;
+	struct efi_block_io *block_io;
+	struct efi_gpt_data *event = NULL;
+	efi_guid_t null_guid = NULL_GUID;
+	gpt_header *orig_gpt_h = NULL;
+	gpt_entry *orig_gpt_e = NULL;
+	gpt_header *gpt_h = NULL;
+	gpt_entry *entry = NULL;
+	gpt_entry *gpt_e;
+	u32 num_of_valid_entry = 0;
+	u32 event_size;
+	u32 i;
+	u32 total_gpt_entry_size;
+
+	ret = efi_search_protocol(&loaded_image->header,
+				  &efi_guid_loaded_image_device_path,
+				  &dp_handler);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	orig_device_path = dp_handler->protocol_interface;
+	device_path = efi_dp_dup(orig_device_path);
+	if (!device_path)
+		return EFI_OUT_OF_RESOURCES;
+
+	dp = search_gpt_dp_node(device_path);
+	if (!dp) {
+		/* no GPT device path node found, skip GPT measurement */
+		ret = EFI_SUCCESS;
+		goto out1;
+	}
+
+	/* read GPT header */
+	dp->type = DEVICE_PATH_TYPE_END;
+	dp->sub_type = DEVICE_PATH_SUB_TYPE_END;
+	dp = device_path;
+	ret = EFI_CALL(systab.boottime->locate_device_path(&efi_block_io_guid,
+							   &dp, &handle));
+	if (ret != EFI_SUCCESS)
+		goto out1;
+
+	ret = EFI_CALL(efi_handle_protocol(handle,
+					   &efi_block_io_guid, (void **)&block_io));
+	if (ret != EFI_SUCCESS)
+		goto out1;
+
+	orig_gpt_h = calloc(1, (block_io->media->block_size + block_io->media->io_align));
+	if (!orig_gpt_h) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out2;
+	}
+
+	gpt_h = (gpt_header *)ALIGN((uintptr_t)orig_gpt_h, block_io->media->io_align);
+	ret = block_io->read_blocks(block_io, block_io->media->media_id, 1,
+				    block_io->media->block_size, gpt_h);
+	if (ret != EFI_SUCCESS)
+		goto out2;
+
+	/* read GPT entry */
+	total_gpt_entry_size = gpt_h->num_partition_entries *
+			       gpt_h->sizeof_partition_entry;
+	orig_gpt_e = calloc(1, total_gpt_entry_size + block_io->media->io_align);
+	entry = (void *)ALIGN((uintptr_t)orig_gpt_e, block_io->media->io_align);
+	if (!entry) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out2;
+	}
+
+	ret = block_io->read_blocks(block_io, block_io->media->media_id,
+				    gpt_h->partition_entry_lba,
+				    total_gpt_entry_size, entry);
+	if (ret != EFI_SUCCESS)
+		goto out2;
+
+	/* count valid GPT entry */
+	gpt_e = entry;
+	for (i = 0; i < gpt_h->num_partition_entries; i++) {
+		if (guidcmp(&null_guid, &gpt_e->partition_type_guid))
+			num_of_valid_entry++;
+
+		gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
+	}
+
+	/* prepare event data for measurement */
+	event_size = sizeof(struct efi_gpt_data) +
+		(num_of_valid_entry * gpt_h->sizeof_partition_entry);
+	event = calloc(1, event_size);
+	if (!event) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out2;
+	}
+	memcpy(event, gpt_h, sizeof(gpt_header));
+	put_unaligned_le64(num_of_valid_entry, &event->number_of_partitions);
+
+	/* copy valid GPT entry */
+	gpt_e = entry;
+	num_of_valid_entry = 0;
+	for (i = 0; i < gpt_h->num_partition_entries; i++) {
+		if (guidcmp(&null_guid, &gpt_e->partition_type_guid)) {
+			memcpy((u8 *)event->partitions +
+			       (num_of_valid_entry * gpt_h->sizeof_partition_entry),
+			       gpt_e, gpt_h->sizeof_partition_entry);
+			num_of_valid_entry++;
+		}
+
+		gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
+	}
+
+	ret = tcg2_measure_event(dev, 5, EV_EFI_GPT_EVENT, event_size, (u8 *)event);
+	if (ret != EFI_SUCCESS)
+		goto out2;
+
+out2:
+	EFI_CALL(efi_close_protocol((efi_handle_t)block_io, &efi_block_io_guid,
+				    NULL, NULL));
+	free(orig_gpt_h);
+	free(orig_gpt_e);
+	free(event);
+out1:
+	efi_free_pool(device_path);
+
+	return ret;
+}
+
 /**
  * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
  *
  * Return:	status code
  */
-efi_status_t efi_tcg2_measure_efi_app_invocation(void)
+efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle)
 {
 	efi_status_t ret;
 	u32 pcr_index;
@@ -1569,6 +1738,10 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
 			goto out;
 	}
 
+	ret = tcg2_measure_gpt_data(dev, handle);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 	tcg2_efi_app_invoked = true;
 out:
 	return ret;
-- 
2.17.1

[PATCH v2 3/3] efi_loader: add DeployedMode and AuditMode variable measurement

[Masahisa Kojima]

This commit adds the DeployedMode and AuditMode variable
measurement required in TCG PC Client PFP Spec.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---

(no changes since v1)

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

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index ea2c1ead03..68542c7cd3 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -12,6 +12,7 @@
 #include <dm.h>
 #include <efi_loader.h>
 #include <efi_tcg2.h>
+#include <efi_variable.h>
 #include <log.h>
 #include <malloc.h>
 #include <smbios.h>
@@ -1828,6 +1829,50 @@ out:
 	return ret;
 }
 
+/**
+ * tcg2_measure_deployed_audit_mode() - measure deployedmode and auditmode
+ *
+ * @dev:	TPM device
+ *
+ * Return:	status code
+ */
+static efi_status_t tcg2_measure_deployed_audit_mode(struct udevice *dev)
+{
+	u8 deployed_mode;
+	u8 audit_mode;
+	efi_uintn_t size;
+	efi_status_t ret;
+	u32 pcr_index;
+
+	size = sizeof(deployed_mode);
+	ret = efi_get_variable_int(L"DeployedMode", &efi_global_variable_guid,
+				   NULL, &size, &deployed_mode, NULL);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	pcr_index = (deployed_mode ? 1 : 7);
+
+	ret = tcg2_measure_variable(dev, pcr_index,
+				    EV_EFI_VARIABLE_DRIVER_CONFIG,
+				    L"DeployedMode",
+				    &efi_global_variable_guid,
+				    size, &deployed_mode);
+
+	size = sizeof(audit_mode);
+	ret = efi_get_variable_int(L"AuditMode", &efi_global_variable_guid,
+				   NULL, &size, &audit_mode, NULL);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = tcg2_measure_variable(dev, pcr_index,
+				    EV_EFI_VARIABLE_DRIVER_CONFIG,
+				    L"AuditMode",
+				    &efi_global_variable_guid,
+				    size, &audit_mode);
+
+	return ret;
+}
+
 /**
  * tcg2_measure_secure_boot_variable() - measure secure boot variables
  *
@@ -1891,6 +1936,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
 		free(data);
 	}
 
+	ret = tcg2_measure_deployed_audit_mode(dev);
+
 error:
 	return ret;
 }
-- 
2.17.1

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Simon Glass]

Hi Masahisa,

On Tue, 21 Sept 2021 at 01:17, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> TCG PC Client spec requires to measure the SMBIOS
> table that contain static configuration information
> (e.g. Platform Manufacturer Enterprise Number assigned by IANA,
> platform model number, Vendor and Device IDs for each SMBIOS table).
>
> The device and environment dependent information such as

device- and environment-dependent

> serial number is cleared to zero or space character for
> the measurement.
>
> Existing smbios_string() function returns pointer to the string
> with const qualifier, but exisintg use case is updating version
> string and const qualifier must be removed.
> This commit removes const qualifier from smbios_string()
> return value and reuses to clear the strings for the measurement.
>
> This commit also fixes the following compiler warning:
>
> lib/smbios-parser.c:59:39: warning: cast to pointer from integer of
> different size [-Wint-to-pointer-cast]
>   const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>
> Changes in v2:
> - use flexible array for table_entry field
> - modify funtion name to find_smbios_table()
> - remove unnecessary const qualifier from smbios_string()
> - create non-const version of next_header()
>
>  include/efi_loader.h          |   2 +
>  include/efi_tcg2.h            |  15 ++++
>  include/smbios.h              |  17 +++-
>  lib/efi_loader/Kconfig        |   1 +
>  lib/efi_loader/efi_boottime.c |   2 +
>  lib/efi_loader/efi_smbios.c   |   2 -
>  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
>  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
>  8 files changed, 261 insertions(+), 14 deletions(-)

Where are the tests for this new code, please?

Would it make sense to have a function that iterates through the data
that does need to be hashed, instead?

Regards,
Simon

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Ilias Apalodimas]

Hi Simon,

On Wed, 22 Sept 2021 at 19:19, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Tue, 21 Sept 2021 at 01:17, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > TCG PC Client spec requires to measure the SMBIOS
> > table that contain static configuration information
> > (e.g. Platform Manufacturer Enterprise Number assigned by IANA,
> > platform model number, Vendor and Device IDs for each SMBIOS table).
> >
> > The device and environment dependent information such as
>
> device- and environment-dependent
>
> > serial number is cleared to zero or space character for
> > the measurement.
> >
> > Existing smbios_string() function returns pointer to the string
> > with const qualifier, but exisintg use case is updating version
> > string and const qualifier must be removed.
> > This commit removes const qualifier from smbios_string()
> > return value and reuses to clear the strings for the measurement.
> >
> > This commit also fixes the following compiler warning:
> >
> > lib/smbios-parser.c:59:39: warning: cast to pointer from integer of
> > different size [-Wint-to-pointer-cast]
> >   const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >
> > Changes in v2:
> > - use flexible array for table_entry field
> > - modify funtion name to find_smbios_table()
> > - remove unnecessary const qualifier from smbios_string()
> > - create non-const version of next_header()
> >
> >  include/efi_loader.h          |   2 +
> >  include/efi_tcg2.h            |  15 ++++
> >  include/smbios.h              |  17 +++-
> >  lib/efi_loader/Kconfig        |   1 +
> >  lib/efi_loader/efi_boottime.c |   2 +
> >  lib/efi_loader/efi_smbios.c   |   2 -
> >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> >  8 files changed, 261 insertions(+), 14 deletions(-)
>
> Where are the tests for this new code, please?

We've mentioned this in the past.  The sandbox TPM is very limited wrt
tpm testing for the EFI TCG protocol.
I did send TPM MMIO patches a while back [1].  This would allow us to
test everything under QEMU,  but you asked for *another* device to be
part of the API I posted (apart from the MMIO).  I've found some time
and changed the tpm2 spi driver we have,  but I can't test it yet,
since I don't have a device for that.

[1] https://lore.kernel.org/u-boot/20210707162604.84196-1-ilias.apalodimas@linaro.org/

Cheers
/Ilias
>
> Would it make sense to have a function that iterates through the data
> that does need to be hashed, instead?
>
> Regards,
> Simon

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Simon Glass]

Hi Ilias,

On Thu, 23 Sept 2021 at 03:17, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> On Wed, 22 Sept 2021 at 19:19, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Masahisa,
> >
> > On Tue, 21 Sept 2021 at 01:17, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > TCG PC Client spec requires to measure the SMBIOS
> > > table that contain static configuration information
> > > (e.g. Platform Manufacturer Enterprise Number assigned by IANA,
> > > platform model number, Vendor and Device IDs for each SMBIOS table).
> > >
> > > The device and environment dependent information such as
> >
> > device- and environment-dependent
> >
> > > serial number is cleared to zero or space character for
> > > the measurement.
> > >
> > > Existing smbios_string() function returns pointer to the string
> > > with const qualifier, but exisintg use case is updating version
> > > string and const qualifier must be removed.
> > > This commit removes const qualifier from smbios_string()
> > > return value and reuses to clear the strings for the measurement.
> > >
> > > This commit also fixes the following compiler warning:
> > >
> > > lib/smbios-parser.c:59:39: warning: cast to pointer from integer of
> > > different size [-Wint-to-pointer-cast]
> > >   const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >
> > > Changes in v2:
> > > - use flexible array for table_entry field
> > > - modify funtion name to find_smbios_table()
> > > - remove unnecessary const qualifier from smbios_string()
> > > - create non-const version of next_header()
> > >
> > >  include/efi_loader.h          |   2 +
> > >  include/efi_tcg2.h            |  15 ++++
> > >  include/smbios.h              |  17 +++-
> > >  lib/efi_loader/Kconfig        |   1 +
> > >  lib/efi_loader/efi_boottime.c |   2 +
> > >  lib/efi_loader/efi_smbios.c   |   2 -
> > >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> > >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> > >  8 files changed, 261 insertions(+), 14 deletions(-)
> >
> > Where are the tests for this new code, please?
>
> We've mentioned this in the past.  The sandbox TPM is very limited wrt
> tpm testing for the EFI TCG protocol.

So let's add some more features? If it helps, think of the sandbox TPM
as test code, not an emulator. It is a very simple kind of emulator to
allow tests to work.

> I did send TPM MMIO patches a while back [1].  This would allow us to
> test everything under QEMU,  but you asked for *another* device to be
> part of the API I posted (apart from the MMIO).  I've found some time

Yes that is because if you just add a new protocol you have not made
anything better, just added one more way of doing things.

> and changed the tpm2 spi driver we have,  but I can't test it yet,
> since I don't have a device for that.

OK I think we are both going to get one.

[..]

Regards,
SImon

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Ilias Apalodimas]

Hi Simon,

[...]

> > > > - remove unnecessary const qualifier from smbios_string()
> > > > - create non-const version of next_header()
> > > >
> > > >  include/efi_loader.h          |   2 +
> > > >  include/efi_tcg2.h            |  15 ++++
> > > >  include/smbios.h              |  17 +++-
> > > >  lib/efi_loader/Kconfig        |   1 +
> > > >  lib/efi_loader/efi_boottime.c |   2 +
> > > >  lib/efi_loader/efi_smbios.c   |   2 -
> > > >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> > > >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> > > >  8 files changed, 261 insertions(+), 14 deletions(-)
> > >
> > > Where are the tests for this new code, please?
> >
> > We've mentioned this in the past.  The sandbox TPM is very limited wrt
> > tpm testing for the EFI TCG protocol.
> 
> So let's add some more features? If it helps, think of the sandbox TPM
> as test code, not an emulator. It is a very simple kind of emulator to
> allow tests to work.

The amount of features needed to test EFI TCG are not minimal.  Since I'll
upstream the mmio tpm anyway,  we'll just test TCG there.  If someone wants
to go ahead and make the sandbox TPM a TIS compliant device that covers the
requirements of the EFI TCG,  I am fine using it.

> 
> > I did send TPM MMIO patches a while back [1].  This would allow us to
> > test everything under QEMU,  but you asked for *another* device to be
> > part of the API I posted (apart from the MMIO).  I've found some time
> 
> Yes that is because if you just add a new protocol you have not made
> anything better, just added one more way of doing things.

Our perspective of 'better' seems to be different. 

I added a TIS API for any driver to use.  I actually did 2 iterations of
the driver.  The first one was replicating all the code and you said 'why
are we replicating code',  which was done already in a bunch of drivers
already...
Then I added an API and a driver using it but you wanted to convert more 
*existing* drivers to the API before merging it. But the fact is that if
anyone wants to add a new driver he has to code  ~900 lines instead of the
~150 needed with the API in place,  not to mention the duplication of bugs
all over the place....

> 
> > and changed the tpm2 spi driver we have,  but I can't test it yet,
> > since I don't have a device for that.
> 
> OK I think we are both going to get one.
> 
> [..]
> 
> Regards,
> SImon

Regards
/Ilias

Re: [PATCH v2 3/3] efi_loader: add DeployedMode and AuditMode variable measurement

[Ilias Apalodimas]

On Tue, 21 Sept 2021 at 10:17, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This commit adds the DeployedMode and AuditMode variable
> measurement required in TCG PC Client PFP Spec.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>
> (no changes since v1)
>
>  lib/efi_loader/efi_tcg2.c | 47 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index ea2c1ead03..68542c7cd3 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -12,6 +12,7 @@
>  #include <dm.h>
>  #include <efi_loader.h>
>  #include <efi_tcg2.h>
> +#include <efi_variable.h>
>  #include <log.h>
>  #include <malloc.h>
>  #include <smbios.h>
> @@ -1828,6 +1829,50 @@ out:
>         return ret;
>  }
>
> +/**
> + * tcg2_measure_deployed_audit_mode() - measure deployedmode and auditmode
> + *
> + * @dev:       TPM device
> + *
> + * Return:     status code
> + */
> +static efi_status_t tcg2_measure_deployed_audit_mode(struct udevice *dev)
> +{
> +       u8 deployed_mode;
> +       u8 audit_mode;
> +       efi_uintn_t size;
> +       efi_status_t ret;
> +       u32 pcr_index;
> +
> +       size = sizeof(deployed_mode);
> +       ret = efi_get_variable_int(L"DeployedMode", &efi_global_variable_guid,
> +                                  NULL, &size, &deployed_mode, NULL);
> +       if (ret != EFI_SUCCESS)
> +               return ret;
> +
> +       pcr_index = (deployed_mode ? 1 : 7);
> +
> +       ret = tcg2_measure_variable(dev, pcr_index,
> +                                   EV_EFI_VARIABLE_DRIVER_CONFIG,
> +                                   L"DeployedMode",
> +                                   &efi_global_variable_guid,
> +                                   size, &deployed_mode);
> +

tcg2_measure_variable() can't fail here?  Do we care if it does?

> +       size = sizeof(audit_mode);
> +       ret = efi_get_variable_int(L"AuditMode", &efi_global_variable_guid,
> +                                  NULL, &size, &audit_mode, NULL);
> +       if (ret != EFI_SUCCESS)
> +               return ret;
> +
> +       ret = tcg2_measure_variable(dev, pcr_index,
> +                                   EV_EFI_VARIABLE_DRIVER_CONFIG,
> +                                   L"AuditMode",
> +                                   &efi_global_variable_guid,
> +                                   size, &audit_mode);
> +

Does it make sense to read both of the variables first and measure
them only if both are present?
IOW is there any connection between AuditMode and DeployedMode measurements?


Regards
/Ilias
> +       return ret;
> +}
> +
>  /**
>   * tcg2_measure_secure_boot_variable() - measure secure boot variables
>   *
> @@ -1891,6 +1936,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
>                 free(data);
>         }
>
> +       ret = tcg2_measure_deployed_audit_mode(dev);
> +
>  error:
>         return ret;
>  }
> --
> 2.17.1
>

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Simon Glass]

Hi Ilias,

On Mon, 27 Sept 2021 at 02:52, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > > > - remove unnecessary const qualifier from smbios_string()
> > > > > - create non-const version of next_header()
> > > > >
> > > > >  include/efi_loader.h          |   2 +
> > > > >  include/efi_tcg2.h            |  15 ++++
> > > > >  include/smbios.h              |  17 +++-
> > > > >  lib/efi_loader/Kconfig        |   1 +
> > > > >  lib/efi_loader/efi_boottime.c |   2 +
> > > > >  lib/efi_loader/efi_smbios.c   |   2 -
> > > > >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> > > > >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> > > > >  8 files changed, 261 insertions(+), 14 deletions(-)
> > > >
> > > > Where are the tests for this new code, please?
> > >
> > > We've mentioned this in the past.  The sandbox TPM is very limited wrt
> > > tpm testing for the EFI TCG protocol.
> >
> > So let's add some more features? If it helps, think of the sandbox TPM
> > as test code, not an emulator. It is a very simple kind of emulator to
> > allow tests to work.
>
> The amount of features needed to test EFI TCG are not minimal.  Since I'll
> upstream the mmio tpm anyway,  we'll just test TCG there.  If someone wants
> to go ahead and make the sandbox TPM a TIS compliant device that covers the
> requirements of the EFI TCG,  I am fine using it.

Do you know how many features? There's 250 LOC in this patch.

>
> >
> > > I did send TPM MMIO patches a while back [1].  This would allow us to
> > > test everything under QEMU,  but you asked for *another* device to be
> > > part of the API I posted (apart from the MMIO).  I've found some time
> >
> > Yes that is because if you just add a new protocol you have not made
> > anything better, just added one more way of doing things.
>
> Our perspective of 'better' seems to be different.
>
> I added a TIS API for any driver to use.  I actually did 2 iterations of
> the driver.  The first one was replicating all the code and you said 'why
> are we replicating code',  which was done already in a bunch of drivers
> already...
> Then I added an API and a driver using it but you wanted to convert more
> *existing* drivers to the API before merging it. But the fact is that if
> anyone wants to add a new driver he has to code  ~900 lines instead of the
> ~150 needed with the API in place,  not to mention the duplication of bugs
> all over the place....

It would be like adding a new filesystem in U-Boot with its own new
framework for filesystems. It creates technical debt and we don't know
if anyone will actually use it.

https://xkcd.com/927/

I think your API is a great idea but we need some effort to migrate to
it, to avoid the problem above. After all, who else is going to do it?

Regards,
Simon

Re: [PATCH v2 2/3] efi_loader: add UEFI GPT measurement

[Ilias Apalodimas]

On Tue, Sep 21, 2021 at 04:19:30PM +0900, Masahisa Kojima wrote:
> This commit adds the UEFI GPT disk partition topology
> measurement required in TCG PC Client PFP Spec.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> 
> (no changes since v1)
> 
>  include/blk.h                 |   3 +
>  include/efi_loader.h          |   2 +-
>  include/efi_tcg2.h            |  12 +++
>  lib/efi_loader/efi_boottime.c |   2 +-
>  lib/efi_loader/efi_tcg2.c     | 175 +++++++++++++++++++++++++++++++++-
>  5 files changed, 191 insertions(+), 3 deletions(-)
> 
> diff --git a/include/blk.h b/include/blk.h
> index 19bab081c2..f0cc7ca1a2 100644
> --- a/include/blk.h
> +++ b/include/blk.h
> @@ -45,6 +45,9 @@ enum if_type {
>  #define BLK_PRD_SIZE		20
>  #define BLK_REV_SIZE		8
>  
> +#define PART_FORMAT_PCAT	0x1

This isn't used anywhere

> +#define PART_FORMAT_GPT		0x2
> +
>  /*
>   * Identifies the partition table type (ie. MBR vs GPT GUID) signature
>   */
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 13f0c24058..dbcc296e01 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -503,7 +503,7 @@ efi_status_t efi_init_variables(void);
>  void efi_variables_boot_exit_notify(void);
>  efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
>  /* Measure efi application invocation */
> -efi_status_t efi_tcg2_measure_efi_app_invocation(void);
> +efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle);
>  /* Measure efi application exit */
>  efi_status_t efi_tcg2_measure_efi_app_exit(void);
>  /* Called by bootefi to initialize root node */
> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> index 85a032dbbd..0ecc7f99d7 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -230,6 +230,18 @@ struct smbios_handoff_table_pointers2 {
>  	struct efi_configuration_table table_entry[];
>  } __packed;
>  
> +/**
> + * struct tdUEFI_GPT_DATA - event log structure of industry standard tables
> + * @uefi_partition_header:	gpt partition header
> + * @number_of_partitions:	the number of partition
> + * @partitions:			partition entries
> + */
> +struct efi_gpt_data {
> +	gpt_header uefi_partition_header;
> +	u64 number_of_partitions;
> +	gpt_entry partitions[];
> +} __packed;
> +
>  struct efi_tcg2_protocol {
>  	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
>  					       struct efi_tcg2_boot_service_capability *capability);
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 701e2212c8..bf5661e1ee 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -3003,7 +3003,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>  
>  	if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
>  		if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> -			ret = efi_tcg2_measure_efi_app_invocation();
> +			ret = efi_tcg2_measure_efi_app_invocation(image_obj);
>  			if (ret != EFI_SUCCESS) {
>  				log_warning("tcg2 measurement fails(0x%lx)\n",
>  					    ret);
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 4f68f6dfd5..ea2c1ead03 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1525,12 +1525,181 @@ static void *find_smbios_table(void)
>  	return NULL;
>  }
>  
> +/**
> + * search_gpt_dp_node() - search gpt device path node
> + *
> + * @device_path:	device path
> + *
> + * Return:	pointer to the gpt device path node
> + */
> +static struct
> +efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
> +{
> +	struct efi_device_path *dp = device_path;
> +
> +	while (dp) {
> +		if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> +		    dp->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
> +			struct efi_device_path_hard_drive_path *hd_dp =
> +				(struct efi_device_path_hard_drive_path *)dp;
> +
> +			if (hd_dp->partmap_type == PART_FORMAT_GPT &&

On dp_part_node() partmap_type is set to 2 for EFI partition types.  Why do
we interpret it as 'GPT' here?

> +			    hd_dp->signature_type == SIG_TYPE_GUID)
> +				return dp;
> +		}
> +		dp = efi_dp_next(dp);
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * tcg2_measure_gpt_table() - measure gpt table
> + *
> + * @dev:		TPM device
> + * @loaded_image:	handle to the loaded image
> + *
> + * Return:	status code
> + */
> +static efi_status_t
> +tcg2_measure_gpt_data(struct udevice *dev,
> +		      struct efi_loaded_image_obj *loaded_image)
> +{
> +	efi_status_t ret;
> +	efi_handle_t handle;
> +	struct efi_handler *dp_handler;
> +	struct efi_device_path *orig_device_path;
> +	struct efi_device_path *device_path;
> +	struct efi_device_path *dp;
> +	struct efi_block_io *block_io;
> +	struct efi_gpt_data *event = NULL;
> +	efi_guid_t null_guid = NULL_GUID;
> +	gpt_header *orig_gpt_h = NULL;
> +	gpt_entry *orig_gpt_e = NULL;
> +	gpt_header *gpt_h = NULL;
> +	gpt_entry *entry = NULL;
> +	gpt_entry *gpt_e;
> +	u32 num_of_valid_entry = 0;
> +	u32 event_size;
> +	u32 i;
> +	u32 total_gpt_entry_size;
> +
> +	ret = efi_search_protocol(&loaded_image->header,
> +				  &efi_guid_loaded_image_device_path,
> +				  &dp_handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	orig_device_path = dp_handler->protocol_interface;
> +	device_path = efi_dp_dup(orig_device_path);
> +	if (!device_path)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	dp = search_gpt_dp_node(device_path);
> +	if (!dp) {
> +		/* no GPT device path node found, skip GPT measurement */
> +		ret = EFI_SUCCESS;
> +		goto out1;
> +	}
> +
> +	/* read GPT header */
> +	dp->type = DEVICE_PATH_TYPE_END;
> +	dp->sub_type = DEVICE_PATH_SUB_TYPE_END;
> +	dp = device_path;
> +	ret = EFI_CALL(systab.boottime->locate_device_path(&efi_block_io_guid,
> +							   &dp, &handle));
> +	if (ret != EFI_SUCCESS)
> +		goto out1;
> +
> +	ret = EFI_CALL(efi_handle_protocol(handle,
> +					   &efi_block_io_guid, (void **)&block_io));
> +	if (ret != EFI_SUCCESS)
> +		goto out1;
> +
> +	orig_gpt_h = calloc(1, (block_io->media->block_size + block_io->media->io_align));
> +	if (!orig_gpt_h) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out2;
> +	}
> +
> +	gpt_h = (gpt_header *)ALIGN((uintptr_t)orig_gpt_h, block_io->media->io_align);
> +	ret = block_io->read_blocks(block_io, block_io->media->media_id, 1,
> +				    block_io->media->block_size, gpt_h);
> +	if (ret != EFI_SUCCESS)
> +		goto out2;
> +
> +	/* read GPT entry */
> +	total_gpt_entry_size = gpt_h->num_partition_entries *
> +			       gpt_h->sizeof_partition_entry;
> +	orig_gpt_e = calloc(1, total_gpt_entry_size + block_io->media->io_align);
> +	entry = (void *)ALIGN((uintptr_t)orig_gpt_e, block_io->media->io_align);
> +	if (!entry) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out2;
> +	}
> +
> +	ret = block_io->read_blocks(block_io, block_io->media->media_id,
> +				    gpt_h->partition_entry_lba,
> +				    total_gpt_entry_size, entry);
> +	if (ret != EFI_SUCCESS)
> +		goto out2;
> +

Can't you use efi_bl_read() here, instead of searching for the protocol? 
If you can, moving that out of a static function would make more sense. 

> +	/* count valid GPT entry */
> +	gpt_e = entry;
> +	for (i = 0; i < gpt_h->num_partition_entries; i++) {
> +		if (guidcmp(&null_guid, &gpt_e->partition_type_guid))
> +			num_of_valid_entry++;
> +
> +		gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> +	}
> +
> +	/* prepare event data for measurement */
> +	event_size = sizeof(struct efi_gpt_data) +
> +		(num_of_valid_entry * gpt_h->sizeof_partition_entry);
> +	event = calloc(1, event_size);
> +	if (!event) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out2;
> +	}
> +	memcpy(event, gpt_h, sizeof(gpt_header));
> +	put_unaligned_le64(num_of_valid_entry, &event->number_of_partitions);
> +
> +	/* copy valid GPT entry */
> +	gpt_e = entry;
> +	num_of_valid_entry = 0;
> +	for (i = 0; i < gpt_h->num_partition_entries; i++) {
> +		if (guidcmp(&null_guid, &gpt_e->partition_type_guid)) {
> +			memcpy((u8 *)event->partitions +
> +			       (num_of_valid_entry * gpt_h->sizeof_partition_entry),
> +			       gpt_e, gpt_h->sizeof_partition_entry);
> +			num_of_valid_entry++;
> +		}
> +
> +		gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> +	}
> +
> +	ret = tcg2_measure_event(dev, 5, EV_EFI_GPT_EVENT, event_size, (u8 *)event);
> +	if (ret != EFI_SUCCESS)
> +		goto out2;
> +
> +out2:
> +	EFI_CALL(efi_close_protocol((efi_handle_t)block_io, &efi_block_io_guid,
> +				    NULL, NULL));
> +	free(orig_gpt_h);
> +	free(orig_gpt_e);
> +	free(event);
> +out1:
> +	efi_free_pool(device_path);
> +
> +	return ret;
> +}
> +
>  /**
>   * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
>   *
>   * Return:	status code
>   */
> -efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> +efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle)
>  {
>  	efi_status_t ret;
>  	u32 pcr_index;
> @@ -1569,6 +1738,10 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
>  			goto out;
>  	}
>  
> +	ret = tcg2_measure_gpt_data(dev, handle);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>  	tcg2_efi_app_invoked = true;
>  out:
>  	return ret;
> -- 
> 2.17.1
> 

Cheers
/Ilias

Re: [PATCH v2 3/3] efi_loader: add DeployedMode and AuditMode variable measurement

[Masahisa Kojima]

On Mon, 27 Sept 2021 at 22:53, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Tue, 21 Sept 2021 at 10:17, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This commit adds the DeployedMode and AuditMode variable
> > measurement required in TCG PC Client PFP Spec.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >
> > (no changes since v1)
> >
> >  lib/efi_loader/efi_tcg2.c | 47 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index ea2c1ead03..68542c7cd3 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -12,6 +12,7 @@
> >  #include <dm.h>
> >  #include <efi_loader.h>
> >  #include <efi_tcg2.h>
> > +#include <efi_variable.h>
> >  #include <log.h>
> >  #include <malloc.h>
> >  #include <smbios.h>
> > @@ -1828,6 +1829,50 @@ out:
> >         return ret;
> >  }
> >
> > +/**
> > + * tcg2_measure_deployed_audit_mode() - measure deployedmode and auditmode
> > + *
> > + * @dev:       TPM device
> > + *
> > + * Return:     status code
> > + */
> > +static efi_status_t tcg2_measure_deployed_audit_mode(struct udevice *dev)
> > +{
> > +       u8 deployed_mode;
> > +       u8 audit_mode;
> > +       efi_uintn_t size;
> > +       efi_status_t ret;
> > +       u32 pcr_index;
> > +
> > +       size = sizeof(deployed_mode);
> > +       ret = efi_get_variable_int(L"DeployedMode", &efi_global_variable_guid,
> > +                                  NULL, &size, &deployed_mode, NULL);
> > +       if (ret != EFI_SUCCESS)
> > +               return ret;
> > +
> > +       pcr_index = (deployed_mode ? 1 : 7);
> > +
> > +       ret = tcg2_measure_variable(dev, pcr_index,
> > +                                   EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                   L"DeployedMode",
> > +                                   &efi_global_variable_guid,
> > +                                   size, &deployed_mode);
> > +
>
> tcg2_measure_variable() can't fail here?  Do we care if it does?

I will add appropriate error handling.

>
> > +       size = sizeof(audit_mode);
> > +       ret = efi_get_variable_int(L"AuditMode", &efi_global_variable_guid,
> > +                                  NULL, &size, &audit_mode, NULL);
> > +       if (ret != EFI_SUCCESS)
> > +               return ret;
> > +
> > +       ret = tcg2_measure_variable(dev, pcr_index,
> > +                                   EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                   L"AuditMode",
> > +                                   &efi_global_variable_guid,
> > +                                   size, &audit_mode);
> > +
>
> Does it make sense to read both of the variables first and measure
> them only if both are present?

Yes, it is better. If one of the variable is not present, skip both DeployedMode
and AuditMode measurement.

> IOW is there any connection between AuditMode and DeployedMode measurements?

In UEFI spec:
 DeployedMode = 1 -> AuditMode is always 0
 DeployedMode = 0 -> AuditMode can be 0 or 1

Thanks,
Masahisa Kojima

>
>
> Regards
> /Ilias
> > +       return ret;
> > +}
> > +
> >  /**
> >   * tcg2_measure_secure_boot_variable() - measure secure boot variables
> >   *
> > @@ -1891,6 +1936,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> >                 free(data);
> >         }
> >
> > +       ret = tcg2_measure_deployed_audit_mode(dev);
> > +
> >  error:
> >         return ret;
> >  }
> > --
> > 2.17.1
> >

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Ilias Apalodimas]

Hi Simon,


[...]
> > > > We've mentioned this in the past.  The sandbox TPM is very limited wrt
> > > > tpm testing for the EFI TCG protocol.
> > >
> > > So let's add some more features? If it helps, think of the sandbox TPM
> > > as test code, not an emulator. It is a very simple kind of emulator to
> > > allow tests to work.
> >
> > The amount of features needed to test EFI TCG are not minimal.  Since I'll
> > upstream the mmio tpm anyway,  we'll just test TCG there.  If someone wants
> > to go ahead and make the sandbox TPM a TIS compliant device that covers the
> > requirements of the EFI TCG,  I am fine using it.
>
> Do you know how many features? There's 250 LOC in this patch.

I haven't checked for a while but back when I tested it
tpm2_get_capability() was failing on a number of cases.  The EFI TCG
code expects:
- TPM2_PT_MAX_COMMAND_SIZE
- TPM2_PT_MAX_RESPONSE_SIZE
- TPM2_PT_MANUFACTURER
- TPM2_PT_PCR_COUNT
- TPM2_CAP_PCRS
when querying capabilities.  Ideally we'd also want to extend more
than 1 PCRs and verify that worked correctly.

>
> >
> > >
> > > > I did send TPM MMIO patches a while back [1].  This would allow us to
> > > > test everything under QEMU,  but you asked for *another* device to be
> > > > part of the API I posted (apart from the MMIO).  I've found some time
> > >
> > > Yes that is because if you just add a new protocol you have not made
> > > anything better, just added one more way of doing things.
> >
> > Our perspective of 'better' seems to be different.
> >
> > I added a TIS API for any driver to use.  I actually did 2 iterations of
> > the driver.  The first one was replicating all the code and you said 'why
> > are we replicating code',  which was done already in a bunch of drivers
> > already...
> > Then I added an API and a driver using it but you wanted to convert more
> > *existing* drivers to the API before merging it. But the fact is that if
> > anyone wants to add a new driver he has to code  ~900 lines instead of the
> > ~150 needed with the API in place,  not to mention the duplication of bugs
> > all over the place....
>
> It would be like adding a new filesystem in U-Boot with its own new
> framework for filesystems. It creates technical debt and we don't know
> if anyone will actually use it.
>
> https://xkcd.com/927/
>
> I think your API is a great idea but we need some effort to migrate to
> it, to avoid the problem above. After all, who else is going to do it?

I ordered the SPI TPM, so hopefully, I'll be able to have the MMIO and
SPI drivers using it!

Cheers
/Ilias

Re: [PATCH v2 2/3] efi_loader: add UEFI GPT measurement

[Masahisa Kojima]

On Tue, 28 Sept 2021 at 05:21, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Tue, Sep 21, 2021 at 04:19:30PM +0900, Masahisa Kojima wrote:
> > This commit adds the UEFI GPT disk partition topology
> > measurement required in TCG PC Client PFP Spec.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >
> > (no changes since v1)
> >
> >  include/blk.h                 |   3 +
> >  include/efi_loader.h          |   2 +-
> >  include/efi_tcg2.h            |  12 +++
> >  lib/efi_loader/efi_boottime.c |   2 +-
> >  lib/efi_loader/efi_tcg2.c     | 175 +++++++++++++++++++++++++++++++++-
> >  5 files changed, 191 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/blk.h b/include/blk.h
> > index 19bab081c2..f0cc7ca1a2 100644
> > --- a/include/blk.h
> > +++ b/include/blk.h
> > @@ -45,6 +45,9 @@ enum if_type {
> >  #define BLK_PRD_SIZE         20
> >  #define BLK_REV_SIZE         8
> >
> > +#define PART_FORMAT_PCAT     0x1
>
> This isn't used anywhere

UEFI spec says:
---
Partition Format:
  0x01 – PC-AT compatible legacy MBR
  0x02 – GUID Partition Table
---
I think it is better to define both value even if one of them is not used.

>
> > +#define PART_FORMAT_GPT              0x2
> > +
> >  /*
> >   * Identifies the partition table type (ie. MBR vs GPT GUID) signature
> >   */
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 13f0c24058..dbcc296e01 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -503,7 +503,7 @@ efi_status_t efi_init_variables(void);
> >  void efi_variables_boot_exit_notify(void);
> >  efi_status_t efi_tcg2_notify_exit_boot_services_failed(void);
> >  /* Measure efi application invocation */
> > -efi_status_t efi_tcg2_measure_efi_app_invocation(void);
> > +efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle);
> >  /* Measure efi application exit */
> >  efi_status_t efi_tcg2_measure_efi_app_exit(void);
> >  /* Called by bootefi to initialize root node */
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > index 85a032dbbd..0ecc7f99d7 100644
> > --- a/include/efi_tcg2.h
> > +++ b/include/efi_tcg2.h
> > @@ -230,6 +230,18 @@ struct smbios_handoff_table_pointers2 {
> >       struct efi_configuration_table table_entry[];
> >  } __packed;
> >
> > +/**
> > + * struct tdUEFI_GPT_DATA - event log structure of industry standard tables
> > + * @uefi_partition_header:   gpt partition header
> > + * @number_of_partitions:    the number of partition
> > + * @partitions:                      partition entries
> > + */
> > +struct efi_gpt_data {
> > +     gpt_header uefi_partition_header;
> > +     u64 number_of_partitions;
> > +     gpt_entry partitions[];
> > +} __packed;
> > +
> >  struct efi_tcg2_protocol {
> >       efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
> >                                              struct efi_tcg2_boot_service_capability *capability);
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 701e2212c8..bf5661e1ee 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -3003,7 +3003,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >
> >       if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> >               if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > -                     ret = efi_tcg2_measure_efi_app_invocation();
> > +                     ret = efi_tcg2_measure_efi_app_invocation(image_obj);
> >                       if (ret != EFI_SUCCESS) {
> >                               log_warning("tcg2 measurement fails(0x%lx)\n",
> >                                           ret);
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 4f68f6dfd5..ea2c1ead03 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1525,12 +1525,181 @@ static void *find_smbios_table(void)
> >       return NULL;
> >  }
> >
> > +/**
> > + * search_gpt_dp_node() - search gpt device path node
> > + *
> > + * @device_path:     device path
> > + *
> > + * Return:   pointer to the gpt device path node
> > + */
> > +static struct
> > +efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
> > +{
> > +     struct efi_device_path *dp = device_path;
> > +
> > +     while (dp) {
> > +             if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> > +                 dp->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
> > +                     struct efi_device_path_hard_drive_path *hd_dp =
> > +                             (struct efi_device_path_hard_drive_path *)dp;
> > +
> > +                     if (hd_dp->partmap_type == PART_FORMAT_GPT &&
>
> On dp_part_node() partmap_type is set to 2 for EFI partition types.  Why do
> we interpret it as 'GPT' here?

UEFI spec says:
 partmap_type = 0x02 – GUID Partition Table

>
> > +                         hd_dp->signature_type == SIG_TYPE_GUID)
> > +                             return dp;
> > +             }
> > +             dp = efi_dp_next(dp);
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +/**
> > + * tcg2_measure_gpt_table() - measure gpt table
> > + *
> > + * @dev:             TPM device
> > + * @loaded_image:    handle to the loaded image
> > + *
> > + * Return:   status code
> > + */
> > +static efi_status_t
> > +tcg2_measure_gpt_data(struct udevice *dev,
> > +                   struct efi_loaded_image_obj *loaded_image)
> > +{
> > +     efi_status_t ret;
> > +     efi_handle_t handle;
> > +     struct efi_handler *dp_handler;
> > +     struct efi_device_path *orig_device_path;
> > +     struct efi_device_path *device_path;
> > +     struct efi_device_path *dp;
> > +     struct efi_block_io *block_io;
> > +     struct efi_gpt_data *event = NULL;
> > +     efi_guid_t null_guid = NULL_GUID;
> > +     gpt_header *orig_gpt_h = NULL;
> > +     gpt_entry *orig_gpt_e = NULL;
> > +     gpt_header *gpt_h = NULL;
> > +     gpt_entry *entry = NULL;
> > +     gpt_entry *gpt_e;
> > +     u32 num_of_valid_entry = 0;
> > +     u32 event_size;
> > +     u32 i;
> > +     u32 total_gpt_entry_size;
> > +
> > +     ret = efi_search_protocol(&loaded_image->header,
> > +                               &efi_guid_loaded_image_device_path,
> > +                               &dp_handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     orig_device_path = dp_handler->protocol_interface;
> > +     device_path = efi_dp_dup(orig_device_path);
> > +     if (!device_path)
> > +             return EFI_OUT_OF_RESOURCES;
> > +
> > +     dp = search_gpt_dp_node(device_path);
> > +     if (!dp) {
> > +             /* no GPT device path node found, skip GPT measurement */
> > +             ret = EFI_SUCCESS;
> > +             goto out1;
> > +     }
> > +
> > +     /* read GPT header */
> > +     dp->type = DEVICE_PATH_TYPE_END;
> > +     dp->sub_type = DEVICE_PATH_SUB_TYPE_END;
> > +     dp = device_path;
> > +     ret = EFI_CALL(systab.boottime->locate_device_path(&efi_block_io_guid,
> > +                                                        &dp, &handle));
> > +     if (ret != EFI_SUCCESS)
> > +             goto out1;
> > +
> > +     ret = EFI_CALL(efi_handle_protocol(handle,
> > +                                        &efi_block_io_guid, (void **)&block_io));
> > +     if (ret != EFI_SUCCESS)
> > +             goto out1;
> > +
> > +     orig_gpt_h = calloc(1, (block_io->media->block_size + block_io->media->io_align));
> > +     if (!orig_gpt_h) {
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +             goto out2;
> > +     }
> > +
> > +     gpt_h = (gpt_header *)ALIGN((uintptr_t)orig_gpt_h, block_io->media->io_align);
> > +     ret = block_io->read_blocks(block_io, block_io->media->media_id, 1,
> > +                                 block_io->media->block_size, gpt_h);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out2;
> > +
> > +     /* read GPT entry */
> > +     total_gpt_entry_size = gpt_h->num_partition_entries *
> > +                            gpt_h->sizeof_partition_entry;
> > +     orig_gpt_e = calloc(1, total_gpt_entry_size + block_io->media->io_align);
> > +     entry = (void *)ALIGN((uintptr_t)orig_gpt_e, block_io->media->io_align);
> > +     if (!entry) {
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +             goto out2;
> > +     }
> > +
> > +     ret = block_io->read_blocks(block_io, block_io->media->media_id,
> > +                                 gpt_h->partition_entry_lba,
> > +                                 total_gpt_entry_size, entry);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out2;
> > +
>
> Can't you use efi_bl_read() here, instead of searching for the protocol?
> If you can, moving that out of a static function would make more sense.

With chat discussion with Ilias, we concluded not to do this modification.

Thanks,
Masahisa Kojima

>
> > +     /* count valid GPT entry */
> > +     gpt_e = entry;
> > +     for (i = 0; i < gpt_h->num_partition_entries; i++) {
> > +             if (guidcmp(&null_guid, &gpt_e->partition_type_guid))
> > +                     num_of_valid_entry++;
> > +
> > +             gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> > +     }
> > +
> > +     /* prepare event data for measurement */
> > +     event_size = sizeof(struct efi_gpt_data) +
> > +             (num_of_valid_entry * gpt_h->sizeof_partition_entry);
> > +     event = calloc(1, event_size);
> > +     if (!event) {
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +             goto out2;
> > +     }
> > +     memcpy(event, gpt_h, sizeof(gpt_header));
> > +     put_unaligned_le64(num_of_valid_entry, &event->number_of_partitions);
> > +
> > +     /* copy valid GPT entry */
> > +     gpt_e = entry;
> > +     num_of_valid_entry = 0;
> > +     for (i = 0; i < gpt_h->num_partition_entries; i++) {
> > +             if (guidcmp(&null_guid, &gpt_e->partition_type_guid)) {
> > +                     memcpy((u8 *)event->partitions +
> > +                            (num_of_valid_entry * gpt_h->sizeof_partition_entry),
> > +                            gpt_e, gpt_h->sizeof_partition_entry);
> > +                     num_of_valid_entry++;
> > +             }
> > +
> > +             gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> > +     }
> > +
> > +     ret = tcg2_measure_event(dev, 5, EV_EFI_GPT_EVENT, event_size, (u8 *)event);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out2;
> > +
> > +out2:
> > +     EFI_CALL(efi_close_protocol((efi_handle_t)block_io, &efi_block_io_guid,
> > +                                 NULL, NULL));
> > +     free(orig_gpt_h);
> > +     free(orig_gpt_e);
> > +     free(event);
> > +out1:
> > +     efi_free_pool(device_path);
> > +
> > +     return ret;
> > +}
> > +
> >  /**
> >   * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> >   *
> >   * Return:   status code
> >   */
> > -efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> > +efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle)
> >  {
> >       efi_status_t ret;
> >       u32 pcr_index;
> > @@ -1569,6 +1738,10 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> >                       goto out;
> >       }
> >
> > +     ret = tcg2_measure_gpt_data(dev, handle);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> >       tcg2_efi_app_invoked = true;
> >  out:
> >       return ret;
> > --
> > 2.17.1
> >
>
> Cheers
> /Ilias

Re: [PATCH v2 2/3] efi_loader: add UEFI GPT measurement

[Ilias Apalodimas]

On Fri, Oct 01, 2021 at 04:37:40PM +0900, Masahisa Kojima wrote:
> On Tue, 28 Sept 2021 at 05:21, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Tue, Sep 21, 2021 at 04:19:30PM +0900, Masahisa Kojima wrote:
> > > This commit adds the UEFI GPT disk partition topology
> > > measurement required in TCG PC Client PFP Spec.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >
> > > (no changes since v1)
> > >
> > >  include/blk.h                 |   3 +
> > >  include/efi_loader.h          |   2 +-
> > >  include/efi_tcg2.h            |  12 +++
> > >  lib/efi_loader/efi_boottime.c |   2 +-
> > >  lib/efi_loader/efi_tcg2.c     | 175 +++++++++++++++++++++++++++++++++-
> > >  5 files changed, 191 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/blk.h b/include/blk.h
> > > index 19bab081c2..f0cc7ca1a2 100644
> > > --- a/include/blk.h
> > > +++ b/include/blk.h
> > > @@ -45,6 +45,9 @@ enum if_type {
> > >  #define BLK_PRD_SIZE         20
> > >  #define BLK_REV_SIZE         8
> > >
> > > +#define PART_FORMAT_PCAT     0x1
> >
> > This isn't used anywhere
> 
> UEFI spec says:
> ---
> Partition Format:
>   0x01 – PC-AT compatible legacy MBR
>   0x02 – GUID Partition Table
> ---
> I think it is better to define both value even if one of them is not used.
> 

Fair enough

> >
> > > @@ -230,6 +230,18 @@ struct smbios_handoff_table_pointers2 {
> > >       struct efi_configuration_table table_entry[];
> > >  } __packed;
> > >
> > > +/**
> > > + * struct tdUEFI_GPT_DATA - event log structure of industry standard tables
> > > + * @uefi_partition_header:   gpt partition header
> > > + * @number_of_partitions:    the number of partition
> > > + * @partitions:                      partition entries
> > > + */
> > > +struct efi_gpt_data {
> > > +     gpt_header uefi_partition_header;
> > > +     u64 number_of_partitions;
> > > +     gpt_entry partitions[];
> > > +} __packed;
> > > +
> > >  struct efi_tcg2_protocol {
> > >       efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
> > >                                              struct efi_tcg2_boot_service_capability *capability);
> > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > > index 701e2212c8..bf5661e1ee 100644
> > > --- a/lib/efi_loader/efi_boottime.c
> > > +++ b/lib/efi_loader/efi_boottime.c
> > > @@ -3003,7 +3003,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > >
> > >       if (IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL)) {
> > >               if (image_obj->image_type == IMAGE_SUBSYSTEM_EFI_APPLICATION) {
> > > -                     ret = efi_tcg2_measure_efi_app_invocation();
> > > +                     ret = efi_tcg2_measure_efi_app_invocation(image_obj);
> > >                       if (ret != EFI_SUCCESS) {
> > >                               log_warning("tcg2 measurement fails(0x%lx)\n",
> > >                                           ret);
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index 4f68f6dfd5..ea2c1ead03 100644
> > > --- a/lib/efi_loader/efi_tcg2.c
> > > +++ b/lib/efi_loader/efi_tcg2.c
> > > @@ -1525,12 +1525,181 @@ static void *find_smbios_table(void)
> > >       return NULL;
> > >  }
> > >
> > > +/**
> > > + * search_gpt_dp_node() - search gpt device path node
> > > + *
> > > + * @device_path:     device path
> > > + *
> > > + * Return:   pointer to the gpt device path node
> > > + */
> > > +static struct
> > > +efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
> > > +{
> > > +     struct efi_device_path *dp = device_path;
> > > +
> > > +     while (dp) {
> > > +             if (dp->type == DEVICE_PATH_TYPE_MEDIA_DEVICE &&
> > > +                 dp->sub_type == DEVICE_PATH_SUB_TYPE_HARD_DRIVE_PATH) {
> > > +                     struct efi_device_path_hard_drive_path *hd_dp =
> > > +                             (struct efi_device_path_hard_drive_path *)dp;
> > > +
> > > +                     if (hd_dp->partmap_type == PART_FORMAT_GPT &&
> >
> > On dp_part_node() partmap_type is set to 2 for EFI partition types.  Why do
> > we interpret it as 'GPT' here?
> 
> UEFI spec says:
>  partmap_type = 0x02 – GUID Partition Table

Ah yes my bad, it's setting that to '2' indeed based on the 'struct
blk_desc' info.  This is correct

> > > +     total_gpt_entry_size = gpt_h->num_partition_entries *
[...]
> > > +                            gpt_h->sizeof_partition_entry;
> > > +     orig_gpt_e = calloc(1, total_gpt_entry_size + block_io->media->io_align);
> > > +     entry = (void *)ALIGN((uintptr_t)orig_gpt_e, block_io->media->io_align);
> > > +     if (!entry) {
> > > +             ret = EFI_OUT_OF_RESOURCES;
> > > +             goto out2;
> > > +     }
> > > +
> > > +     ret = block_io->read_blocks(block_io, block_io->media->media_id,
> > > +                                 gpt_h->partition_entry_lba,
> > > +                                 total_gpt_entry_size, entry);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out2;
> > > +
> >
> > Can't you use efi_bl_read() here, instead of searching for the protocol?
> > If you can, moving that out of a static function would make more sense.
> 
> With chat discussion with Ilias, we concluded not to do this modification.

Indeed efi_bl_read() cant be used here.  Just a heads up to the rest of the
people in this thread,  just replace the alignment calls with PTR_ALIGN()
and split the allocation/reading to a different fucntion.  This should make
this code a lot more readable.

Thanks!
/Ilias
> 
> Thanks,
> Masahisa Kojima
> 
> >
> > > +     /* count valid GPT entry */
> > > +     gpt_e = entry;
> > > +     for (i = 0; i < gpt_h->num_partition_entries; i++) {
> > > +             if (guidcmp(&null_guid, &gpt_e->partition_type_guid))
> > > +                     num_of_valid_entry++;
> > > +
> > > +             gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> > > +     }
> > > +
> > > +     /* prepare event data for measurement */
> > > +     event_size = sizeof(struct efi_gpt_data) +
> > > +             (num_of_valid_entry * gpt_h->sizeof_partition_entry);
> > > +     event = calloc(1, event_size);
> > > +     if (!event) {
> > > +             ret = EFI_OUT_OF_RESOURCES;
> > > +             goto out2;
> > > +     }
> > > +     memcpy(event, gpt_h, sizeof(gpt_header));
> > > +     put_unaligned_le64(num_of_valid_entry, &event->number_of_partitions);
> > > +
> > > +     /* copy valid GPT entry */
> > > +     gpt_e = entry;
> > > +     num_of_valid_entry = 0;
> > > +     for (i = 0; i < gpt_h->num_partition_entries; i++) {
> > > +             if (guidcmp(&null_guid, &gpt_e->partition_type_guid)) {
> > > +                     memcpy((u8 *)event->partitions +
> > > +                            (num_of_valid_entry * gpt_h->sizeof_partition_entry),
> > > +                            gpt_e, gpt_h->sizeof_partition_entry);
> > > +                     num_of_valid_entry++;
> > > +             }
> > > +
> > > +             gpt_e = (gpt_entry *)((u8 *)gpt_e + gpt_h->sizeof_partition_entry);
> > > +     }
> > > +
> > > +     ret = tcg2_measure_event(dev, 5, EV_EFI_GPT_EVENT, event_size, (u8 *)event);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out2;
> > > +
> > > +out2:
> > > +     EFI_CALL(efi_close_protocol((efi_handle_t)block_io, &efi_block_io_guid,
> > > +                                 NULL, NULL));
> > > +     free(orig_gpt_h);
> > > +     free(orig_gpt_e);
> > > +     free(event);
> > > +out1:
> > > +     efi_free_pool(device_path);
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  /**
> > >   * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation
> > >   *
> > >   * Return:   status code
> > >   */
> > > -efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> > > +efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *handle)
> > >  {
> > >       efi_status_t ret;
> > >       u32 pcr_index;
> > > @@ -1569,6 +1738,10 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> > >                       goto out;
> > >       }
> > >
> > > +     ret = tcg2_measure_gpt_data(dev, handle);
> > > +     if (ret != EFI_SUCCESS)
> > > +             goto out;
> > > +
> > >       tcg2_efi_app_invoked = true;
> > >  out:
> > >       return ret;
> > > --
> > > 2.17.1
> > >
> >
> > Cheers
> > /Ilias

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Masahisa Kojima]

On Thu, 23 Sept 2021 at 01:19, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Tue, 21 Sept 2021 at 01:17, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > TCG PC Client spec requires to measure the SMBIOS
> > table that contain static configuration information
> > (e.g. Platform Manufacturer Enterprise Number assigned by IANA,
> > platform model number, Vendor and Device IDs for each SMBIOS table).
> >
> > The device and environment dependent information such as
>
> device- and environment-dependent
>
> > serial number is cleared to zero or space character for
> > the measurement.
> >
> > Existing smbios_string() function returns pointer to the string
> > with const qualifier, but exisintg use case is updating version
> > string and const qualifier must be removed.
> > This commit removes const qualifier from smbios_string()
> > return value and reuses to clear the strings for the measurement.
> >
> > This commit also fixes the following compiler warning:
> >
> > lib/smbios-parser.c:59:39: warning: cast to pointer from integer of
> > different size [-Wint-to-pointer-cast]
> >   const struct smbios_header *header = (struct smbios_header *)entry->struct_table_address;
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >
> > Changes in v2:
> > - use flexible array for table_entry field
> > - modify funtion name to find_smbios_table()
> > - remove unnecessary const qualifier from smbios_string()
> > - create non-const version of next_header()
> >
> >  include/efi_loader.h          |   2 +
> >  include/efi_tcg2.h            |  15 ++++
> >  include/smbios.h              |  17 +++-
> >  lib/efi_loader/Kconfig        |   1 +
> >  lib/efi_loader/efi_boottime.c |   2 +
> >  lib/efi_loader/efi_smbios.c   |   2 -
> >  lib/efi_loader/efi_tcg2.c     |  84 +++++++++++++++++++
> >  lib/smbios-parser.c           | 152 +++++++++++++++++++++++++++++++---
> >  8 files changed, 261 insertions(+), 14 deletions(-)
>
> Where are the tests for this new code, please?
>
> Would it make sense to have a function that iterates through the data
> that does need to be hashed, instead?

I agree that it is straightforward if we iterates the data to be hashed.
But the data NOT to be hashed is less than the data to be hashed,
and this is what edk2 implements, we can easily compare with edk2.
So I would like to keep current implementation.

Thanks,
Masahisa Kojima

>
> Regards,
> Simon

Re: [PATCH v2 1/3] efi_loader: add SMBIOS table measurement

[Simon Glass]

Hi Ilias,

On Tue, 28 Sept 2021 at 11:41, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
>
> [...]
> > > > > We've mentioned this in the past.  The sandbox TPM is very limited wrt
> > > > > tpm testing for the EFI TCG protocol.
> > > >
> > > > So let's add some more features? If it helps, think of the sandbox TPM
> > > > as test code, not an emulator. It is a very simple kind of emulator to
> > > > allow tests to work.
> > >
> > > The amount of features needed to test EFI TCG are not minimal.  Since I'll
> > > upstream the mmio tpm anyway,  we'll just test TCG there.  If someone wants
> > > to go ahead and make the sandbox TPM a TIS compliant device that covers the
> > > requirements of the EFI TCG,  I am fine using it.
> >
> > Do you know how many features? There's 250 LOC in this patch.
>
> I haven't checked for a while but back when I tested it
> tpm2_get_capability() was failing on a number of cases.  The EFI TCG
> code expects:
> - TPM2_PT_MAX_COMMAND_SIZE
> - TPM2_PT_MAX_RESPONSE_SIZE
> - TPM2_PT_MANUFACTURER
> - TPM2_PT_PCR_COUNT
> - TPM2_CAP_PCRS
> when querying capabilities.  Ideally we'd also want to extend more
> than 1 PCRs and verify that worked correctly.

That doesn't seem like much. As you say I already added support for
multiple PCRs. I think we should do it and add a test.

>
> >
> > >
> > > >
> > > > > I did send TPM MMIO patches a while back [1].  This would allow us to
> > > > > test everything under QEMU,  but you asked for *another* device to be
> > > > > part of the API I posted (apart from the MMIO).  I've found some time
> > > >
> > > > Yes that is because if you just add a new protocol you have not made
> > > > anything better, just added one more way of doing things.
> > >
> > > Our perspective of 'better' seems to be different.
> > >
> > > I added a TIS API for any driver to use.  I actually did 2 iterations of
> > > the driver.  The first one was replicating all the code and you said 'why
> > > are we replicating code',  which was done already in a bunch of drivers
> > > already...
> > > Then I added an API and a driver using it but you wanted to convert more
> > > *existing* drivers to the API before merging it. But the fact is that if
> > > anyone wants to add a new driver he has to code  ~900 lines instead of the
> > > ~150 needed with the API in place,  not to mention the duplication of bugs
> > > all over the place....
> >
> > It would be like adding a new filesystem in U-Boot with its own new
> > framework for filesystems. It creates technical debt and we don't know
> > if anyone will actually use it.
> >
> > https://xkcd.com/927/
> >
> > I think your API is a great idea but we need some effort to migrate to
> > it, to avoid the problem above. After all, who else is going to do it?
>
> I ordered the SPI TPM, so hopefully, I'll be able to have the MMIO and
> SPI drivers using it!

Snap!

Regards,
Simon