Hi Ilias, Thanks for the review. On 2/18/24 10:38 PM, Ilias Apalodimas wrote: > Hi Kuppuswamy, > > On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote: >> If the virtual firmware implements TPM support, TCG2 protocol will be >> used for kernel measurements and event logging support. But in CC >> environment, not all platforms support or enable the TPM feature. UEFI >> specification [1] exposes protocol and interfaces used for kernel >> measurements in CC platforms without TPM support. >> >> Currently, the efi-stub only supports the kernel related measurements >> for the platform that supports TCG2 protocol. So, extend it add >> CC measurement protocol (EFI_CC_MEASUREMENT_PROTOCOL) and event logging >> support. Event logging format in the CC environment is the same as >> TCG2. >> >> More details about the EFI CC measurements and logging can be found >> in [1]. >> >> Link: https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#efi-cc-measurement-protocol [1] >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> >> --- >> >> Changes since v1: >> * Fixed missing tagged event data. >> >> .../firmware/efi/libstub/efi-stub-helper.c | 127 ++++++++++++++---- >> drivers/firmware/efi/libstub/efistub.h | 74 ++++++++++ >> include/linux/efi.h | 1 + >> 3 files changed, 174 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c >> index bfa30625f5d0..cc31f8143190 100644 >> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >> @@ -219,50 +219,121 @@ static const struct { >> }, >> }; >> >> +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_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; > This is defined as size_t on the cc variant. I guess both are ok, just pick one Ok >> + efi_status_t status; >> + >> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, >> + (void **)&evt); >> + if (status != EFI_SUCCESS) > pr_err() here as done in the cc variant? I will remove the error message in the CC variant as well. I don't want to introduce additional logs for existing case (tcg2) as well. May be I can can use efi_debug just for the map_pcr_to_mr_index call. >> + 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; +} > >> + 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
