Re: [PATCH v8 3/3] efi_loader: add PE/COFF image measurement

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

On Mon, 24 May 2021 at 21:53, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> new_efi);
> > +
> >  bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >                    WIN_CERTIFICATE **auth, size_t *auth_len);
> >
> > diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> > index 40e241ce31..bcfb98168a 100644
> > --- a/include/efi_tcg2.h
> > +++ b/include/efi_tcg2.h
> > @@ -9,6 +9,7 @@
> >  #if !defined _EFI_TCG2_PROTOCOL_H_
> >  #define _EFI_TCG2_PROTOCOL_H_
> >
> > +#include <efi_api.h>
> >  #include <tpm-v2.h>
> >
> >  #define EFI_TCG2_PROTOCOL_GUID \
> > @@ -53,6 +54,14 @@ struct efi_tcg2_event {
> >       u8 event[];
> >  } __packed;
> >
> > +struct uefi_image_load_event {
> > +     efi_physical_addr_t image_location_in_memory;
> > +     u64 image_length_in_memory;
> > +     u64 image_link_time_address;
> > +     u64 length_of_device_path;
> > +     struct efi_device_path device_path[];
> > +};
> > +
> >  struct efi_tcg2_boot_service_capability {
> >       u8 size;
> >       struct efi_tcg2_version structure_version;
> > diff --git a/include/tpm-v2.h b/include/tpm-v2.h
> > index 7de7d6a57d..247b386967 100644
> > --- a/include/tpm-v2.h
> > +++ b/include/tpm-v2.h
> > @@ -70,6 +70,24 @@ struct udevice;
> >  #define EV_TABLE_OF_DEVICES          ((u32)0x0000000B)
> >  #define EV_COMPACT_HASH                      ((u32)0x0000000C)
> >
> > +/*
> > + * event types, cf.
> > + * "TCG PC Client Platform Firmware Profile Specification", Family "2.0"
> > + * rev 1.04, June 3, 2019
> > + */
> > +#define EV_EFI_EVENT_BASE                    ((u32)0x80000000)
> > +#define EV_EFI_VARIABLE_DRIVER_CONFIG                ((u32)0x80000001)
> > +#define EV_EFI_VARIABLE_BOOT                 ((u32)0x80000002)
> > +#define EV_EFI_BOOT_SERVICES_APPLICATION     ((u32)0x80000003)
> > +#define EV_EFI_BOOT_SERVICES_DRIVER          ((u32)0x80000004)
> > +#define EV_EFI_RUNTIME_SERVICES_DRIVER               ((u32)0x80000005)
> > +#define EV_EFI_GPT_EVENT                     ((u32)0x80000006)
> > +#define EV_EFI_ACTION                                ((u32)0x80000007)
> > +#define EV_EFI_PLATFORM_FIRMWARE_BLOB                ((u32)0x80000008)
> > +#define EV_EFI_HANDOFF_TABLES                        ((u32)0x80000009)
> > +#define EV_EFI_HCRTM_EVENT                   ((u32)0x80000010)
> > +#define EV_EFI_VARIABLE_AUTHORITY            ((u32)0x800000E0)
> > +
> >  /* TPMS_TAGGED_PROPERTY Structure */
> >  struct tpms_tagged_property {
> >       u32 property;
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 98845b8ba3..0e6200fa25 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -309,6 +309,7 @@ config EFI_TCG2_PROTOCOL
> >       select SHA512_ALGO
> >       select SHA384
> >       select SHA512
> > +     select HASH_CALCULATE
> >       help
> >         Provide a EFI_TCG2_PROTOCOL implementation using the TPM hardware
> >         of the platform.
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index fe1ee198e2..f37a85e56e 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -302,6 +302,40 @@ static int cmp_pe_section(const void *arg1, const void *arg2)
> >               return 1;
> >  }
> >
> > +/**
> > + * efi_prepare_aligned_image() - prepare 8-byte aligned image
> > + * @efi:             pointer to the EFI binary
> > + * @efi_size:                size of @efi binary
> > + * @new_efi:         pointer to the newly allocated image
> > + *
> > + * If @efi is not 8-byte aligned, this function newly allocates
> > + * the image buffer and updates @efi_size.
> > + *
> > + * Return:   valid pointer to a image, return NULL if allocation fails.
> > + */
> > +void *efi_prepare_aligned_image(void *efi, u64 *efi_size, void **new_efi)
> > +{
> > +     size_t new_efi_size;
> > +     void *p;
> > +
> > +     /*
> > +      * Size must be 8-byte aligned and the trailing bytes must be
> > +      * zero'ed. Otherwise hash value may be incorrect.
> > +      */
> > +     if (!IS_ALIGNED(*efi_size, 8)) {
> > +             new_efi_size = ALIGN(*efi_size, 8);
> > +             p = calloc(new_efi_size, 1);
> > +             if (!p)
> > +                     return NULL;
> > +             memcpy(p, efi, *efi_size);
> > +             *efi_size = new_efi_size;
>
> Do we really need new_efi here?  I might be missing some context for the
> original code, but since we return the new pointer, can't we just
> use that in the caller? If so the whole void **new_efi is not needed?

new_efi is required.
The caller uses returned pointer, it will be the pointer to the
original efi image or newly allocated buffer. Caller will need new_efi
to free the newly allocated buffer.

>
> > +
>
> [...]
>
> > +     ret = __get_active_pcr_banks(&active);
> > +     if (ret != EFI_SUCCESS) {
> > +             ret = EFI_DEVICE_ERROR;
>
> __get_active_pcr_banks is supposed to return the correct efi_status_t code.
> I don't think we need EFI_DEVICE_ERROR here.

Thank you. I will just use return value of __get_active_pcr_banks().

> +       if (!efi_image_parse(efi, efi_size, &regs, &wincerts,
> +                            &wincerts_len)) {
> +               log_err("Parsing PE executable image failed\n");
> +               ret = EFI_INVALID_PARAMETER;
> +               goto out;

I re-checked TCG spec, if PE/COFF image is corrupted or not understood,
the function shall return EFI_UNSUPPORTED. I will also update.

Thanks,
Masahisa Kojima

>
>
> Thanks!
> /Ilias