Re: [PATCH v3 1/5] efi_loader: add secure boot variable measurement

[Masahisa Kojima <masahisa.kojima@linaro.org>]

Hi Akashi-san,

Thank you for your comment.

On Tue, 10 Aug 2021 at 10:44, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Kojima-san,
>
> On Fri, Aug 06, 2021 at 04:02:11PM +0900, 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", "dbx", "dbt", and "dbr".
> >
> > 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@linaro.org>
> > ---
> > Changes in v3:
> > - add "dbt" and "dbr" measurement
> > - accept empty variable measurement for "SecureBoot", "PK",
> >   "KEK", "db" and "dbx" as TCG2 spec requires
> > - fix comment format
> >
> > Changes in v2:
> > - missing null check for getting variable data
> > - some minor fix for readability
> >
> >  include/efi_tcg2.h        |  20 +++++
> >  lib/efi_loader/efi_tcg2.c | 165 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 185 insertions(+)
> >
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > index bcfb98168a..497ba3ce94 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 - event log structure of UEFI variable
> > + * @variable_name:           The vendorGUID parameter in the
> > + *                           GetVariable() API.
> > + * @unicode_name_length:     The length in CHAR16 of the Unicode name of
> > + *                           the variable.
> > + * @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..a2e9587cd0 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},
> > +};
> > +
> >  #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,118 @@ 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(u16)) + data_size;
> > +     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(u16)));
> > +     if (data) {
> > +             memcpy((u16 *)event->unicode_name + event->unicode_name_length,
> > +                    data, data_size);
> > +     }
> > +     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++) {
> > +             /*
> > +              * According to the TCG2 PC Client PFP spec, "SecureBoot",
> > +              * "PK", "KEK", "db" and "dbx" variables must be measured
> > +              * even if they are empty.
> > +              */
> > +             data = efi_get_var(secure_variables[i].name,
> > +                                secure_variables[i].guid,
> > +                                &data_size);
> > +
> > +             ret = tcg2_measure_variable(dev, 7,
> > +                                         EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                         secure_variables[i].name,
> > +                                         secure_variables[i].guid,
> > +                                         data_size, data);
> > +             free(data);
> > +             if (ret != EFI_SUCCESS)
> > +                     goto error;
> > +     }
> > +
> > +     /*
> > +      * TCG2 PC Client PFP spec says "dbt" and "dbr" are
> > +      * measured if present and not empty.
> > +      */
>
> The current implementation of secure boot doesn't support neither
> dbt or dbr. Do we have to measure them now?

As Heinrich commented to my v2 patch, dbt and dbr measurement are added,
because now would not harm anything.

> I think that EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION bit in
> osIndicationSupported should always be checked first.

According to the TCG PC Client PFP spec,
dbt and dbr are measured if present and not empty, regardless of
the osIndicationSupported bit.
 # EDK2 also measures dbt without checking osIndicationSupported bit.

Thanks,
Masahisa Kojima

>
> > +     data = efi_get_var(L"dbt",
> > +                        &efi_global_variable_guid,
> > +                        &data_size);
> > +     if (data) {
> > +             ret = tcg2_measure_variable(dev, 7,
> > +                                         EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                         L"dbt",
> > +                                         &efi_global_variable_guid,
>
> => efi_guid_image_security_database
>
> > +                                         data_size, data);
> > +             free(data);
> > +     }
> > +
> > +     data = efi_get_var(L"dbr",
> > +                        &efi_global_variable_guid,
> > +                        &data_size);
> > +     if (data) {
> > +             ret = tcg2_measure_variable(dev, 7,
> > +                                         EV_EFI_VARIABLE_DRIVER_CONFIG,
> > +                                         L"dbr",
> > +                                         &efi_global_variable_guid,
>
> ditto for dbr
>
> -Takahiro Akashi
>
> > +                                         data_size, data);
> > +             free(data);
> > +     }
> > +
> > +error:
> > +     return ret;
> > +}
> > +
> >  /**
> >   * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> >   *
> > @@ -1328,6 +1486,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:
> > --
> > 2.17.1
> >