Received-SPF: pass (google.com: domain of linux-kernel+bounces-83309-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240215030002.281456-1-sathyanarayanan.kuppuswamy@linux.intel.com>
 <20240215030002.281456-2-sathyanarayanan.kuppuswamy@linux.intel.com>
 <CAC_iWj+p6gmcb4-_-f8Kr4Ds6EmSsq2D4GK1XymyybX7i1TMFw@mail.gmail.com> <7bd53417-af59-43a8-965e-f63dfc827f3c@linux.intel.com>
In-Reply-To: <7bd53417-af59-43a8-965e-f63dfc827f3c@linux.intel.com>
From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Date: Tue, 27 Feb 2024 15:22:57 +0200
Message-ID: <CAC_iWjK3sXq1O4tgR0vEr9n2erfrr+9hqU+xUqiMK-TQa0t-hg@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC)
 measurement support
To: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Ard Biesheuvel <ardb@kernel.org>, linux-kernel@vger.kernel.org, 
	linux-efi@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

Hi,

Thanks for taking a shot at this.

[...]

> >> +               return status;
> >> +
> >> +       evt->event_data = (struct efi_tcg2_event){
> >> +               .event_size                     = size,
> >> +               .event_header.header_size       = sizeof(evt->event_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,
> >> +       };
> >> +
> >> +       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);
> > The struct filling/memcpying looks similar across the 2 functions.  I
> > wonder if it makes sense to have a common function for that, with an
> > argument for the event data type.
>
> If we want to use helper function, the updated code looks like below.
>
> Are you fine with this version? (compile-tested only)
>
> +struct efi_tcg2_measured_event {
> +       efi_tcg2_event_t        event_data;
> +       efi_tcg2_tagged_event_t tagged_event;
> +       u8                      tagged_event_data[];
> +};
> +
> +struct efi_cc_measured_event {
> +       efi_cc_event_t  event_data;
> +       efi_tcg2_tagged_event_t tagged_event;
> +       u8                      tagged_event_data[];
> +};
> +
> +static void efi_tcg2_event_init(struct efi_tcg2_measured_event *evt,
> +                               size_t size,
> +                               enum efistub_event event)
> +{
> +       evt->event_data = (struct efi_tcg2_event){
> +               .event_size                     = size,
> +               .event_header.header_size       = sizeof(evt->event_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,
> +       };
> +
> +       memcpy(evt->tagged_event_data, events[event].event_data,
> +              events[event].event_data_len);
> +}
> +
> +static efi_status_t tcg2_efi_measure(efi_tcg2_protocol_t *tcg2,
> +                                    unsigned long load_addr,
> +                                    unsigned long load_size,
> +                                    enum efistub_event event)
> +{
> +       struct efi_tcg2_measured_event *evt;
> +       efi_status_t status;
> +       size_t size;
> +
> +       size = sizeof(*evt) + events[event].event_data_len;
> +
> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> +                            (void **)&evt);
> +       if (status != EFI_SUCCESS)
> +               return status;
> +
> +       efi_tcg2_event_init(evt, size, event);
> +
> +       status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> +                               load_addr, load_size, &evt->event_data);
> +       efi_bs_call(free_pool, evt);
> +
> +       return status;
> +}
>
> +
> +static efi_status_t cc_efi_measure(efi_cc_protocol_t *cc,
> +                                  unsigned long load_addr,
> +                                  unsigned long load_size,
> +                                  enum efistub_event event)
> +{
> +       struct efi_cc_measured_event *evt;
> +       efi_cc_mr_index_t mr;
> +       efi_status_t status;
> +       size_t size;
> +
> +       status = efi_call_proto(cc, map_pcr_to_mr_index, events[event].pcr_index, &mr);
> +       if (status != EFI_SUCCESS) {
> +               efi_debug("CC_MEASURE: PCR to MR mapping failed\n");
> +               return status;
> +       }
> +
> +       size = sizeof(*evt) + events[event].event_data_len;
> +
> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> +       if (status != EFI_SUCCESS)
> +               return status;
> +
> +       efi_tcg2_event_init((struct efi_tcg2_measured_event *)evt, size, event);
> +
> +       evt->event_data = (struct efi_cc_event){
> +               .event_header.header_size       = sizeof(evt->event_data.event_header),
> +               .event_header.header_version    = EFI_CC_EVENT_HEADER_VERSION,
> +               .event_header.mr_index          = mr,
> +       };
> +
> +       status = efi_call_proto(cc, hash_log_extend_event, 0,
> +                               load_addr, load_size, &evt->event_data);
> +
> +       efi_bs_call(free_pool, evt);
> +
> +       return status;
> +}
>

Yes, I think looks cleaner. Ard thoughts?

Thanks
/Ilias
>
> >
> >> +       efi_bs_call(free_pool, evt);
> >> +
> >> +       return status;
> >> +}
> >> +
> >> +static efi_status_t cc_efi_measure(efi_cc_protocol_t *cc,
> >> +                                  unsigned long load_addr,
> >> +                                  unsigned long load_size,
> >> +                                  enum efistub_event event)
> >> +{
> >> +       struct efi_measured_event {
> >> +               efi_cc_event_t  event_data;
> >> +               efi_tcg2_tagged_event_t tagged_event;
> >> +               u8                      tagged_event_data[];
> >> +       } *evt;
> >> +       size_t size = sizeof(*evt) + events[event].event_data_len;
> >> +       efi_cc_mr_index_t mr;
> >> +       efi_status_t status;
> >> +
> >> +       status = efi_call_proto(cc, map_pcr_to_mr_index, events[event].pcr_index, &mr);
> >> +       if (status != EFI_SUCCESS) {
> >> +               efi_err("CC_MEASURE: PCR to MR mapping failed\n");
> >> +               return status;
> >> +       }
> >> +
> >> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> >> +       if (status != EFI_SUCCESS) {
> >> +               efi_err("CC_MEASURE: Allocating event struct failed\n");
> >> +               return status;
> >> +       }
> >> +
> >> +       evt->event_data = (struct efi_cc_event){
> >> +               .event_size                     = size,
> >> +               .event_header.header_size       = sizeof(evt->event_data.event_header),
> >> +               .event_header.header_version    = EFI_CC_EVENT_HEADER_VERSION,
> >> +               .event_header.mr_index          = mr,
> >> +               .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,
> >> +       };
> >> +
> >> +       memcpy(evt->tagged_event_data, events[event].event_data,
> >> +              events[event].event_data_len);
> >> +
> >> +       status = efi_call_proto(cc, hash_log_extend_event, 0,
> >> +                               load_addr, load_size, &evt->event_data);
> >> +
> >> +       efi_bs_call(free_pool, evt);
> >> +
> >> +       return status;
> >> +}
> >>  static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> >>                                              unsigned long load_size,
> >>                                              enum efistub_event event)
> >>  {
> >>         efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> >> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> >> +       efi_cc_protocol_t *cc = NULL;
> >>         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);
> >> +               status = tcg2_efi_measure(tcg2, load_addr, load_size, event);
> >>                 if (status != EFI_SUCCESS)
> >>                         goto fail;
> >>
> >> -               evt->event_data = (struct efi_tcg2_event){
> >> -                       .event_size                     = size,
> >> -                       .event_header.header_size       = sizeof(evt->event_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,
> >> -               };
> >> -
> >> -               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);
> >> -               efi_bs_call(free_pool, evt);
> >> +               return EFI_SUCCESS;
> >> +       }
> >>
> >> +       efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> >> +       if (cc) {
> >> +               status = cc_efi_measure(cc, load_addr, load_size, event);
> >>                 if (status != EFI_SUCCESS)
> >>                         goto fail;
> >> +
> >>                 return EFI_SUCCESS;
> >>         }
> >>
> > [...]
> >
> > Thanks
> > /Ilias
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>