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

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

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;
>  

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer