[PATCH v3 0/3] Enhance Measured Boot

[PATCH v3 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             |   5 +-
 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_device_path.c |  27 +++
 lib/efi_loader/efi_smbios.c      |   2 -
 lib/efi_loader/efi_tcg2.c        | 297 ++++++++++++++++++++++++++++++-
 lib/smbios-parser.c              | 152 ++++++++++++++--
 10 files changed, 518 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH v3 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 v3:
- TCG spec says EV_SEPARATOR must be the last,
  swap the order of measurement

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 8f02d4fb0b..ca66695b39 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -210,6 +210,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 f48d9e8b51..e691b1ea96 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
@@ -1455,6 +1456,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
  *
@@ -1466,6 +1542,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;
@@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 
+	entry = (struct smbios_entry *)find_smbios_table();
+	if (entry) {
+		ret = tcg2_measure_smbios(dev, entry);
+		if (ret != EFI_SUCCESS)
+			goto out;
+	}
+
 	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
 		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
 					 sizeof(event), (u8 *)&event);
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

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

[Simon Glass]

Hi,

On Fri, 1 Oct 2021 at 05:19, 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
> 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 v3:
> - TCG spec says EV_SEPARATOR must be the last,
>   swap the order of measurement
>
> 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(-)

As part of this work, can you or someone else at Linaro please add
tests for the SMBIOS code?

Also as mentioned in the previous version, we should have tests for
this. Ilias suggested what needs to be added to the sandbox emulator.

Thanks,
Simon

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

[Ilias Apalodimas]

Hi Simon, 

[...]
> As part of this work, can you or someone else at Linaro please add
> tests for the SMBIOS code?
> 
> Also as mentioned in the previous version, we should have tests for
> this. Ilias suggested what needs to be added to the sandbox emulator.

I pointed out what's missing on the sandbox.  Asking for the tests is
fine and those will be added,  but since the TPM is already working in
QEMU,  I strongly prefer doing it there.

I know you like sandbox and you are trying to promote using it for testing, 
but imho having it for the TPM is not the best of examples.  You are adding 
code emulating a really complex device.  There's *always* going to be
missing functionality (not to mention bugs). 

The EFI TCG tests will ask more and more from the TPM.  E.g there's a call for 
changing the active PCRs which we haven't implemented yet in EFI.  We'll have
to keep adding  complex features to sandbox for every patch?
QEMU and it's TPM already work and have all the functionality we'll ever need.
In fact this [1] came out from testing on QEMU.   So since QEMU and testing
is an acceptable way for testing U-Boot,  I'll resend my TPM MMIO patchset
and we can create the tests there.

[1] https://source.denx.de/u-boot/custodians/u-boot-efi/-/commit/346cee3ac5782fefeaeda2b54914b029547adf52


Thanks
/Ilias

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

[Heinrich Schuchardt]

On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> TCG PC Client spec requires to measure the SMBIOS

I guess you mean the "TCG PC Client Platform Firmware Profile
Specification"? Please, provide the full name in the commit message.

> 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.

This contradicts the spec requiring the device IDs (i.e. the serial
number) to be measured. You don't want to get the same measurement when
the device is swapped.

>
> 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 v3:
> - TCG spec says EV_SEPARATOR must be the last,
>    swap the order of measurement
>
> 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 8f02d4fb0b..ca66695b39 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -210,6 +210,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);

This looks wrong. Your code is changing the actual SMBIOS table that we
will pass via an UEFI configuration table. Why would you do this? For
instance the serial number is an information that you actually want to
see in the operating system. I think you should first copy the string
and then manipulate the copy if this is necessary for the measurement.

Best regards

Heinrich

>
>   /**
>    * 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 f48d9e8b51..e691b1ea96 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> @@ -1455,6 +1456,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
>    *
> @@ -1466,6 +1542,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;
> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>
> +	entry = (struct smbios_entry *)find_smbios_table();
> +	if (entry) {
> +		ret = tcg2_measure_smbios(dev, entry);
> +		if (ret != EFI_SUCCESS)
> +			goto out;
> +	}
> +
>   	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>   		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
>   					 sizeof(event), (u8 *)&event);
> 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);
> +	}
> +}
>

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

[Masahisa Kojima]

On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> > TCG PC Client spec requires to measure the SMBIOS
>
> I guess you mean the "TCG PC Client Platform Firmware Profile
> Specification"? Please, provide the full name in the commit message.

Sorry, I will write full spec name.

>
> > 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.
>
> This contradicts the spec requiring the device IDs (i.e. the serial
> number) to be measured. You don't want to get the same measurement when
> the device is swapped.

In my understanding, the same measurement result is expected
among one product. Even if the device is swapped, measurement is the same.
If device unique information such as serial number
is included in the measurement, attestation process will be very complicated.

>
> >
> > 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 v3:
> > - TCG spec says EV_SEPARATOR must be the last,
> >    swap the order of measurement
> >
> > 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 8f02d4fb0b..ca66695b39 100644
> > --- a/include/efi_tcg2.h
> > +++ b/include/efi_tcg2.h
> > @@ -210,6 +210,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);
>
> This looks wrong. Your code is changing the actual SMBIOS table that we
> will pass via an UEFI configuration table. Why would you do this? For
> instance the serial number is an information that you actually want to
> see in the operating system. I think you should first copy the string
> and then manipulate the copy if this is necessary for the measurement.

No, I prepare the copied SMBIOS table and update it for the measurement.

tcg2_measure_smbios():
+       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);

I have tested that linux can show the original SMBIOS table
by "dmidocode" command.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> >   /**
> >    * 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 f48d9e8b51..e691b1ea96 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> > @@ -1455,6 +1456,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
> >    *
> > @@ -1466,6 +1542,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;
> > @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> >       if (ret != EFI_SUCCESS)
> >               goto out;
> >
> > +     entry = (struct smbios_entry *)find_smbios_table();
> > +     if (entry) {
> > +             ret = tcg2_measure_smbios(dev, entry);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto out;
> > +     }
> > +
> >       for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> >               ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> >                                        sizeof(event), (u8 *)&event);
> > 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);
> > +     }
> > +}
> >
>

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

[Heinrich Schuchardt]

On 10/21/21 10:38, Masahisa Kojima wrote:
> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
>>> TCG PC Client spec requires to measure the SMBIOS
>>
>> I guess you mean the "TCG PC Client Platform Firmware Profile
>> Specification"? Please, provide the full name in the commit message.
>
> Sorry, I will write full spec name.
>
>>
>>> 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.
>>
>> This contradicts the spec requiring the device IDs (i.e. the serial
>> number) to be measured. You don't want to get the same measurement when
>> the device is swapped.
>
> In my understanding, the same measurement result is expected
> among one product. Even if the device is swapped, measurement is the same.
> If device unique information such as serial number
> is included in the measurement, attestation process will be very complicated.
>
>>
>>>
>>> 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 v3:
>>> - TCG spec says EV_SEPARATOR must be the last,
>>>     swap the order of measurement
>>>
>>> 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 8f02d4fb0b..ca66695b39 100644
>>> --- a/include/efi_tcg2.h
>>> +++ b/include/efi_tcg2.h
>>> @@ -210,6 +210,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);
>>
>> This looks wrong. Your code is changing the actual SMBIOS table that we
>> will pass via an UEFI configuration table. Why would you do this? For
>> instance the serial number is an information that you actually want to
>> see in the operating system. I think you should first copy the string
>> and then manipulate the copy if this is necessary for the measurement.
>
> No, I prepare the copied SMBIOS table and update it for the measurement.

Then why do you move from const char * to char *?

You are passing in a const struct smbios_header *header. This implies
that every part of this structure is const. This is a contract with the
caller that every part of the table will be treated as const. You should
not return anything as not const.

Best regards

Heinrich

>
> tcg2_measure_smbios():
> +       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);
>
> I have tested that linux can show the original SMBIOS table
> by "dmidocode" command.
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>    /**
>>>     * 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 f48d9e8b51..e691b1ea96 100644
>>> --- a/lib/efi_loader/Kconfig
>>> +++ b/lib/efi_loader/Kconfig
>>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
>>> @@ -1455,6 +1456,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
>>>     *
>>> @@ -1466,6 +1542,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;
>>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
>>>        if (ret != EFI_SUCCESS)
>>>                goto out;
>>>
>>> +     entry = (struct smbios_entry *)find_smbios_table();
>>> +     if (entry) {
>>> +             ret = tcg2_measure_smbios(dev, entry);
>>> +             if (ret != EFI_SUCCESS)
>>> +                     goto out;
>>> +     }
>>> +
>>>        for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>>>                ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
>>>                                         sizeof(event), (u8 *)&event);
>>> 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);
>>> +     }
>>> +}
>>>
>>

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

[Masahisa Kojima]

On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 10/21/21 10:38, Masahisa Kojima wrote:
> > On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> >>> TCG PC Client spec requires to measure the SMBIOS
> >>
> >> I guess you mean the "TCG PC Client Platform Firmware Profile
> >> Specification"? Please, provide the full name in the commit message.
> >
> > Sorry, I will write full spec name.
> >
> >>
> >>> 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.
> >>
> >> This contradicts the spec requiring the device IDs (i.e. the serial
> >> number) to be measured. You don't want to get the same measurement when
> >> the device is swapped.
> >
> > In my understanding, the same measurement result is expected
> > among one product. Even if the device is swapped, measurement is the same.
> > If device unique information such as serial number
> > is included in the measurement, attestation process will be very complicated.
> >
> >>
> >>>
> >>> 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 v3:
> >>> - TCG spec says EV_SEPARATOR must be the last,
> >>>     swap the order of measurement
> >>>
> >>> 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 8f02d4fb0b..ca66695b39 100644
> >>> --- a/include/efi_tcg2.h
> >>> +++ b/include/efi_tcg2.h
> >>> @@ -210,6 +210,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);
> >>
> >> This looks wrong. Your code is changing the actual SMBIOS table that we
> >> will pass via an UEFI configuration table. Why would you do this? For
> >> instance the serial number is an information that you actually want to
> >> see in the operating system. I think you should first copy the string
> >> and then manipulate the copy if this is necessary for the measurement.
> >
> > No, I prepare the copied SMBIOS table and update it for the measurement.
>
> Then why do you move from const char * to char *?
>
> You are passing in a const struct smbios_header *header. This implies
> that every part of this structure is const. This is a contract with the
> caller that every part of the table will be treated as const. You should
> not return anything as not const.

struct smbios_header only has following members.

struct __packed smbios_header {
        u8 type;
        u8 length;
        u16 handle;
};

SMBIOS table is like following.

struct smbios_header;
struct type0_specific_info;
"index1 string\0"
"index2 string\0"
"\0\0"
struct smbios_header;
struct type1_specific_info;
"index1 string\0"
...

+ char *smbios_string(const struct smbios_header *header, int index)

smbios_string() function will not update input smbios_header, and return
the char pointer to the string specified by "index" argument, to
update the string
by caller.
So I think there is no problem.

Best Regards,
Masahisa Kojima


Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > tcg2_measure_smbios():
> > +       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);
> >
> > I have tested that linux can show the original SMBIOS table
> > by "dmidocode" command.
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>    /**
> >>>     * 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 f48d9e8b51..e691b1ea96 100644
> >>> --- a/lib/efi_loader/Kconfig
> >>> +++ b/lib/efi_loader/Kconfig
> >>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> >>> @@ -1455,6 +1456,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
> >>>     *
> >>> @@ -1466,6 +1542,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;
> >>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> >>>        if (ret != EFI_SUCCESS)
> >>>                goto out;
> >>>
> >>> +     entry = (struct smbios_entry *)find_smbios_table();
> >>> +     if (entry) {
> >>> +             ret = tcg2_measure_smbios(dev, entry);
> >>> +             if (ret != EFI_SUCCESS)
> >>> +                     goto out;
> >>> +     }
> >>> +
> >>>        for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> >>>                ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> >>>                                         sizeof(event), (u8 *)&event);
> >>> 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);
> >>> +     }
> >>> +}
> >>>
> >>

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

[Heinrich Schuchardt]

On 10/21/21 14:52, Masahisa Kojima wrote:
> On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> On 10/21/21 10:38, Masahisa Kojima wrote:
>>> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
>>>>> TCG PC Client spec requires to measure the SMBIOS
>>>>
>>>> I guess you mean the "TCG PC Client Platform Firmware Profile
>>>> Specification"? Please, provide the full name in the commit message.
>>>
>>> Sorry, I will write full spec name.
>>>
>>>>
>>>>> 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.
>>>>
>>>> This contradicts the spec requiring the device IDs (i.e. the serial
>>>> number) to be measured. You don't want to get the same measurement when
>>>> the device is swapped.
>>>
>>> In my understanding, the same measurement result is expected
>>> among one product. Even if the device is swapped, measurement is the same.
>>> If device unique information such as serial number
>>> is included in the measurement, attestation process will be very complicated.
>>>
>>>>
>>>>>
>>>>> 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 v3:
>>>>> - TCG spec says EV_SEPARATOR must be the last,
>>>>>      swap the order of measurement
>>>>>
>>>>> 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 8f02d4fb0b..ca66695b39 100644
>>>>> --- a/include/efi_tcg2.h
>>>>> +++ b/include/efi_tcg2.h
>>>>> @@ -210,6 +210,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);
>>>>
>>>> This looks wrong. Your code is changing the actual SMBIOS table that we
>>>> will pass via an UEFI configuration table. Why would you do this? For
>>>> instance the serial number is an information that you actually want to
>>>> see in the operating system. I think you should first copy the string
>>>> and then manipulate the copy if this is necessary for the measurement.
>>>
>>> No, I prepare the copied SMBIOS table and update it for the measurement.
>>
>> Then why do you move from const char * to char *?
>>
>> You are passing in a const struct smbios_header *header. This implies
>> that every part of this structure is const. This is a contract with the
>> caller that every part of the table will be treated as const. You should
>> not return anything as not const.
>
> struct smbios_header only has following members.
>
> struct __packed smbios_header {
>          u8 type;
>          u8 length;
>          u16 handle;
> };
>
> SMBIOS table is like following.
>
> struct smbios_header;
> struct type0_specific_info;
> "index1 string\0"
> "index2 string\0"
> "\0\0"
> struct smbios_header;
> struct type1_specific_info;
> "index1 string\0"
> ...
>
> + char *smbios_string(const struct smbios_header *header, int index)
>
> smbios_string() function will not update input smbios_header, and return
> the char pointer to the string specified by "index" argument, to
> update the string
> by caller.
> So I think there is no problem.

I would prefer if you leave the signature as is.

If header points to a copy that your code has created, you can cast the
return value to (char *). That way the function does what is expected
and the deviation will be restricted to your specific usage.

Best regards

Heinrich

>
> Best Regards,
> Masahisa Kojima
>
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> tcg2_measure_smbios():
>>> +       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);
>>>
>>> I have tested that linux can show the original SMBIOS table
>>> by "dmidocode" command.
>>>
>>> Thanks,
>>> Masahisa Kojima
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>>     /**
>>>>>      * 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 f48d9e8b51..e691b1ea96 100644
>>>>> --- a/lib/efi_loader/Kconfig
>>>>> +++ b/lib/efi_loader/Kconfig
>>>>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
>>>>> @@ -1455,6 +1456,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
>>>>>      *
>>>>> @@ -1466,6 +1542,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;
>>>>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
>>>>>         if (ret != EFI_SUCCESS)
>>>>>                 goto out;
>>>>>
>>>>> +     entry = (struct smbios_entry *)find_smbios_table();
>>>>> +     if (entry) {
>>>>> +             ret = tcg2_measure_smbios(dev, entry);
>>>>> +             if (ret != EFI_SUCCESS)
>>>>> +                     goto out;
>>>>> +     }
>>>>> +
>>>>>         for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>>>>>                 ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
>>>>>                                          sizeof(event), (u8 *)&event);
>>>>> 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);
>>>>> +     }
>>>>> +}
>>>>>
>>>>

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

[Masahisa Kojima]

 is

On Thu, 21 Oct 2021 at 22:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 10/21/21 14:52, Masahisa Kojima wrote:
> > On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >>
> >>
> >> On 10/21/21 10:38, Masahisa Kojima wrote:
> >>> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> >>>>> TCG PC Client spec requires to measure the SMBIOS
> >>>>
> >>>> I guess you mean the "TCG PC Client Platform Firmware Profile
> >>>> Specification"? Please, provide the full name in the commit message.
> >>>
> >>> Sorry, I will write full spec name.
> >>>
> >>>>
> >>>>> 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.
> >>>>
> >>>> This contradicts the spec requiring the device IDs (i.e. the serial
> >>>> number) to be measured. You don't want to get the same measurement when
> >>>> the device is swapped.
> >>>
> >>> In my understanding, the same measurement result is expected
> >>> among one product. Even if the device is swapped, measurement is the same.
> >>> If device unique information such as serial number
> >>> is included in the measurement, attestation process will be very complicated.
> >>>
> >>>>
> >>>>>
> >>>>> 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 v3:
> >>>>> - TCG spec says EV_SEPARATOR must be the last,
> >>>>>      swap the order of measurement
> >>>>>
> >>>>> 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 8f02d4fb0b..ca66695b39 100644
> >>>>> --- a/include/efi_tcg2.h
> >>>>> +++ b/include/efi_tcg2.h
> >>>>> @@ -210,6 +210,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);
> >>>>
> >>>> This looks wrong. Your code is changing the actual SMBIOS table that we
> >>>> will pass via an UEFI configuration table. Why would you do this? For
> >>>> instance the serial number is an information that you actually want to
> >>>> see in the operating system. I think you should first copy the string
> >>>> and then manipulate the copy if this is necessary for the measurement.
> >>>
> >>> No, I prepare the copied SMBIOS table and update it for the measurement.
> >>
> >> Then why do you move from const char * to char *?
> >>
> >> You are passing in a const struct smbios_header *header. This implies
> >> that every part of this structure is const. This is a contract with the
> >> caller that every part of the table will be treated as const. You should
> >> not return anything as not const.
> >
> > struct smbios_header only has following members.
> >
> > struct __packed smbios_header {
> >          u8 type;
> >          u8 length;
> >          u16 handle;
> > };
> >
> > SMBIOS table is like following.
> >
> > struct smbios_header;
> > struct type0_specific_info;
> > "index1 string\0"
> > "index2 string\0"
> > "\0\0"
> > struct smbios_header;
> > struct type1_specific_info;
> > "index1 string\0"
> > ...
> >
> > + char *smbios_string(const struct smbios_header *header, int index)
> >
> > smbios_string() function will not update input smbios_header, and return
> > the char pointer to the string specified by "index" argument, to
> > update the string
> > by caller.
> > So I think there is no problem.
>
> I would prefer if you leave the signature as is.
>
> If header points to a copy that your code has created, you can cast the
> return value to (char *). That way the function does what is expected
> and the deviation will be restricted to your specific usage.

There is one existing use case of smbios_string() in
smbios-parser.c::smbios_update_version_full().
smbios_update_version_full() uses smbios_string() to get the
version string pointer to update the version string.
So I think smbios_string() is designed to return the non-const char*.

I also think removing const by type cast is unsafe.

Best Regards,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Best Regards,
> > Masahisa Kojima
> >
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> tcg2_measure_smbios():
> >>> +       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);
> >>>
> >>> I have tested that linux can show the original SMBIOS table
> >>> by "dmidocode" command.
> >>>
> >>> Thanks,
> >>> Masahisa Kojima
> >>>
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>>     /**
> >>>>>      * 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 f48d9e8b51..e691b1ea96 100644
> >>>>> --- a/lib/efi_loader/Kconfig
> >>>>> +++ b/lib/efi_loader/Kconfig
> >>>>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> >>>>> @@ -1455,6 +1456,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
> >>>>>      *
> >>>>> @@ -1466,6 +1542,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;
> >>>>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> >>>>>         if (ret != EFI_SUCCESS)
> >>>>>                 goto out;
> >>>>>
> >>>>> +     entry = (struct smbios_entry *)find_smbios_table();
> >>>>> +     if (entry) {
> >>>>> +             ret = tcg2_measure_smbios(dev, entry);
> >>>>> +             if (ret != EFI_SUCCESS)
> >>>>> +                     goto out;
> >>>>> +     }
> >>>>> +
> >>>>>         for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> >>>>>                 ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> >>>>>                                          sizeof(event), (u8 *)&event);
> >>>>> 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);
> >>>>> +     }
> >>>>> +}
> >>>>>
> >>>>

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

[Simon Glass]

Hi Masahisa,

On Thu, 21 Oct 2021 at 07:41, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
>  is
>
> On Thu, 21 Oct 2021 at 22:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 10/21/21 14:52, Masahisa Kojima wrote:
> > > On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >>
> > >>
> > >> On 10/21/21 10:38, Masahisa Kojima wrote:
> > >>> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>>>
> > >>>> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> > >>>>> TCG PC Client spec requires to measure the SMBIOS
> > >>>>
> > >>>> I guess you mean the "TCG PC Client Platform Firmware Profile
> > >>>> Specification"? Please, provide the full name in the commit message.
> > >>>
> > >>> Sorry, I will write full spec name.
> > >>>
> > >>>>
> > >>>>> 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.
> > >>>>
> > >>>> This contradicts the spec requiring the device IDs (i.e. the serial
> > >>>> number) to be measured. You don't want to get the same measurement when
> > >>>> the device is swapped.
> > >>>
> > >>> In my understanding, the same measurement result is expected
> > >>> among one product. Even if the device is swapped, measurement is the same.
> > >>> If device unique information such as serial number
> > >>> is included in the measurement, attestation process will be very complicated.
> > >>>
> > >>>>
> > >>>>>
> > >>>>> 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 v3:
> > >>>>> - TCG spec says EV_SEPARATOR must be the last,
> > >>>>>      swap the order of measurement
> > >>>>>
> > >>>>> 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 8f02d4fb0b..ca66695b39 100644
> > >>>>> --- a/include/efi_tcg2.h
> > >>>>> +++ b/include/efi_tcg2.h
> > >>>>> @@ -210,6 +210,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);
> > >>>>
> > >>>> This looks wrong. Your code is changing the actual SMBIOS table that we
> > >>>> will pass via an UEFI configuration table. Why would you do this? For
> > >>>> instance the serial number is an information that you actually want to
> > >>>> see in the operating system. I think you should first copy the string
> > >>>> and then manipulate the copy if this is necessary for the measurement.
> > >>>
> > >>> No, I prepare the copied SMBIOS table and update it for the measurement.
> > >>
> > >> Then why do you move from const char * to char *?
> > >>
> > >> You are passing in a const struct smbios_header *header. This implies
> > >> that every part of this structure is const. This is a contract with the
> > >> caller that every part of the table will be treated as const. You should
> > >> not return anything as not const.
> > >
> > > struct smbios_header only has following members.
> > >
> > > struct __packed smbios_header {
> > >          u8 type;
> > >          u8 length;
> > >          u16 handle;
> > > };
> > >
> > > SMBIOS table is like following.
> > >
> > > struct smbios_header;
> > > struct type0_specific_info;
> > > "index1 string\0"
> > > "index2 string\0"
> > > "\0\0"
> > > struct smbios_header;
> > > struct type1_specific_info;
> > > "index1 string\0"
> > > ...
> > >
> > > + char *smbios_string(const struct smbios_header *header, int index)
> > >
> > > smbios_string() function will not update input smbios_header, and return
> > > the char pointer to the string specified by "index" argument, to
> > > update the string
> > > by caller.
> > > So I think there is no problem.
> >
> > I would prefer if you leave the signature as is.
> >
> > If header points to a copy that your code has created, you can cast the
> > return value to (char *). That way the function does what is expected
> > and the deviation will be restricted to your specific usage.
>
> There is one existing use case of smbios_string() in
> smbios-parser.c::smbios_update_version_full().
> smbios_update_version_full() uses smbios_string() to get the
> version string pointer to update the version string.
> So I think smbios_string() is designed to return the non-const char*.

Not really, but as you say there is a cast when updating the version.
Perhaps we should have a non-const version of the function?


- Simon


> I also think removing const by type cast is unsafe.
>
> Best Regards,
> Masahisa Kojima
>
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > > Best Regards,
> > > Masahisa Kojima
> > >
> > >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>>
> > >>> tcg2_measure_smbios():
> > >>> +       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);
> > >>>
> > >>> I have tested that linux can show the original SMBIOS table
> > >>> by "dmidocode" command.
> > >>>
> > >>> Thanks,
> > >>> Masahisa Kojima
> > >>>
> > >>>>
> > >>>> Best regards
> > >>>>
> > >>>> Heinrich
> > >>>>
> > >>>>>
> > >>>>>     /**
> > >>>>>      * 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 f48d9e8b51..e691b1ea96 100644
> > >>>>> --- a/lib/efi_loader/Kconfig
> > >>>>> +++ b/lib/efi_loader/Kconfig
> > >>>>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> > >>>>> @@ -1455,6 +1456,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
> > >>>>>      *
> > >>>>> @@ -1466,6 +1542,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;
> > >>>>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> > >>>>>         if (ret != EFI_SUCCESS)
> > >>>>>                 goto out;
> > >>>>>
> > >>>>> +     entry = (struct smbios_entry *)find_smbios_table();
> > >>>>> +     if (entry) {
> > >>>>> +             ret = tcg2_measure_smbios(dev, entry);
> > >>>>> +             if (ret != EFI_SUCCESS)
> > >>>>> +                     goto out;
> > >>>>> +     }
> > >>>>> +
> > >>>>>         for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > >>>>>                 ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > >>>>>                                          sizeof(event), (u8 *)&event);
> > >>>>> 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);
> > >>>>> +     }
> > >>>>> +}
> > >>>>>
> > >>>>

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

[Masahisa Kojima]

Hi Simon,

On Tue, 2 Nov 2021 at 23:56, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Thu, 21 Oct 2021 at 07:41, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> >  is
> >
> > On Thu, 21 Oct 2021 at 22:04, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > On 10/21/21 14:52, Masahisa Kojima wrote:
> > > > On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >>
> > > >>
> > > >> On 10/21/21 10:38, Masahisa Kojima wrote:
> > > >>> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>>>
> > > >>>> On 10/1/21 1:18 PM, Masahisa Kojima wrote:
> > > >>>>> TCG PC Client spec requires to measure the SMBIOS
> > > >>>>
> > > >>>> I guess you mean the "TCG PC Client Platform Firmware Profile
> > > >>>> Specification"? Please, provide the full name in the commit message.
> > > >>>
> > > >>> Sorry, I will write full spec name.
> > > >>>
> > > >>>>
> > > >>>>> 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.
> > > >>>>
> > > >>>> This contradicts the spec requiring the device IDs (i.e. the serial
> > > >>>> number) to be measured. You don't want to get the same measurement when
> > > >>>> the device is swapped.
> > > >>>
> > > >>> In my understanding, the same measurement result is expected
> > > >>> among one product. Even if the device is swapped, measurement is the same.
> > > >>> If device unique information such as serial number
> > > >>> is included in the measurement, attestation process will be very complicated.
> > > >>>
> > > >>>>
> > > >>>>>
> > > >>>>> 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 v3:
> > > >>>>> - TCG spec says EV_SEPARATOR must be the last,
> > > >>>>>      swap the order of measurement
> > > >>>>>
> > > >>>>> 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 8f02d4fb0b..ca66695b39 100644
> > > >>>>> --- a/include/efi_tcg2.h
> > > >>>>> +++ b/include/efi_tcg2.h
> > > >>>>> @@ -210,6 +210,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);
> > > >>>>
> > > >>>> This looks wrong. Your code is changing the actual SMBIOS table that we
> > > >>>> will pass via an UEFI configuration table. Why would you do this? For
> > > >>>> instance the serial number is an information that you actually want to
> > > >>>> see in the operating system. I think you should first copy the string
> > > >>>> and then manipulate the copy if this is necessary for the measurement.
> > > >>>
> > > >>> No, I prepare the copied SMBIOS table and update it for the measurement.
> > > >>
> > > >> Then why do you move from const char * to char *?
> > > >>
> > > >> You are passing in a const struct smbios_header *header. This implies
> > > >> that every part of this structure is const. This is a contract with the
> > > >> caller that every part of the table will be treated as const. You should
> > > >> not return anything as not const.
> > > >
> > > > struct smbios_header only has following members.
> > > >
> > > > struct __packed smbios_header {
> > > >          u8 type;
> > > >          u8 length;
> > > >          u16 handle;
> > > > };
> > > >
> > > > SMBIOS table is like following.
> > > >
> > > > struct smbios_header;
> > > > struct type0_specific_info;
> > > > "index1 string\0"
> > > > "index2 string\0"
> > > > "\0\0"
> > > > struct smbios_header;
> > > > struct type1_specific_info;
> > > > "index1 string\0"
> > > > ...
> > > >
> > > > + char *smbios_string(const struct smbios_header *header, int index)
> > > >
> > > > smbios_string() function will not update input smbios_header, and return
> > > > the char pointer to the string specified by "index" argument, to
> > > > update the string
> > > > by caller.
> > > > So I think there is no problem.
> > >
> > > I would prefer if you leave the signature as is.
> > >
> > > If header points to a copy that your code has created, you can cast the
> > > return value to (char *). That way the function does what is expected
> > > and the deviation will be restricted to your specific usage.
> >
> > There is one existing use case of smbios_string() in
> > smbios-parser.c::smbios_update_version_full().
> > smbios_update_version_full() uses smbios_string() to get the
> > version string pointer to update the version string.
> > So I think smbios_string() is designed to return the non-const char*.
>
> Not really, but as you say there is a cast when updating the version.
> Perhaps we should have a non-const version of the function?

I'm not sure I correctly understand your point, but I modified the
existing const version of the smbios_string() to non-const version
in this patch.
 # I don't come up with any use cases of const version of the function
    aside from dumping smbios string for the debug purpose.

Thanks,
Masahisa Kojima

>
>
> - Simon
>
>
> > I also think removing const by type cast is unsafe.
> >
> > Best Regards,
> > Masahisa Kojima
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > > >
> > > > Best Regards,
> > > > Masahisa Kojima
> > > >
> > > >
> > > > Thanks,
> > > > Masahisa Kojima
> > > >
> > > >>
> > > >> Best regards
> > > >>
> > > >> Heinrich
> > > >>
> > > >>>
> > > >>> tcg2_measure_smbios():
> > > >>> +       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);
> > > >>>
> > > >>> I have tested that linux can show the original SMBIOS table
> > > >>> by "dmidocode" command.
> > > >>>
> > > >>> Thanks,
> > > >>> Masahisa Kojima
> > > >>>
> > > >>>>
> > > >>>> Best regards
> > > >>>>
> > > >>>> Heinrich
> > > >>>>
> > > >>>>>
> > > >>>>>     /**
> > > >>>>>      * 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 f48d9e8b51..e691b1ea96 100644
> > > >>>>> --- a/lib/efi_loader/Kconfig
> > > >>>>> +++ b/lib/efi_loader/Kconfig
> > > >>>>> @@ -320,6 +320,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 d3b8f93f14..f14d4d6da1 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>
> > > >>>>> @@ -1455,6 +1456,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
> > > >>>>>      *
> > > >>>>> @@ -1466,6 +1542,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;
> > > >>>>> @@ -1484,6 +1561,13 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void)
> > > >>>>>         if (ret != EFI_SUCCESS)
> > > >>>>>                 goto out;
> > > >>>>>
> > > >>>>> +     entry = (struct smbios_entry *)find_smbios_table();
> > > >>>>> +     if (entry) {
> > > >>>>> +             ret = tcg2_measure_smbios(dev, entry);
> > > >>>>> +             if (ret != EFI_SUCCESS)
> > > >>>>> +                     goto out;
> > > >>>>> +     }
> > > >>>>> +
> > > >>>>>         for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> > > >>>>>                 ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> > > >>>>>                                          sizeof(event), (u8 *)&event);
> > > >>>>> 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);
> > > >>>>> +     }
> > > >>>>> +}
> > > >>>>>
> > > >>>>

[PATCH v3 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>
---

Changes in v3:
- EV_EFI_GPT_EVENT is measured before EV_SEPARATOR, same as
  other PCRs
- use PTR_ARRAY instead of ARRAY
- create sub-function of allocating io_aligned buffer
- move search_gpt_dp_node() into efi_device_path.c

 include/blk.h                    |   3 +
 include/efi_loader.h             |   3 +-
 include/efi_tcg2.h               |  12 +++
 lib/efi_loader/efi_boottime.c    |   2 +-
 lib/efi_loader/efi_device_path.c |  27 +++++
 lib/efi_loader/efi_tcg2.c        | 163 ++++++++++++++++++++++++++++++-
 6 files changed, 207 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..c557e8bee6 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 */
@@ -845,6 +845,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
 				       efi_uintn_t *size, efi_guid_t guid);
 struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
 				      const struct efi_device_path *dp2);
+struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
 efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
 					 efi_uintn_t *size);
 unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
index ca66695b39..50a59f9263 100644
--- a/include/efi_tcg2.h
+++ b/include/efi_tcg2.h
@@ -225,6 +225,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_device_path.c b/lib/efi_loader/efi_device_path.c
index cbdb466da4..6aec64f373 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -1294,3 +1294,30 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
 
 	return NULL;
 }
+
+/**
+ * search_gpt_dp_node() - search gpt device path node
+ *
+ * @device_path:	device path
+ *
+ * Return:	pointer to the gpt device path node
+ */
+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;
+}
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index f14d4d6da1..28e0362bf2 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1531,12 +1531,169 @@ static void *find_smbios_table(void)
 	return NULL;
 }
 
+static void *allocate_io_aligned_buf(void **buf, u32 size, u32 io_align)
+{
+	if (!buf)
+		return NULL;
+
+	*buf = calloc(1, size);
+	if (!*buf)
+		return NULL;
+
+	return (void *)PTR_ALIGN((uintptr_t)*buf, io_align);
+}
+
+/**
+ * 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;
+	void *orig_gpt_h = NULL;
+	void *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;
+
+	gpt_h = allocate_io_aligned_buf(&orig_gpt_h,
+					block_io->media->block_size +
+					block_io->media->io_align,
+					block_io->media->io_align);
+	if (!orig_gpt_h || !gpt_h) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out2;
+	}
+
+	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;
+	entry = allocate_io_aligned_buf(&orig_gpt_e,
+					total_gpt_entry_size +
+					block_io->media->io_align,
+					block_io->media->io_align);
+	if (!orig_gpt_e || !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;
@@ -1568,6 +1725,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;
+
 	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
 		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
 					 sizeof(event), (u8 *)&event);
-- 
2.17.1

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

[Simon Glass]

Hi Masahisa,

On Fri, 1 Oct 2021 at 05:19, Masahisa Kojima <masahisa.kojima@linaro.org> 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>
> ---
>
> Changes in v3:
> - EV_EFI_GPT_EVENT is measured before EV_SEPARATOR, same as
>   other PCRs
> - use PTR_ARRAY instead of ARRAY
> - create sub-function of allocating io_aligned buffer
> - move search_gpt_dp_node() into efi_device_path.c
>
>  include/blk.h                    |   3 +
>  include/efi_loader.h             |   3 +-
>  include/efi_tcg2.h               |  12 +++
>  lib/efi_loader/efi_boottime.c    |   2 +-
>  lib/efi_loader/efi_device_path.c |  27 +++++
>  lib/efi_loader/efi_tcg2.c        | 163 ++++++++++++++++++++++++++++++-
>  6 files changed, 207 insertions(+), 3 deletions(-)

Please can you go back through your patches and add tests for each one?

Regards,
Simon

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

[Masahisa Kojima]

Hi Simon,

On Sat, 2 Oct 2021 at 00:23, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Fri, 1 Oct 2021 at 05:19, Masahisa Kojima <masahisa.kojima@linaro.org> 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>
> > ---
> >
> > Changes in v3:
> > - EV_EFI_GPT_EVENT is measured before EV_SEPARATOR, same as
> >   other PCRs
> > - use PTR_ARRAY instead of ARRAY
> > - create sub-function of allocating io_aligned buffer
> > - move search_gpt_dp_node() into efi_device_path.c
> >
> >  include/blk.h                    |   3 +
> >  include/efi_loader.h             |   3 +-
> >  include/efi_tcg2.h               |  12 +++
> >  lib/efi_loader/efi_boottime.c    |   2 +-
> >  lib/efi_loader/efi_device_path.c |  27 +++++
> >  lib/efi_loader/efi_tcg2.c        | 163 ++++++++++++++++++++++++++++++-
> >  6 files changed, 207 insertions(+), 3 deletions(-)
>
> Please can you go back through your patches and add tests for each one?

I will send the selftest patch for the efi_tcg2.c.

Thanks,
Masahisa Kojima

>
> Regards,
> Simon

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

[Heinrich Schuchardt]

On 10/1/21 13:18, 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>
> ---
>
> Changes in v3:
> - EV_EFI_GPT_EVENT is measured before EV_SEPARATOR, same as
>    other PCRs
> - use PTR_ARRAY instead of ARRAY
> - create sub-function of allocating io_aligned buffer
> - move search_gpt_dp_node() into efi_device_path.c
>
>   include/blk.h                    |   3 +
>   include/efi_loader.h             |   3 +-
>   include/efi_tcg2.h               |  12 +++
>   lib/efi_loader/efi_boottime.c    |   2 +-
>   lib/efi_loader/efi_device_path.c |  27 +++++
>   lib/efi_loader/efi_tcg2.c        | 163 ++++++++++++++++++++++++++++++-
>   6 files changed, 207 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..c557e8bee6 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 */
> @@ -845,6 +845,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
>   				       efi_uintn_t *size, efi_guid_t guid);
>   struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
>   				      const struct efi_device_path *dp2);
> +struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
>   efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
>   					 efi_uintn_t *size);
>   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> index ca66695b39..50a59f9263 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -225,6 +225,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_device_path.c b/lib/efi_loader/efi_device_path.c
> index cbdb466da4..6aec64f373 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -1294,3 +1294,30 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
>
>   	return NULL;
>   }
> +
> +/**
> + * search_gpt_dp_node() - search gpt device path node
> + *
> + * @device_path:	device path
> + *
> + * Return:	pointer to the gpt device path node
> + */
> +struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)

You are not adding the function to an include. So it should be static.

> +{
> +	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;
> +}
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index f14d4d6da1..28e0362bf2 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1531,12 +1531,169 @@ static void *find_smbios_table(void)
>   	return NULL;
>   }
>
> +static void *allocate_io_aligned_buf(void **buf, u32 size, u32 io_align)
> +{
Using memalign() avoids having two pointers and obsoletes this function.

> +	if (!buf)
> +		return NULL;
> +
> +	*buf = calloc(1, size);

To create an aligned buffer with size bytes you would have to use size +
io_align - 1.

Best regards

Heinrich

> +	if (!*buf)
> +		return NULL;
> +
> +	return (void *)PTR_ALIGN((uintptr_t)*buf, io_align);
> +}
> +
> +/**
> + * 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;
> +	void *orig_gpt_h = NULL;
> +	void *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;
> +
> +	gpt_h = allocate_io_aligned_buf(&orig_gpt_h,
> +					block_io->media->block_size +
> +					block_io->media->io_align,
> +					block_io->media->io_align);
> +	if (!orig_gpt_h || !gpt_h) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out2;
> +	}
> +
> +	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;
> +	entry = allocate_io_aligned_buf(&orig_gpt_e,
> +					total_gpt_entry_size +
> +					block_io->media->io_align,
> +					block_io->media->io_align);
> +	if (!orig_gpt_e || !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;
> @@ -1568,6 +1725,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;
> +
>   	for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
>   		ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
>   					 sizeof(event), (u8 *)&event);
>

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

[Masahisa Kojima]

On Sat, 2 Oct 2021 at 01:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 10/1/21 13:18, 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>
> > ---
> >
> > Changes in v3:
> > - EV_EFI_GPT_EVENT is measured before EV_SEPARATOR, same as
> >    other PCRs
> > - use PTR_ARRAY instead of ARRAY
> > - create sub-function of allocating io_aligned buffer
> > - move search_gpt_dp_node() into efi_device_path.c
> >
> >   include/blk.h                    |   3 +
> >   include/efi_loader.h             |   3 +-
> >   include/efi_tcg2.h               |  12 +++
> >   lib/efi_loader/efi_boottime.c    |   2 +-
> >   lib/efi_loader/efi_device_path.c |  27 +++++
> >   lib/efi_loader/efi_tcg2.c        | 163 ++++++++++++++++++++++++++++++-
> >   6 files changed, 207 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..c557e8bee6 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 */
> > @@ -845,6 +845,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
> >                                      efi_uintn_t *size, efi_guid_t guid);
> >   struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
> >                                     const struct efi_device_path *dp2);
> > +struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
> >   efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
> >                                        efi_uintn_t *size);
> >   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > index ca66695b39..50a59f9263 100644
> > --- a/include/efi_tcg2.h
> > +++ b/include/efi_tcg2.h
> > @@ -225,6 +225,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_device_path.c b/lib/efi_loader/efi_device_path.c
> > index cbdb466da4..6aec64f373 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -1294,3 +1294,30 @@ efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
> >
> >       return NULL;
> >   }
> > +
> > +/**
> > + * search_gpt_dp_node() - search gpt device path node
> > + *
> > + * @device_path:     device path
> > + *
> > + * Return:   pointer to the gpt device path node
> > + */
> > +struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
>
> You are not adding the function to an include. So it should be static.

I added the function in efi_loader.h
---
diff --git a/include/efi_loader.h b/include/efi_loader.h
+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;
> > +}
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index f14d4d6da1..28e0362bf2 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -1531,12 +1531,169 @@ static void *find_smbios_table(void)
> >       return NULL;
> >   }
> >
> > +static void *allocate_io_aligned_buf(void **buf, u32 size, u32 io_align)
> > +{
> Using memalign() avoids having two pointers and obsoletes this function.

Thank you, I will use memalign().

Regards,
Masahisa Kojima

>
> > +     if (!buf)
> > +             return NULL;
> > +
> > +     *buf = calloc(1, size);
>
> To create an aligned buffer with size bytes you would have to use size +
> io_align - 1.
>
> Best regards
>
> Heinrich
>
> > +     if (!*buf)
> > +             return NULL;
> > +
> > +     return (void *)PTR_ALIGN((uintptr_t)*buf, io_align);
> > +}
> > +
> > +/**
> > + * 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;
> > +     void *orig_gpt_h = NULL;
> > +     void *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;
> > +
> > +     gpt_h = allocate_io_aligned_buf(&orig_gpt_h,
> > +                                     block_io->media->block_size +
> > +                                     block_io->media->io_align,
> > +                                     block_io->media->io_align);
> > +     if (!orig_gpt_h || !gpt_h) {
> > +             ret = EFI_OUT_OF_RESOURCES;
> > +             goto out2;
> > +     }
> > +
> > +     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;
> > +     entry = allocate_io_aligned_buf(&orig_gpt_e,
> > +                                     total_gpt_entry_size +
> > +                                     block_io->media->io_align,
> > +                                     block_io->media->io_align);
> > +     if (!orig_gpt_e || !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;
> > @@ -1568,6 +1725,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;
> > +
> >       for (pcr_index = 0; pcr_index <= 7; pcr_index++) {
> >               ret = tcg2_measure_event(dev, pcr_index, EV_SEPARATOR,
> >                                        sizeof(event), (u8 *)&event);
> >

[PATCH v3 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>
---

Changes in v3:
- read variable first, then mesure the variable

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

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 28e0362bf2..7fba4bc458 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>
@@ -1822,6 +1823,53 @@ 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;
+
+	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;
+
+	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);
+	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
  *
@@ -1885,6 +1933,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 v3 3/3] efi_loader: add DeployedMode and AuditMode variable measurement

[Heinrich Schuchardt]

On 10/1/21 13:18, Masahisa Kojima 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>
> ---
>
> Changes in v3:
> - read variable first, then mesure the variable
>
>   lib/efi_loader/efi_tcg2.c | 50 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 50 insertions(+)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 28e0362bf2..7fba4bc458 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>
> @@ -1822,6 +1823,53 @@ 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;
> +
> +	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;
> +
> +	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);
> +	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
>    *
> @@ -1885,6 +1933,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
>   		free(data);
>   	}
>
> +	ret = tcg2_measure_deployed_audit_mode(dev);

You do the same thing four times. A loop is preferable.

Best regards

Heinrich

> +
>   error:
>   	return ret;
>   }
>

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

[Masahisa Kojima]

On Sat, 2 Oct 2021 at 01:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 10/1/21 13:18, Masahisa Kojima 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>
> > ---
> >
> > Changes in v3:
> > - read variable first, then mesure the variable
> >
> >   lib/efi_loader/efi_tcg2.c | 50 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 50 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 28e0362bf2..7fba4bc458 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>
> > @@ -1822,6 +1823,53 @@ 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;
> > +
> > +     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;
> > +
> > +     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);
> > +     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
> >    *
> > @@ -1885,6 +1933,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> >               free(data);
> >       }
> >
> > +     ret = tcg2_measure_deployed_audit_mode(dev);
>
> You do the same thing four times. A loop is preferable.

After your series(efi_loader: centralize known vendor GUIDs) is merged,
I will update this patch to use loop for the secure variables.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> > +
> >   error:
> >       return ret;
> >   }
> >

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

[Masahisa Kojima]

Hi Heinrich,

On Mon, 4 Oct 2021 at 11:30, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> On Sat, 2 Oct 2021 at 01:43, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > On 10/1/21 13:18, Masahisa Kojima 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>
> > > ---
> > >
> > > Changes in v3:
> > > - read variable first, then mesure the variable
> > >
> > >   lib/efi_loader/efi_tcg2.c | 50 +++++++++++++++++++++++++++++++++++++++
> > >   1 file changed, 50 insertions(+)
> > >
> > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > > index 28e0362bf2..7fba4bc458 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>
> > > @@ -1822,6 +1823,53 @@ 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;
> > > +
> > > +     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;
> > > +
> > > +     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);
> > > +     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
> > >    *
> > > @@ -1885,6 +1933,8 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> > >               free(data);
> > >       }
> > >
> > > +     ret = tcg2_measure_deployed_audit_mode(dev);
> >
> > You do the same thing four times. A loop is preferable.
>
> After your series(efi_loader: centralize known vendor GUIDs) is merged,
> I will update this patch to use loop for the secure variables.

I was just about to send v4 patch, I noticed that the following patches
are removed from efi-2022-01 branch.

5024ec8ea7 efi_loader: simplify tcg2_measure_secure_boot_variable()
1622f0234b efi_loader: simplify efi_sigstore_parse_sigdb()
57d1f4a29e efi_loader: function to get GUID for variable name
d326a99318 efi_loader: treat UEFI variable name as const

These patches were there at least until last week I think.
Do you plan not to merge these patches?

Thanks,
Masahisa Kojima


>
> Thanks,
> Masahisa Kojima
>
> >
> > Best regards
> >
> > Heinrich
> >
> > > +
> > >   error:
> > >       return ret;
> > >   }
> > >