On Thu, 21 Oct 2021 at 17:49, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > > > On 10/21/21 10:38, Masahisa Kojima wrote: > > On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.g...@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.koj...@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); > >>> + } > >>> +} > >>> > >>