Received-SPF: pass (google.com: domain of linux-kernel+bounces-90448-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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> <CAC_iWjK3sXq1O4tgR0vEr9n2erfrr+9hqU+xUqiMK-TQa0t-hg@mail.gmail.com>
In-Reply-To: <CAC_iWjK3sXq1O4tgR0vEr9n2erfrr+9hqU+xUqiMK-TQa0t-hg@mail.gmail.com>
From: Ard Biesheuvel <ardb@kernel.org>
Date: Mon, 4 Mar 2024 11:41:50 +0100
Message-ID: <CAMj1kXHHWMhasBPpxWeAJwgQOBX2U0DuW=ADxGH+ppYeCQT15A@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC)
 measurement support
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>, 
	linux-kernel@vger.kernel.org, linux-efi@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"

On Tue, 27 Feb 2024 at 14:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> 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?
>

I'd prefer to radically unify this code much further.

AFAICT, the *only* difference is the need to call
map_pcr_to_mr_index(), beyond that, everything is the same:
- efi_tcg2_event is identical to efi_cc_event
- the hash_log_extend_event() protocol member lives at the same offset
in the protocol struct, and has the same prototype

If we weren't as far along in the merge window, I'd ask you to respin
with this in mind. However, we're at -rc7 and so to avoid missing the
merge window, I went ahead and reworked the code. I'll send those out
momentarily.