Re: [PATCH 2/2] efi/libstub: measure EFI LoadOptions

[Ard Biesheuvel]

On Fri, 16 Sept 2022 at 10:15, Ilias Apalodimas
<ilias.apalodimas@xxxxxxxxxx> wrote:
>
> The EFI TCG spec, in §10.2.6 Measuring UEFI Variables and UEFI GPT Data,
> is  measuring the entire UEFI_LOAD_OPTION (in PCR5).  As a result boot
> variables that point to the same UEFI application but with different
> optional data,  will have distinct measurements.
>

That is not the main problem. The main problem is that
LoadImage()/StartImage() may be used to invoke things beyond Boot####
options, and at StartImage() time, the load options could be anything.
So not measuring the load options when the image is actually being
invoked is a huge oversight.


> However, PCR5 is used for more than that and there might be a need to use
> a PCR with a more limited scope which measures our initramfs and
> LoadOptions.
>
> So add a measurement in PCR9 (which we already use for our initrd) and
> extend it with the LoadOption measurements
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
> ---
>  .../firmware/efi/libstub/efi-stub-helper.c    | 21 +++++++++++++++++++
>  drivers/firmware/efi/libstub/efistub.h        |  1 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 3ef4867344b9..5b03248527c6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -370,6 +370,27 @@ char *efi_convert_cmdline(efi_loaded_image_t *image, int *cmd_line_len)
>         int options_bytes = 0, safe_options_bytes = 0;  /* UTF-8 bytes */
>         bool in_quote = false;
>         efi_status_t status;
> +       static const struct efi_measured_event load_options_tcg2_event = {
> +               {
> +                       sizeof(load_options_tcg2_event) + sizeof("Load Options"),
> +                       {
> +                               sizeof(load_options_tcg2_event.event_data.event_header),
> +                               EFI_TCG2_EVENT_HEADER_VERSION,
> +                               9,
> +                               EV_EVENT_TAG,
> +                       },
> +               },
> +               {
> +                       LOAD_OPTIONS_EVENT_TAG_ID,
> +                       sizeof("Load Options"),
> +               },
> +               { "Load Options" },
> +       };
> +
> +       if (options_chars > 0)
> +               efi_measure_tagged_event((unsigned long) options,
> +                                        (unsigned long) options_chars,
> +                                        &load_options_tcg2_event);
>

The name 'options_chars' is a bit misleading here, as it is actually
the size in bytes at this point.

>         efi_apply_loadoptions_quirk((const void **)&options, &options_chars);
>         options_chars /= sizeof(*options);
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index cb7eb5ed9f14..e3605b383964 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -741,6 +741,7 @@ union apple_properties_protocol {
>  typedef u32 efi_tcg2_event_log_format;
>
>  #define INITRD_EVENT_TAG_ID 0x8F3B22ECU
> +#define LOAD_OPTIONS_EVENT_TAG_ID 0x8F3B22EDU

Is this an arbitrarily chosen value?

>  #define EV_EVENT_TAG 0x00000006U
>  #define EFI_TCG2_EVENT_HEADER_VERSION  0x1
>
> --
> 2.34.1
>