[PATCH 2/5] efi_loader: add secure boot variable measurement
Masahisa Kojima
masahisa.kojima at linaro.org
Fri Jul 9 04:34:50 CEST 2021
On Fri, 9 Jul 2021 at 02:46, Heinrich Schuchardt <xypron.glpk at 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.kojima at 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:
> >
>
More information about the U-Boot
mailing list