On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 7/7/21 3:36 PM, Masahisa Kojima wrote: > > TCG PC Client PFP spec requires to measure the secure > > boot policy before validating the UEFI image. > > This commit adds the secure boot variable measurement > > of "SecureBoot", "PK", "KEK", "db" and "dbx". > > > > Note that this implementation assumes that secure boot > > variables are pre-configured and not be set/updated in runtime. > > > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > > --- > > include/efi_tcg2.h | 20 ++++++ > > lib/efi_loader/efi_tcg2.c | 135 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 155 insertions(+) > > > > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h > > index bcfb98168a..8d7b77c087 100644 > > --- a/include/efi_tcg2.h > > +++ b/include/efi_tcg2.h > > @@ -142,6 +142,26 @@ struct efi_tcg2_final_events_table { > > struct tcg_pcr_event2 event[]; > > }; > > > > +/** > > + * struct tdUEFI_VARIABLE_DATA > > Please, add a reference to the "TCG PC Client PlatformFirmware Profile > specification".
In addition to this tdUEFI_VARIABLE_DATA structure, other structures defined in efi_tcg2.h such as tdEFI_TCG2_FINAL_EVENTS_TABLE are also referencing "TCG PC Client PlatformFirmware Profile specification. So I will add file-wide reference in the file header. efi_tcg2.h also includes definitions described in "TCG EFI Protocol Specification Revision 00.13 March 30, 2016" > > > + * @variable_name: The vendorGUID parameter in the > > + * GetVariable() API. > > + * @unicode_name_length: The length in CHAR16 of the Unicode name of > > + * the variable. > > Where is this specified as CHAR16 count? I could not find it in the "TCG > PC Client PlatformFirmware Profile specification". Would you refer the latest spec? https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/ (Version 1.05 Revision 23 May 7, 2021), Page 86. > > > + * @variable_data_length: The size of the variable data. > > + * @unicode_name: The CHAR16 unicode name of the variable > > + * without NULL-terminator. > > + * @variable_data: The data parameter of the efi variable > > + * in the GetVariable() API. > > + */ > > +struct efi_tcg2_uefi_variable_data { > > + efi_guid_t variable_name; > > + u64 unicode_name_length; > > + u64 variable_data_length; > > + u16 unicode_name[1]; > > + u8 variable_data[1]; > > +}; > > + > > 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/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index 1319a8b378..2a248bd62a 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -78,6 +78,19 @@ static const struct digest_info hash_algo_list[] = { > > }, > > }; > > > > +struct variable_info { > > + u16 *name; > > + const efi_guid_t *guid; > > +}; > > + > > +static struct variable_info secure_variables[] = { > > + {L"SecureBoot", &efi_global_variable_guid}, > > + {L"PK", &efi_global_variable_guid}, > > + {L"KEK", &efi_global_variable_guid}, > > + {L"db", &efi_guid_image_security_database}, > > + {L"dbx", &efi_guid_image_security_database}, > > You have to measure BootOrder and Boot#### too. > See TCG PC Client Platform Firmware Profile Specification, March 30, 2016. These variables measurement is included in a separate patch in this series. Could you check this? "[PATCH 3/5] efi_loader: add boot variable measurement" > > > +}; > > + > > #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list) > > > > /** > > @@ -1264,6 +1277,39 @@ free_pool: > > return ret; > > } > > > > +/** > > + * tcg2_measure_event() - common function to add event log and extend PCR > > + * > > + * @dev: TPM device > > + * @pcr_index: PCR index > > + * @event_type: type of event added > > + * @size: event size > > + * @event: event data > > + * > > + * Return: status code > > + */ > > +static efi_status_t EFIAPI > > +tcg2_measure_event(struct udevice *dev, u32 pcr_index, u32 event_type, > > + u32 size, u8 event[]) > > +{ > > + struct tpml_digest_values digest_list; > > + efi_status_t ret; > > + > > + ret = tcg2_create_digest(event, size, &digest_list); > > + if (ret != EFI_SUCCESS) > > + goto out; > > + > > + ret = tcg2_pcr_extend(dev, pcr_index, &digest_list); > > + if (ret != EFI_SUCCESS) > > + goto out; > > + > > + ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list, > > + size, event); > > + > > +out: > > + return ret; > > +} > > + > > /** > > * efi_append_scrtm_version - Append an S-CRTM EV_S_CRTM_VERSION event on > > the > > * eventlog and extend the PCRs > > @@ -1294,6 +1340,88 @@ out: > > return ret; > > } > > > > +/** > > + * tcg2_measure_variable() - add variable event log and extend PCR > > + * > > + * @dev: TPM device > > + * @pcr_index: PCR index > > + * @event_type: type of event added > > + * @var_name: variable name > > + * @guid: guid > > + * @data_size: variable data size > > + * @data: variable data > > + * > > + * Return: status code > > + */ > > +static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 > > pcr_index, > > + u32 event_type, u16 *var_name, > > + const efi_guid_t *guid, > > + efi_uintn_t data_size, u8 *data) > > +{ > > + u32 event_size; > > + efi_status_t ret; > > + struct efi_tcg2_uefi_variable_data *event; > > + > > + event_size = sizeof(event->variable_name) + > > + sizeof(event->unicode_name_length) + > > + sizeof(event->variable_data_length) + > > + (u16_strlen(var_name) * sizeof(*var_name)) + data_size; > > Please, replace by sizeof(*var_name) by sizeof(u16) to improve readability. > > > + event = malloc(event_size); > > + if (!event) > > + return EFI_OUT_OF_RESOURCES; > > + > > + guidcpy(&event->variable_name, guid); > > + event->unicode_name_length = u16_strlen(var_name); > > + event->variable_data_length = data_size; > > + memcpy(event->unicode_name, var_name, > > + (event->unicode_name_length * sizeof(*event->unicode_name))); > > sizeof(u16) > > > + memcpy((u16 *)event->unicode_name + event->unicode_name_length, > > + (u8 *)data, data_size); > > memcpy() wants void *. data is already of type u8 *. This conversion can > be removed. > > > + ret = tcg2_measure_event(dev, pcr_index, event_type, event_size, > > + (u8 *)event); > > + free(event); > > + return ret; > > +} > > + > > +/** > > + * tcg2_measure_secure_boot_variable() - measure secure boot variables > > + * > > + * @dev: TPM device > > + * > > + * Return: status code > > + */ > > +static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev) > > +{ > > + u8 *data; > > + efi_uintn_t data_size; > > + u32 count, i; > > + efi_status_t ret; > > + > > + count = ARRAY_SIZE(secure_variables); > > + for (i = 0; i < count; i++) { > > + data = efi_get_var(secure_variables[i].name, > > + secure_variables[i].guid, > > + &data_size); > > You need to check if data==NULL. There is no guarantee that the > variables exist. Yes, I missed this checking. Thanks, Masahisa Kojima > > Best regards > > Heinrich > > > + > > + ret = tcg2_measure_variable(dev, 7, > > + EV_EFI_VARIABLE_DRIVER_CONFIG, > > + secure_variables[i].name, > > + secure_variables[i].guid, > > + data_size, (u8 *)data); > > + free(data); > > + if (ret != EFI_SUCCESS) > > + goto error; > > + } > > + > > + /* > > + * TODO: add DBT and DBR measurement support when u-boot supports > > + * these variables. > > + */ > > + > > +error: > > + return ret; > > +} > > + > > /** > > * efi_tcg2_register() - register EFI_TCG2_PROTOCOL > > * > > @@ -1328,6 +1456,13 @@ efi_status_t efi_tcg2_register(void) > > tcg2_uninit(); > > goto fail; > > } > > + > > + ret = tcg2_measure_secure_boot_variable(dev); > > + if (ret != EFI_SUCCESS) { > > + tcg2_uninit(); > > + goto fail; > > + } > > + > > return ret; > > > > fail: > > >