From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7FAA5C433F5 for ; Wed, 15 Sep 2021 08:37:18 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E645061185 for ; Wed, 15 Sep 2021 08:37:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E645061185 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 1555682C84; Wed, 15 Sep 2021 10:37:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="EQnIzG0e"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 5786982D13; Wed, 15 Sep 2021 10:37:14 +0200 (CEST) Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [IPv6:2a00:1450:4864:20::32e]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id E87C1829C6 for ; Wed, 15 Sep 2021 10:37:09 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wm1-x32e.google.com with SMTP id j17-20020a05600c1c1100b002e754875260so1377762wms.4 for ; Wed, 15 Sep 2021 01:37:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=vxHdQSdj8yEvVLmJXQGWdx8xxq9KqVgKeidi8Y5MqG0=; b=EQnIzG0e4GBDuqD0IjjrAXQgj6yDSXOryN0HzcnVSDRiIZwIbgoX5/VzEfguzoCh77 bIl6GEeqaewDUhEcQCpIMEK3tikk9mHH9qBtk9IjxFUzQXMKD8rVR2usJuZn7cddj32X SAv6OJbkvIg9ld35LPgL1/4VFRkuKSS+inQEtmDqDOPoroy03KRX8IRRVa+NEgtoWYZX V2HMtwssHcEh5LJuVQ9DzQJV7Ye9EbkHxmvRMZkKjhLiG9FUcaOIADdCfu+7aLliieju 5wwwykIULx0tNwTFDzEUKuE4YqXE3k2fsg8A4o7e6Q7C6C5+esRxpj7KRcQaHkfPwww1 LkUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=vxHdQSdj8yEvVLmJXQGWdx8xxq9KqVgKeidi8Y5MqG0=; b=jSvOLpAVnIwYRwpXbKyNPhILbak16N5g+Sz7DmzCOlbFpaOS3CjRkVS7gRAxee9YO5 8+NV/JaG6YVNtDlmBm3VEzWUUYv3DSIfGfo0S7L2ihhM/kzA+hWDaGmEdAdswI4a56oD +OLhOuN481Spj6kSWN5F7Wf56UF+p3sqYinm+9CQBbElXGd/jobvN0Djedn0NHhZ41Rl w2AeyAJdhWNlNpe8505ekpVe/qx0Z/9D/i3/k7xdQGRsCfEk7Qr5EK2cSgXo2Rstkk3y ycS3jXJrWR9fevjLZlRAU18ecKvpm30JWhzKPM909wDBtcNZ3ONZ/UHs066TnPxQgemU 0FpA== X-Gm-Message-State: AOAM533L3eBC9PJmrY7Lo2piyhwnPB167s152bbvPxANmBEtfy+ZY4OF m1kAV5izlRUwoLkYfIExy8JjQA== X-Google-Smtp-Source: ABdhPJyVG6Gem9Yga59FDUlXwlBA3Ueq3ozBjOmPqNcco3r6dWUjuQrBewjA2rGbQHgtae9fHq9Odg== X-Received: by 2002:a1c:3881:: with SMTP id f123mr3121198wma.185.1631695029305; Wed, 15 Sep 2021 01:37:09 -0700 (PDT) Received: from apalos.home (ppp-94-66-220-137.home.otenet.gr. [94.66.220.137]) by smtp.gmail.com with ESMTPSA id i18sm8778680wrn.64.2021.09.15.01.37.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Sep 2021 01:37:08 -0700 (PDT) Date: Wed, 15 Sep 2021 11:37:06 +0300 From: Ilias Apalodimas To: Masahisa Kojima Cc: Heinrich Schuchardt , Alexander Graf , Simon Glass , Bin Meng , Christian Gmeiner , u-boot@lists.denx.de Subject: Re: [PATCH 1/3] efi_loader: add SMBIOS table measurement Message-ID: References: <20210915051546.500-1-masahisa.kojima@linaro.org> <20210915051546.500-2-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210915051546.500-2-masahisa.kojima@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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 > --- > 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? > + > 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 > #include > > -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 > #include > #include > +#include > #include > #include > #include > @@ -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? > + 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? > +{ > + 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? > + > + } 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