Re: [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 3/5/24 12:21 AM, Ard Biesheuvel wrote:
> On Tue, 5 Mar 2024 at 05:30, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 3/4/24 2:44 AM, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> In spite of the efi_ prefix, struct efi_tcg2_tagged_event is specific to
>>> the EFI stub, and so we can tweak it to our liking if needed, e.g., to
>>> accommodate the TDX variant of the TCG2 measurement protocol.
>>>
>>> In preparation for that, get rid of it entirely, and combine it with the
>>> efi_measured_event struct used by the measurement code.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---

With nits fixed,

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>>>  drivers/firmware/efi/libstub/efi-stub-helper.c | 26 ++++++++------------
>>>  drivers/firmware/efi/libstub/efistub.h         | 18 ++++++++------
>>>  2 files changed, 21 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> index bfa30625f5d0..0dbc9d3f4abd 100644
>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> @@ -193,7 +193,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
>>>       *load_options_size = load_option_unpacked.optional_data_size;
>>>  }
>>>
>>> -enum efistub_event {
>>> +enum efistub_event_type {
>>>       EFISTUB_EVT_INITRD,
>>>       EFISTUB_EVT_LOAD_OPTIONS,
>>>       EFISTUB_EVT_COUNT,
>>> @@ -221,44 +221,38 @@ static const struct {
>>>
>>>  static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>>>                                            unsigned long load_size,
>>> -                                          enum efistub_event event)
>>> +                                          enum efistub_event_type event)
>>>  {
>>> +     struct efistub_measured_event *evt;
>>> +     int size = struct_size(evt, tagged_event_data,
>>> +                            events[event].event_data_len);
>> Include linux/overflow.h explicitly?
>>
> Yes, good point.
>
>>>       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);
>> It looks like in patch 3 you have converted evt as stack variable. Since that
>> change is not specific to CC fallback, can it be moved here?
>>
> Not sure what you mean here. evt is still there after parch #3

Sorry, it looks like I misread the patch # 3. Please ignore this comment.

>
>>>               if (status != EFI_SUCCESS)
>>>                       goto fail;
>>>
>>> -             evt->event_data = (struct efi_tcg2_event){
>>> +             evt->event_data.tcg2_data = (struct efi_tcg2_event){
>>>                       .event_size                     = size,
>>> -                     .event_header.header_size       = sizeof(evt->event_data.event_header),
>>> +                     .event_header.header_size       = sizeof(evt->event_data.tcg2_data.event_header),
>>>                       .event_header.header_version    = EFI_TCG2_EVENT_HEADER_VERSION,
>>>                       .event_header.pcr_index         = events[event].pcr_index,
>>>                       .event_header.event_type        = EV_EVENT_TAG,
>>>               };
>>>
>>> -             evt->tagged_event = (struct efi_tcg2_tagged_event){
>>> -                     .tagged_event_id                = events[event].event_id,
>>> -                     .tagged_event_data_size         = events[event].event_data_len,
>>> -             };
>>> +             evt->tagged_event_id            = events[event].event_id;
>>> +             evt->tagged_event_data_size     = events[event].event_data_len;
>>>
>>>               memcpy(evt->tagged_event_data, events[event].event_data,
>>>                      events[event].event_data_len);
>>>
>>>               status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>>> -                                     load_addr, load_size, &evt->event_data);
>>> +                                     load_addr, load_size, &evt->event_data.tcg2_data);
>>>               efi_bs_call(free_pool, evt);
>>>
>>>               if (status != EFI_SUCCESS)
>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>>> index c04b82ea40f2..b2c50dce48b8 100644
>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>> @@ -843,14 +843,7 @@ 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;
>>> -
>>>  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 {
>>> @@ -882,6 +875,17 @@ union efi_tcg2_protocol {
>>>       } mixed_mode;
>>>  };
>>>
>>> +union efistub_event {
>>> +     efi_tcg2_event_t        tcg2_data;
>>> +};
>>> +
>>> +struct efistub_measured_event {
>>> +     union efistub_event     event_data;
>>> +     u32                     tagged_event_id;
>>> +     u32                     tagged_event_data_size;
>>> +     u8                      tagged_event_data[];
>>> +} __packed;
>>> +
>> Since efistub_measured_event is only used efi-stub-helper.c, why
>> not leave it there?
>>
> Indeed. I will move it back.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer