| Message ID | 20240304104409.2326422-7-ardb+git@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | efi/libstub: Fall back to CC proto for measurement | expand |
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> > --- > 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? > 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? > 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? > struct riscv_efi_boot_protocol { > u64 revision; >
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> > > --- > > 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 > > 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.
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.
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); 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) 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; + struct riscv_efi_boot_protocol { u64 revision;