[v3,1/5] efi/libstub: Use correct event size when measuring data into the TPM

From: Ard Biesheuvel <ardb@kernel.org>

Our efi_tcg2_tagged_event is not defined in the EFI spec, but it is not
a local invention either: it was taken from the TCG PC Client spec,
where it is called TCG_PCClientTaggedEvent.

This spec also contains some guidance on how to populate it, which
is not being followed closely at the moment; the event size should cover
the TCG_PCClientTaggedEvent and its payload only, but it currently
covers the preceding efi_tcg2_event too, and this may result in trailing
garbage being measured into the TPM.

So rename the struct and document its provenance, and fix up the use so
only the tagged event data is represented in the size field.

Cc: <stable@vger.kernel.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++---------
 drivers/firmware/efi/libstub/efistub.h         | 12 ++++++------
 2 files changed, 17 insertions(+), 15 deletions(-)

Hi Ard

On Fri, 8 Mar 2024 at 10:58, Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> Our efi_tcg2_tagged_event is not defined in the EFI spec, but it is not
> a local invention either: it was taken from the TCG PC Client spec,
> where it is called TCG_PCClientTaggedEvent.
>
> This spec also contains some guidance on how to populate it, which
> is not being followed closely at the moment; the event size should cover
> the TCG_PCClientTaggedEvent and its payload only, but it currently
> covers the preceding efi_tcg2_event too, and this may result in trailing
> garbage being measured into the TPM.

I think there's a confusion here and the current code we have is correct.
The EFI TCG spec [0] says that the tdEFI_TCG2_EVENT size is:
"Total size of the event including the Size component, the header and the
Event data." which obviously contradicts the definition of the tagged
event in the PC client spec.
But given the fact that TCG_PCClientTaggedEvent has its own size field
I think we should use what we already have.


[0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
page 33

Cheers
/Ilias

>
> So rename the struct and document its provenance, and fix up the use so
> only the tagged event data is represented in the size field.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  drivers/firmware/efi/libstub/efi-stub-helper.c | 20 +++++++++++---------
>  drivers/firmware/efi/libstub/efistub.h         | 12 ++++++------
>  2 files changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index bfa30625f5d0..16843ab9b64d 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -11,6 +11,7 @@
>
>  #include <linux/efi.h>
>  #include <linux/kernel.h>
> +#include <linux/overflow.h>
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>
> @@ -219,23 +220,24 @@ static const struct {
>         },
>  };
>
> +struct efistub_measured_event {
> +       efi_tcg2_event_t        event_data;
> +       TCG_PCClientTaggedEvent tagged_event;
> +} __packed;
> +
>  static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>                                              unsigned long load_size,
>                                              enum efistub_event event)
>  {
> +       struct efistub_measured_event *evt;
> +       int size = struct_size(&evt->tagged_event, tagged_event_data,
> +                              events[event].event_data_len);
>         efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
>         efi_tcg2_protocol_t *tcg2 = NULL;
>         efi_status_t status;
>
>         efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
>         if (tcg2) {
> -               struct efi_measured_event {
> -                       efi_tcg2_event_t        event_data;
> -                       efi_tcg2_tagged_event_t tagged_event;
> -                       u8                      tagged_event_data[];
> -               } *evt;
> -               int size = sizeof(*evt) + events[event].event_data_len;
> -
>                 status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>                                      (void **)&evt);
>                 if (status != EFI_SUCCESS)
> @@ -249,12 +251,12 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>                         .event_header.event_type        = EV_EVENT_TAG,
>                 };
>
> -               evt->tagged_event = (struct efi_tcg2_tagged_event){
> +               evt->tagged_event = (TCG_PCClientTaggedEvent){
>                         .tagged_event_id                = events[event].event_id,
>                         .tagged_event_data_size         = events[event].event_data_len,
>                 };
>
> -               memcpy(evt->tagged_event_data, events[event].event_data,
> +               memcpy(evt->tagged_event.tagged_event_data, events[event].event_data,
>                        events[event].event_data_len);
>
>                 status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index c04b82ea40f2..043a3ff435f3 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -843,14 +843,14 @@ struct efi_tcg2_event {
>         /* u8[] event follows here */
>  } __packed;
>
> -struct efi_tcg2_tagged_event {
> -       u32 tagged_event_id;
> -       u32 tagged_event_data_size;
> -       /* u8  tagged event data follows here */
> -} __packed;
> +/* from TCG PC Client Platform Firmware Profile Specification */
> +typedef struct tdTCG_PCClientTaggedEvent {
> +       u32     tagged_event_id;
> +       u32     tagged_event_data_size;
> +       u8      tagged_event_data[];
> +} TCG_PCClientTaggedEvent;
>
>  typedef struct efi_tcg2_event efi_tcg2_event_t;
> -typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
>  typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
>
>  union efi_tcg2_protocol {
> --
> 2.44.0.278.ge034bb2e1d-goog
>

On Fri, 8 Mar 2024 at 11:43, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 8 Mar 2024 at 10:38, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Ard
> >
> > On Fri, 8 Mar 2024 at 10:58, Ard Biesheuvel <ardb+git@google.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Our efi_tcg2_tagged_event is not defined in the EFI spec, but it is not
> > > a local invention either: it was taken from the TCG PC Client spec,
> > > where it is called TCG_PCClientTaggedEvent.
> > >
> > > This spec also contains some guidance on how to populate it, which
> > > is not being followed closely at the moment; the event size should cover
> > > the TCG_PCClientTaggedEvent and its payload only, but it currently
> > > covers the preceding efi_tcg2_event too, and this may result in trailing
> > > garbage being measured into the TPM.
> >
> > I think there's a confusion here and the current code we have is correct.
> > The EFI TCG spec [0] says that the tdEFI_TCG2_EVENT size is:
> > "Total size of the event including the Size component, the header and the
> > Event data." which obviously contradicts the definition of the tagged
> > event in the PC client spec.
> > But given the fact that TCG_PCClientTaggedEvent has its own size field
> > I think we should use what we already have.
> >
> >
> > [0] https://trustedcomputinggroup.org/wp-content/uploads/EFI-Protocol-Specification-rev13-160330final.pdf
> > page 33
> >
>
> Fair enough.
>
> It is rather disappointing that the TCG2 specs contradict each other,

Indeed

> but a quick inspection of the EDK2 implementation shows that it
> follows your interpretation.
>
> For example, in Tcg2HashLogExtendEvent()
> [SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.c], we have a check
>
> if (Event->Size < Event->Header.HeaderSize + sizeof (UINT32)) {
>   return EFI_INVALID_PARAMETER;
> }
>
> which ensures that the event size covers at least the EFI_TCG2_EVENT,
> which obviously implies that 'size' is expected to include it.
>

FWIW the same principle applies in the u-boot implementation as well.
As far as this series is concerned, keeping the
TCG_PCClientTaggedEvent will make reading the code & spec in parallel
easier, but I don't have any strong opinions -- I am fine with both

Cheers
/Ilias

> So please disregard this series