[PATCH v3 1/3] efi_loader: add SMBIOS table measurement
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Oct 21 10:49:50 CEST 2021
On 10/21/21 10:38, Masahisa Kojima wrote:
> On Thu, 21 Oct 2021 at 09:17, Heinrich Schuchardt <xypron.glpk at 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 at 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);
>>> + }
>>> +}
>>>
>>
More information about the U-Boot
mailing list