[PATCH 1/3] efi_loader: add SMBIOS table measurement
Masahisa Kojima
masahisa.kojima at linaro.org
Wed Sep 15 14:59:48 CEST 2021
Hi Ilias,
Thank you for the review.
On Wed, 15 Sept 2021 at 17:37, Ilias Apalodimas
<ilias.apalodimas at linaro.org> 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 <masahisa.kojima at linaro.org>
> > ---
> > 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
More information about the U-Boot
mailing list