Hi Ilias, Thank you for the review.
On Wed, 15 Sept 2021 at 17:37, Ilias Apalodimas <[email protected]> wrote: > > Hi Kojima-san, > > On Wed, Sep 15, 2021 at 02:15:44PM +0900, Masahisa Kojima 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. > > > > 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 <[email protected]> > > --- > > include/efi_loader.h | 2 + > > include/efi_tcg2.h | 15 ++++ > > include/smbios.h | 13 ++++ > > 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 | 127 +++++++++++++++++++++++++++++++++- > > 8 files changed, 243 insertions(+), 3 deletions(-) > > > > diff --git a/include/efi_loader.h b/include/efi_loader.h > > index c440962fe5..13f0c24058 100644 > > --- a/include/efi_loader.h > > +++ b/include/efi_loader.h > > @@ -308,6 +308,8 @@ extern const efi_guid_t efi_guid_capsule_report; > > extern const efi_guid_t efi_guid_firmware_management_protocol; > > /* GUID for the ESRT */ > > extern const efi_guid_t efi_esrt_guid; > > +/* GUID of the SMBIOS table */ > > +extern const efi_guid_t smbios_guid; > > > > extern char __efi_runtime_start[], __efi_runtime_stop[]; > > extern char __efi_runtime_rel_start[], __efi_runtime_rel_stop[]; > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > > index 5a1a36212e..da33f8a1d0 100644 > > --- a/include/efi_tcg2.h > > +++ b/include/efi_tcg2.h > > @@ -215,6 +215,21 @@ struct efi_tcg2_uefi_variable_data { > > u8 variable_data[1]; > > }; > > > > +/** > > + * struct tdUEFI_HANDOFF_TABLE_POINTERS2 - event log structure of SMBOIS > > tables > > + * @table_description_size: size of table description > > + * @table_description: table description > > + * @number_of_tables: number of uefi configuration table > > + * @table_entry: uefi configuration table entry > > + */ > > +#define SMBIOS_HANDOFF_TABLE_DESC "SmbiosTable" > > +struct smbios_handoff_table_pointers2 { > > + u8 table_description_size; > > + u8 table_description[sizeof(SMBIOS_HANDOFF_TABLE_DESC)]; > > + u64 number_of_tables; > > + struct efi_configuration_table table_entry[1]; > > +} __packed; > > Is this included in any other struct or array? If not the last member could > be a flexible array member? Yes, it can be a flexible array. > > > + > > 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..0c22c0c489 100644 > > --- a/include/smbios.h > > +++ b/include/smbios.h > > @@ -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 dacc3b5881..ac1a281a36 100644 > > --- a/lib/efi_loader/Kconfig > > +++ b/lib/efi_loader/Kconfig > > @@ -327,6 +327,7 @@ config EFI_TCG2_PROTOCOL > > select SHA384 > > select SHA512 > > select HASH > > + select SMBIOS_PARSER > > help > > Provide a EFI_TCG2_PROTOCOL implementation using the TPM hardware > > of the platform. > > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c > > index f0283b539e..701e2212c8 100644 > > --- a/lib/efi_loader/efi_boottime.c > > +++ b/lib/efi_loader/efi_boottime.c > > @@ -86,6 +86,8 @@ const efi_guid_t efi_guid_event_group_reset_system = > > /* GUIDs of the Load File and Load File2 protocols */ > > const efi_guid_t efi_guid_load_file_protocol = EFI_LOAD_FILE_PROTOCOL_GUID; > > const efi_guid_t efi_guid_load_file2_protocol = > > EFI_LOAD_FILE2_PROTOCOL_GUID; > > +/* GUID of the SMBIOS table */ > > +const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; > > > > static efi_status_t EFIAPI efi_disconnect_controller( > > efi_handle_t controller_handle, > > diff --git a/lib/efi_loader/efi_smbios.c b/lib/efi_loader/efi_smbios.c > > index 2eb4cb1c1a..fc0b23397c 100644 > > --- a/lib/efi_loader/efi_smbios.c > > +++ b/lib/efi_loader/efi_smbios.c > > @@ -13,8 +13,6 @@ > > #include <mapmem.h> > > #include <smbios.h> > > > > -static const efi_guid_t smbios_guid = SMBIOS_TABLE_GUID; > > - > > /* > > * Install the SMBIOS table as a configuration table. > > * > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index cb48919223..7f47998a55 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -14,6 +14,7 @@ > > #include <efi_tcg2.h> > > #include <log.h> > > #include <malloc.h> > > +#include <smbios.h> > > #include <version.h> > > #include <tpm-v2.h> > > #include <u-boot/hash-checksum.h> > > @@ -1449,6 +1450,81 @@ error: > > return ret; > > } > > > > +/** > > + * tcg2_measure_smbios() - measure smbios table > > + * > > + * @dev: TPM device > > + * @entry: pointer to the smbios_entry structure > > + * > > + * Return: status code > > + */ > > +static efi_status_t > > +tcg2_measure_smbios(struct udevice *dev, > > + const struct smbios_entry *entry) > > +{ > > + efi_status_t ret; > > + struct smbios_header *smbios_copy; > > + struct smbios_handoff_table_pointers2 *event = NULL; > > + u32 event_size; > > + > > + /* > > + * TCG PC Client PFP Spec says > > + * "SMBIOS structures that contain static configuration information > > + * (e.g. Platform Manufacturer Enterprise Number assigned by IANA, > > + * platform model number, Vendor and Device IDs for each SMBIOS table) > > + * that is relevant to the security of the platform MUST be measured". > > + * Device dependent parameters such as serial number are cleared to > > + * zero or spaces for the measurement. > > + */ > > + event_size = sizeof(struct smbios_handoff_table_pointers2) + > > + entry->struct_table_length - > > + FIELD_SIZEOF(struct efi_configuration_table, table); > > Why do we have to subtract the efi_config_table table field here? efi_config_table *table field is used to point the address of smbios table data, actual smbios table size is indicated by entry->struct_table_length. efi_config_table *table field itself is not used and we need to substract it. > > > + 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; > > +} > > + > > +/** > > + * search_smbios_table() - search smbios table > > + * > > + * Return: pointer to the smbios table > > + */ > > +static void *search_smbios_table(void) > > Isn't find_smbios_table a better name? I agree, my first choice was "find", but I finally chose "search". I will replace with "find". > > > +{ > > + u32 i; > > + > > + for (i = 0; i < systab.nr_tables; i++) { > > + if (!guidcmp(&smbios_guid, &systab.tables[i].guid)) > > + return systab.tables[i].table; > > + } > > + > > + return NULL; > > +} > > + > > /** > > * efi_tcg2_measure_efi_app_invocation() - measure efi app invocation > > * > > @@ -1460,6 +1536,7 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(void) > > u32 pcr_index; > > struct udevice *dev; > > u32 event = 0; > > + struct smbios_entry *entry; > > > > if (tcg2_efi_app_invoked) > > return EFI_SUCCESS; > > @@ -1485,6 +1562,13 @@ efi_status_t > > efi_tcg2_measure_efi_app_invocation(void) > > goto out; > > } > > > > + entry = (struct smbios_entry *)search_smbios_table(); > > + if (entry) { > > + ret = tcg2_measure_smbios(dev, entry); > > + if (ret != EFI_SUCCESS) > > + goto out; > > + } > > + > > tcg2_efi_app_invoked = true; > > out: > > return ret; > > diff --git a/lib/smbios-parser.c b/lib/smbios-parser.c > > index 34203f952c..5e0bd8f4ca 100644 > > --- a/lib/smbios-parser.c > > +++ b/lib/smbios-parser.c > > @@ -56,7 +56,7 @@ static const struct smbios_header *next_header(const > > struct smbios_header *curr) > > 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) > > @@ -132,3 +132,128 @@ 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; > > + /* > > + * smbios_string() returns const value, but this > > + * function needs to clear the string. > > + */ > > + str = (char *)smbios_string(header, string_id); > > + if (!str) > > + continue; > > + /* string is cleared to space */ > > + memset(str, ' ', strlen(str)); > > Isn't this undefined behavior ? If the area pointed to by whatever > smbios_string() is ro that will crash? Yes, this code is not good, I will fix. Same instance exists in smbios_prepare_measurement(). Thanks, Masahisa Kojima > > > + > > + } 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; > > + /* > > + * next_header() returns the const value, but some > > + * members need to be cleared for the measurement. > > + */ > > + header = (struct smbios_header *)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 > > > > Regards > /Ilias
