| Message ID | 20240304104409.2326422-9-ardb+git@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | efi/libstub: Fall back to CC proto for measurement | expand |
On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > From: Ard Biesheuvel <ardb@kernel.org> > > To accommodate confidential compute VMs that expose the simplified CC > measurement protocol instead of the full-blown TCG2 one, fall back to > the former if the latter does not exist. > > The CC protocol was designed to be used in this manner, which is why the > types and prototypes have been kept the same where possible. So reuse > the existing code, and only deviate from the TCG2 code path where > needed. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > drivers/firmware/efi/libstub/efistub.h | 3 + > 2 files changed, 53 insertions(+), 17 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index 0dbc9d3f4abd..21f4567324f6 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > unsigned long load_size, > enum efistub_event_type event) > { > + union { > + efi_status_t > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > + u64, const union efistub_event *); > + struct { u32 hash_log_extend_event; } mixed_mode; > + } method; > 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; > + union efistub_event ev; > efi_status_t status; > + void *protocol; > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > if (tcg2) { > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > - (void **)&evt); > - if (status != EFI_SUCCESS) > - goto fail; > - > - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > + ev.tcg2_data = (struct efi_tcg2_event){ > .event_size = size, > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > + .event_header.header_size = sizeof(ev.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, > }; > + protocol = tcg2; > + method.hash_log_extend_event = > + (void *)efi_table_attr(tcg2, hash_log_extend_event); > + } else { +Qinkun Bao Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the TCG protocol breaks backwards compatibility, it'd be preferable to measure into all the measurement protocols that are present. The UEFI v2.10 standard says that firmware "should not" provide both, but it is not MUST NOT. Given this and our desire to provide service continuity, I ask that you remove the "else" guard. > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > + efi_cc_protocol_t *cc = NULL; > > - 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); > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > + if (!cc) > + return EFI_UNSUPPORTED; > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > - load_addr, load_size, &evt->event_data.tcg2_data); > - efi_bs_call(free_pool, evt); > + ev.cc_data = (struct efi_cc_event){ > + .event_size = size, > + .event_header.header_size = sizeof(ev.cc_data.event_header), > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > + .event_header.event_type = EV_EVENT_TAG, > + }; > > + status = efi_call_proto(cc, map_pcr_to_mr_index, > + events[event].pcr_index, > + &ev.cc_data.event_header.mr_index); > if (status != EFI_SUCCESS) > goto fail; > - return EFI_SUCCESS; > + > + protocol = cc; > + method.hash_log_extend_event = > + (void *)efi_table_attr(cc, hash_log_extend_event); > } > > - return EFI_UNSUPPORTED; > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > + if (status != EFI_SUCCESS) > + goto fail; > + > + evt->event_data = ev; > + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > + load_addr, load_size, &evt->event_data); > + efi_bs_call(free_pool, evt); > + > + if (status == EFI_SUCCESS) > + return EFI_SUCCESS; > + > fail: > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > return status; > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index d621bfb719c4..4bf9a76796b7 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -954,8 +954,11 @@ union efi_cc_protocol { > } mixed_mode; > }; > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > + > union efistub_event { > efi_tcg2_event_t tcg2_data; > + efi_cc_event_t cc_data; > }; > > struct efistub_measured_event { > -- > 2.44.0.278.ge034bb2e1d-goog > >
On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > To accommodate confidential compute VMs that expose the simplified CC > > measurement protocol instead of the full-blown TCG2 one, fall back to > > the former if the latter does not exist. > > > > The CC protocol was designed to be used in this manner, which is why the > > types and prototypes have been kept the same where possible. So reuse > > the existing code, and only deviate from the TCG2 code path where > > needed. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > > drivers/firmware/efi/libstub/efistub.h | 3 + > > 2 files changed, 53 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > index 0dbc9d3f4abd..21f4567324f6 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > > unsigned long load_size, > > enum efistub_event_type event) > > { > > + union { > > + efi_status_t > > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > > + u64, const union efistub_event *); > > + struct { u32 hash_log_extend_event; } mixed_mode; > > + } method; > > 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; > > + union efistub_event ev; > > efi_status_t status; > > + void *protocol; > > > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > > if (tcg2) { > > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > > - (void **)&evt); > > - if (status != EFI_SUCCESS) > > - goto fail; > > - > > - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > > + ev.tcg2_data = (struct efi_tcg2_event){ > > .event_size = size, > > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > > + .event_header.header_size = sizeof(ev.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, > > }; > > + protocol = tcg2; > > + method.hash_log_extend_event = > > + (void *)efi_table_attr(tcg2, hash_log_extend_event); > > + } else { > > +Qinkun Bao > Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > TCG protocol breaks backwards compatibility, it'd be preferable to > measure into all the measurement protocols that are present. How so? Older kernels will use TCG2 if it exists, and so will new kernels. The only difference is that on new kernels, the CC protocol will be used in case TCG2 is not implemented. So the only affected scenario here is a system that today implements TCG but not CC, and intends to implement CC later and receive measurements into both protocols. Does that really qualify as backward compatibility? I'd rather not accommodate future systems that implement something that the UEFI spec says they should not. > The UEFI > v2.10 standard says that firmware "should not" provide both, but it is > not MUST NOT. Given this and our desire to provide service continuity, > I ask that you remove the "else" guard. > Ignoring the newer protocol if the established one exists is an excellent way of making sure this does not happen. > > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > > + efi_cc_protocol_t *cc = NULL; > > > > - 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); > > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > > + if (!cc) > > + return EFI_UNSUPPORTED; > > > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > > - load_addr, load_size, &evt->event_data.tcg2_data); > > - efi_bs_call(free_pool, evt); > > + ev.cc_data = (struct efi_cc_event){ > > + .event_size = size, > > + .event_header.header_size = sizeof(ev.cc_data.event_header), > > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > > + .event_header.event_type = EV_EVENT_TAG, > > + }; > > > > + status = efi_call_proto(cc, map_pcr_to_mr_index, > > + events[event].pcr_index, > > + &ev.cc_data.event_header.mr_index); > > if (status != EFI_SUCCESS) > > goto fail; > > - return EFI_SUCCESS; > > + > > + protocol = cc; > > + method.hash_log_extend_event = > > + (void *)efi_table_attr(cc, hash_log_extend_event); > > } > > > > - return EFI_UNSUPPORTED; > > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > > + if (status != EFI_SUCCESS) > > + goto fail; > > + > > + evt->event_data = ev; > > + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > > + load_addr, load_size, &evt->event_data); > > + efi_bs_call(free_pool, evt); > > + > > + if (status == EFI_SUCCESS) > > + return EFI_SUCCESS; > > + > > fail: > > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > > return status; > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > > index d621bfb719c4..4bf9a76796b7 100644 > > --- a/drivers/firmware/efi/libstub/efistub.h > > +++ b/drivers/firmware/efi/libstub/efistub.h > > @@ -954,8 +954,11 @@ union efi_cc_protocol { > > } mixed_mode; > > }; > > > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > > + > > union efistub_event { > > efi_tcg2_event_t tcg2_data; > > + efi_cc_event_t cc_data; > > }; > > > > struct efistub_measured_event { > > -- > > 2.44.0.278.ge034bb2e1d-goog > > > > > > > -- > -Dionna Glaze, PhD (she/her)
On Tue, 5 Mar 2024 at 19:47, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > > > On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > To accommodate confidential compute VMs that expose the simplified CC > > > measurement protocol instead of the full-blown TCG2 one, fall back to > > > the former if the latter does not exist. > > > > > > The CC protocol was designed to be used in this manner, which is why the > > > types and prototypes have been kept the same where possible. So reuse > > > the existing code, and only deviate from the TCG2 code path where > > > needed. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > > > drivers/firmware/efi/libstub/efistub.h | 3 + > > > 2 files changed, 53 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > index 0dbc9d3f4abd..21f4567324f6 100644 > > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > > > unsigned long load_size, > > > enum efistub_event_type event) > > > { > > > + union { > > > + efi_status_t > > > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > > > + u64, const union efistub_event *); > > > + struct { u32 hash_log_extend_event; } mixed_mode; > > > + } method; > > > 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; > > > + union efistub_event ev; > > > efi_status_t status; > > > + void *protocol; > > > > > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > > > if (tcg2) { > > > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > > > - (void **)&evt); > > > - if (status != EFI_SUCCESS) > > > - goto fail; > > > - > > > - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > > > + ev.tcg2_data = (struct efi_tcg2_event){ > > > .event_size = size, > > > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > > > + .event_header.header_size = sizeof(ev.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, > > > }; > > > + protocol = tcg2; > > > + method.hash_log_extend_event = > > > + (void *)efi_table_attr(tcg2, hash_log_extend_event); > > > + } else { > > > > +Qinkun Bao > > Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > > TCG protocol breaks backwards compatibility, it'd be preferable to > > measure into all the measurement protocols that are present. > > How so? Older kernels will use TCG2 if it exists, and so will new > kernels. The only difference is that on new kernels, the CC protocol > will be used in case TCG2 is not implemented. > > So the only affected scenario here is a system that today implements > TCG but not CC, and intends to implement CC later and receive > measurements into both protocols. Does that really qualify as backward > compatibility? I'd rather not accommodate future systems that > implement something that the UEFI spec says they should not. > > > The UEFI > > v2.10 standard says that firmware "should not" provide both, but it is > > not MUST NOT. Given this and our desire to provide service continuity, > > I ask that you remove the "else" guard. > > > > Ignoring the newer protocol if the established one exists is an > excellent way of making sure this does not happen. Apart from what the EFI spec says, why would anyone want to use both protocols? Won't we end up measuring things 2x ? Thanks /Ilias > > > > > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > > > + efi_cc_protocol_t *cc = NULL; > > > > > > - 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); > > > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > > > + if (!cc) > > > + return EFI_UNSUPPORTED; > > > > > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > > > - load_addr, load_size, &evt->event_data.tcg2_data); > > > - efi_bs_call(free_pool, evt); > > > + ev.cc_data = (struct efi_cc_event){ > > > + .event_size = size, > > > + .event_header.header_size = sizeof(ev.cc_data.event_header), > > > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > > > + .event_header.event_type = EV_EVENT_TAG, > > > + }; > > > > > > + status = efi_call_proto(cc, map_pcr_to_mr_index, > > > + events[event].pcr_index, > > > + &ev.cc_data.event_header.mr_index); > > > if (status != EFI_SUCCESS) > > > goto fail; > > > - return EFI_SUCCESS; > > > + > > > + protocol = cc; > > > + method.hash_log_extend_event = > > > + (void *)efi_table_attr(cc, hash_log_extend_event); > > > } > > > > > > - return EFI_UNSUPPORTED; > > > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > > > + if (status != EFI_SUCCESS) > > > + goto fail; > > > + > > > + evt->event_data = ev; > > > + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > > > + load_addr, load_size, &evt->event_data); > > > + efi_bs_call(free_pool, evt); > > > + > > > + if (status == EFI_SUCCESS) > > > + return EFI_SUCCESS; > > > + > > > fail: > > > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > > > return status; > > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > > > index d621bfb719c4..4bf9a76796b7 100644 > > > --- a/drivers/firmware/efi/libstub/efistub.h > > > +++ b/drivers/firmware/efi/libstub/efistub.h > > > @@ -954,8 +954,11 @@ union efi_cc_protocol { > > > } mixed_mode; > > > }; > > > > > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > > > + > > > union efistub_event { > > > efi_tcg2_event_t tcg2_data; > > > + efi_cc_event_t cc_data; > > > }; > > > > > > struct efistub_measured_event { > > > -- > > > 2.44.0.278.ge034bb2e1d-goog > > > > > > > > > > > > -- > > -Dionna Glaze, PhD (she/her) >
On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > > > On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > To accommodate confidential compute VMs that expose the simplified CC > > > measurement protocol instead of the full-blown TCG2 one, fall back to > > > the former if the latter does not exist. > > > > > > The CC protocol was designed to be used in this manner, which is why the > > > types and prototypes have been kept the same where possible. So reuse > > > the existing code, and only deviate from the TCG2 code path where > > > needed. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > > > drivers/firmware/efi/libstub/efistub.h | 3 + > > > 2 files changed, 53 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > index 0dbc9d3f4abd..21f4567324f6 100644 > > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > > > unsigned long load_size, > > > enum efistub_event_type event) > > > { > > > + union { > > > + efi_status_t > > > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > > > + u64, const union efistub_event *); > > > + struct { u32 hash_log_extend_event; } mixed_mode; > > > + } method; > > > 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; > > > + union efistub_event ev; > > > efi_status_t status; > > > + void *protocol; > > > > > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > > > if (tcg2) { > > > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > > > - (void **)&evt); > > > - if (status != EFI_SUCCESS) > > > - goto fail; > > > - > > > - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > > > + ev.tcg2_data = (struct efi_tcg2_event){ > > > .event_size = size, > > > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > > > + .event_header.header_size = sizeof(ev.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, > > > }; > > > + protocol = tcg2; > > > + method.hash_log_extend_event = > > > + (void *)efi_table_attr(tcg2, hash_log_extend_event); > > > + } else { > > > > +Qinkun Bao > > Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > > TCG protocol breaks backwards compatibility, it'd be preferable to > > measure into all the measurement protocols that are present. > > How so? Older kernels will use TCG2 if it exists, and so will new > kernels. The only difference is that on new kernels, the CC protocol > will be used in case TCG2 is not implemented. > > So the only affected scenario here is a system that today implements > TCG but not CC, and intends to implement CC later and receive > measurements into both protocols. Does that really qualify as backward > compatibility? I'd rather not accommodate future systems that > implement something that the UEFI spec says they should not. > > > The UEFI > > v2.10 standard says that firmware "should not" provide both, but it is > > not MUST NOT. Given this and our desire to provide service continuity, > > I ask that you remove the "else" guard. > > > > Ignoring the newer protocol if the established one exists is an > excellent way of making sure this does not happen. > The problem is that the protocols are not equivalent, and we disagree with the standard's claim of "should not" to the point that we're taking it to the USWG. The "should not" advisement is predicated on not trusting boot layers to use both protocols when they're both present, such that you could accidentally miss measuring a security-critical event. That's a strawman though, since you already need to develop trust in those boot layers. We have software supply chain endorsements for tracking that kind of property for use in attestation verification. The CC protocol is useful for hardware-rooted boot measurement, but it does nothing about the rest of TPM 2.0. There are plenty of users that want to use a vTPM that's hosted by the VMM but also get an extra integrity assurance that measurements into TDX RTMRs and attested by an Intel-rooted key pass an extra level of scrutiny. > > > > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > > > + efi_cc_protocol_t *cc = NULL; > > > > > > - 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); > > > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > > > + if (!cc) > > > + return EFI_UNSUPPORTED; > > > > > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > > > - load_addr, load_size, &evt->event_data.tcg2_data); > > > - efi_bs_call(free_pool, evt); > > > + ev.cc_data = (struct efi_cc_event){ > > > + .event_size = size, > > > + .event_header.header_size = sizeof(ev.cc_data.event_header), > > > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > > > + .event_header.event_type = EV_EVENT_TAG, > > > + }; > > > > > > + status = efi_call_proto(cc, map_pcr_to_mr_index, > > > + events[event].pcr_index, > > > + &ev.cc_data.event_header.mr_index); > > > if (status != EFI_SUCCESS) > > > goto fail; > > > - return EFI_SUCCESS; > > > + > > > + protocol = cc; > > > + method.hash_log_extend_event = > > > + (void *)efi_table_attr(cc, hash_log_extend_event); > > > } > > > > > > - return EFI_UNSUPPORTED; > > > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > > > + if (status != EFI_SUCCESS) > > > + goto fail; > > > + > > > + evt->event_data = ev; > > > + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > > > + load_addr, load_size, &evt->event_data); > > > + efi_bs_call(free_pool, evt); > > > + > > > + if (status == EFI_SUCCESS) > > > + return EFI_SUCCESS; > > > + > > > fail: > > > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > > > return status; > > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > > > index d621bfb719c4..4bf9a76796b7 100644 > > > --- a/drivers/firmware/efi/libstub/efistub.h > > > +++ b/drivers/firmware/efi/libstub/efistub.h > > > @@ -954,8 +954,11 @@ union efi_cc_protocol { > > > } mixed_mode; > > > }; > > > > > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > > > + > > > union efistub_event { > > > efi_tcg2_event_t tcg2_data; > > > + efi_cc_event_t cc_data; > > > }; > > > > > > struct efistub_measured_event { > > > -- > > > 2.44.0.278.ge034bb2e1d-goog > > > > > > > > > > > > -- > > -Dionna Glaze, PhD (she/her)
On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote: > On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: >> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: >>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: >>>> From: Ard Biesheuvel <ardb@kernel.org> >>>> >>>> To accommodate confidential compute VMs that expose the simplified CC >>>> measurement protocol instead of the full-blown TCG2 one, fall back to >>>> the former if the latter does not exist. >>>> >>>> The CC protocol was designed to be used in this manner, which is why the >>>> types and prototypes have been kept the same where possible. So reuse >>>> the existing code, and only deviate from the TCG2 code path where >>>> needed. >>>> >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >>>> --- >>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- >>>> drivers/firmware/efi/libstub/efistub.h | 3 + >>>> 2 files changed, 53 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c >>>> index 0dbc9d3f4abd..21f4567324f6 100644 >>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, >>>> unsigned long load_size, >>>> enum efistub_event_type event) >>>> { >>>> + union { >>>> + efi_status_t >>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, >>>> + u64, const union efistub_event *); >>>> + struct { u32 hash_log_extend_event; } mixed_mode; >>>> + } method; >>>> 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; >>>> + union efistub_event ev; >>>> efi_status_t status; >>>> + void *protocol; >>>> >>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); >>>> if (tcg2) { >>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, >>>> - (void **)&evt); >>>> - if (status != EFI_SUCCESS) >>>> - goto fail; >>>> - >>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){ >>>> + ev.tcg2_data = (struct efi_tcg2_event){ >>>> .event_size = size, >>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), >>>> + .event_header.header_size = sizeof(ev.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, >>>> }; >>>> + protocol = tcg2; >>>> + method.hash_log_extend_event = >>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event); >>>> + } else { >>> +Qinkun Bao >>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the >>> TCG protocol breaks backwards compatibility, it'd be preferable to >>> measure into all the measurement protocols that are present. >> How so? Older kernels will use TCG2 if it exists, and so will new >> kernels. The only difference is that on new kernels, the CC protocol >> will be used in case TCG2 is not implemented. >> >> So the only affected scenario here is a system that today implements >> TCG but not CC, and intends to implement CC later and receive >> measurements into both protocols. Does that really qualify as backward >> compatibility? I'd rather not accommodate future systems that >> implement something that the UEFI spec says they should not. >> >>> The UEFI >>> v2.10 standard says that firmware "should not" provide both, but it is >>> not MUST NOT. Given this and our desire to provide service continuity, >>> I ask that you remove the "else" guard. >>> >> Ignoring the newer protocol if the established one exists is an >> excellent way of making sure this does not happen. >> > The problem is that the protocols are not equivalent, and we disagree > with the standard's claim of "should not" to the point that we're > taking it to the USWG. The "should not" advisement is predicated on > not trusting boot layers to use both protocols when they're both > present, such that you could accidentally miss measuring a > security-critical event. That's a strawman though, since you already > need to develop trust in those boot layers. We have software supply > chain endorsements for tracking that kind of property for use in > attestation verification. > > The CC protocol is useful for hardware-rooted boot measurement, but it > does nothing about the rest of TPM 2.0. There are plenty of users that > want to use a vTPM that's hosted by the VMM but also get an extra > integrity assurance that measurements into TDX RTMRs and attested by > an Intel-rooted key pass an extra level of scrutiny. > If you check the EDK2 part of this support, it also uses if else model. It does not measure both. If there a complete vTPM support, why can't user trust measurements from it? I think the CC vendors will ensure their vTPM implementation is protected from attack from the host (like implementing it part of firmware or launching it as service in a separate VM). >>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; >>>> + efi_cc_protocol_t *cc = NULL; >>>> >>>> - 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); >>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); >>>> + if (!cc) >>>> + return EFI_UNSUPPORTED; >>>> >>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0, >>>> - load_addr, load_size, &evt->event_data.tcg2_data); >>>> - efi_bs_call(free_pool, evt); >>>> + ev.cc_data = (struct efi_cc_event){ >>>> + .event_size = size, >>>> + .event_header.header_size = sizeof(ev.cc_data.event_header), >>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, >>>> + .event_header.event_type = EV_EVENT_TAG, >>>> + }; >>>> >>>> + status = efi_call_proto(cc, map_pcr_to_mr_index, >>>> + events[event].pcr_index, >>>> + &ev.cc_data.event_header.mr_index); >>>> if (status != EFI_SUCCESS) >>>> goto fail; >>>> - return EFI_SUCCESS; >>>> + >>>> + protocol = cc; >>>> + method.hash_log_extend_event = >>>> + (void *)efi_table_attr(cc, hash_log_extend_event); >>>> } >>>> >>>> - return EFI_UNSUPPORTED; >>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); >>>> + if (status != EFI_SUCCESS) >>>> + goto fail; >>>> + >>>> + evt->event_data = ev; >>>> + 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_fn_call(&method, hash_log_extend_event, protocol, 0, >>>> + load_addr, load_size, &evt->event_data); >>>> + efi_bs_call(free_pool, evt); >>>> + >>>> + if (status == EFI_SUCCESS) >>>> + return EFI_SUCCESS; >>>> + >>>> fail: >>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); >>>> return status; >>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h >>>> index d621bfb719c4..4bf9a76796b7 100644 >>>> --- a/drivers/firmware/efi/libstub/efistub.h >>>> +++ b/drivers/firmware/efi/libstub/efistub.h >>>> @@ -954,8 +954,11 @@ union efi_cc_protocol { >>>> } mixed_mode; >>>> }; >>>> >>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); >>>> + >>>> union efistub_event { >>>> efi_tcg2_event_t tcg2_data; >>>> + efi_cc_event_t cc_data; >>>> }; >>>> >>>> struct efistub_measured_event { >>>> -- >>>> 2.44.0.278.ge034bb2e1d-goog >>>> >>>> >>> >>> -- >>> -Dionna Glaze, PhD (she/her) > >
On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote: > > On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: > >> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > >>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > >>>> From: Ard Biesheuvel <ardb@kernel.org> > >>>> > >>>> To accommodate confidential compute VMs that expose the simplified CC > >>>> measurement protocol instead of the full-blown TCG2 one, fall back to > >>>> the former if the latter does not exist. > >>>> > >>>> The CC protocol was designed to be used in this manner, which is why the > >>>> types and prototypes have been kept the same where possible. So reuse > >>>> the existing code, and only deviate from the TCG2 code path where > >>>> needed. > >>>> > >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >>>> --- > >>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > >>>> drivers/firmware/efi/libstub/efistub.h | 3 + > >>>> 2 files changed, 53 insertions(+), 17 deletions(-) > >>>> > >>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>> index 0dbc9d3f4abd..21f4567324f6 100644 > >>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > >>>> unsigned long load_size, > >>>> enum efistub_event_type event) > >>>> { > >>>> + union { > >>>> + efi_status_t > >>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > >>>> + u64, const union efistub_event *); > >>>> + struct { u32 hash_log_extend_event; } mixed_mode; > >>>> + } method; > >>>> 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; > >>>> + union efistub_event ev; > >>>> efi_status_t status; > >>>> + void *protocol; > >>>> > >>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > >>>> if (tcg2) { > >>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > >>>> - (void **)&evt); > >>>> - if (status != EFI_SUCCESS) > >>>> - goto fail; > >>>> - > >>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > >>>> + ev.tcg2_data = (struct efi_tcg2_event){ > >>>> .event_size = size, > >>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > >>>> + .event_header.header_size = sizeof(ev.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, > >>>> }; > >>>> + protocol = tcg2; > >>>> + method.hash_log_extend_event = > >>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event); > >>>> + } else { > >>> +Qinkun Bao > >>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > >>> TCG protocol breaks backwards compatibility, it'd be preferable to > >>> measure into all the measurement protocols that are present. > >> How so? Older kernels will use TCG2 if it exists, and so will new > >> kernels. The only difference is that on new kernels, the CC protocol > >> will be used in case TCG2 is not implemented. > >> > >> So the only affected scenario here is a system that today implements > >> TCG but not CC, and intends to implement CC later and receive > >> measurements into both protocols. Does that really qualify as backward > >> compatibility? I'd rather not accommodate future systems that > >> implement something that the UEFI spec says they should not. > >> > >>> The UEFI > >>> v2.10 standard says that firmware "should not" provide both, but it is > >>> not MUST NOT. Given this and our desire to provide service continuity, > >>> I ask that you remove the "else" guard. > >>> > >> Ignoring the newer protocol if the established one exists is an > >> excellent way of making sure this does not happen. > >> > > The problem is that the protocols are not equivalent, and we disagree > > with the standard's claim of "should not" to the point that we're > > taking it to the USWG. The "should not" advisement is predicated on > > not trusting boot layers to use both protocols when they're both > > present, such that you could accidentally miss measuring a > > security-critical event. That's a strawman though, since you already > > need to develop trust in those boot layers. We have software supply > > chain endorsements for tracking that kind of property for use in > > attestation verification. > > > > The CC protocol is useful for hardware-rooted boot measurement, but it > > does nothing about the rest of TPM 2.0. There are plenty of users that > > want to use a vTPM that's hosted by the VMM but also get an extra > > integrity assurance that measurements into TDX RTMRs and attested by > > an Intel-rooted key pass an extra level of scrutiny. > > > > If you check the EDK2 part of this support, it also uses if else model. Yes, we've been discussing this with Intel and they agreed to allow a default false build option to measure into both. > It does not measure both. If there a complete vTPM support, why > can't user trust measurements from it? I think the CC vendors will There are folks who want to do a double-check with TEE quotes, but yes I agree in general this is not the best situation. It's a stepping stones model rather than scaling Everest in one bound. Ideally you'd have a measured and protected TPM implementation with adequate security for persistent data so that the CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed. But anyway, the standard is what it is for now, so I wouldn't block this patch based on this request. When there's more alignment from the UEFI standards working group and an accepted patch into EDK2, then we can revisit this in the different boot layers. > ensure their vTPM implementation is protected from attack from the > host (like implementing it part of firmware or launching it as service in > a separate VM). > > >>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > >>>> + efi_cc_protocol_t *cc = NULL; > >>>> > >>>> - 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); > >>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > >>>> + if (!cc) > >>>> + return EFI_UNSUPPORTED; > >>>> > >>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > >>>> - load_addr, load_size, &evt->event_data.tcg2_data); > >>>> - efi_bs_call(free_pool, evt); > >>>> + ev.cc_data = (struct efi_cc_event){ > >>>> + .event_size = size, > >>>> + .event_header.header_size = sizeof(ev.cc_data.event_header), > >>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > >>>> + .event_header.event_type = EV_EVENT_TAG, > >>>> + }; > >>>> > >>>> + status = efi_call_proto(cc, map_pcr_to_mr_index, > >>>> + events[event].pcr_index, > >>>> + &ev.cc_data.event_header.mr_index); > >>>> if (status != EFI_SUCCESS) > >>>> goto fail; > >>>> - return EFI_SUCCESS; > >>>> + > >>>> + protocol = cc; > >>>> + method.hash_log_extend_event = > >>>> + (void *)efi_table_attr(cc, hash_log_extend_event); > >>>> } > >>>> > >>>> - return EFI_UNSUPPORTED; > >>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > >>>> + if (status != EFI_SUCCESS) > >>>> + goto fail; > >>>> + > >>>> + evt->event_data = ev; > >>>> + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > >>>> + load_addr, load_size, &evt->event_data); > >>>> + efi_bs_call(free_pool, evt); > >>>> + > >>>> + if (status == EFI_SUCCESS) > >>>> + return EFI_SUCCESS; > >>>> + > >>>> fail: > >>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > >>>> return status; > >>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > >>>> index d621bfb719c4..4bf9a76796b7 100644 > >>>> --- a/drivers/firmware/efi/libstub/efistub.h > >>>> +++ b/drivers/firmware/efi/libstub/efistub.h > >>>> @@ -954,8 +954,11 @@ union efi_cc_protocol { > >>>> } mixed_mode; > >>>> }; > >>>> > >>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > >>>> + > >>>> union efistub_event { > >>>> efi_tcg2_event_t tcg2_data; > >>>> + efi_cc_event_t cc_data; > >>>> }; > >>>> > >>>> struct efistub_measured_event { > >>>> -- > >>>> 2.44.0.278.ge034bb2e1d-goog > >>>> > >>>> > >>> > >>> -- > >>> -Dionna Glaze, PhD (she/her) > > > > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer >
On 3/5/24 10:46 AM, Dionna Amalie Glaze wrote: > On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> >> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote: >>> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: >>>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: >>>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: >>>>>> From: Ard Biesheuvel <ardb@kernel.org> >>>>>> >>>>>> To accommodate confidential compute VMs that expose the simplified CC >>>>>> measurement protocol instead of the full-blown TCG2 one, fall back to >>>>>> the former if the latter does not exist. >>>>>> >>>>>> The CC protocol was designed to be used in this manner, which is why the >>>>>> types and prototypes have been kept the same where possible. So reuse >>>>>> the existing code, and only deviate from the TCG2 code path where >>>>>> needed. >>>>>> >>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> >>>>>> --- >>>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- >>>>>> drivers/firmware/efi/libstub/efistub.h | 3 + >>>>>> 2 files changed, 53 insertions(+), 17 deletions(-) >>>>>> >>>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c >>>>>> index 0dbc9d3f4abd..21f4567324f6 100644 >>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c >>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c >>>>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, >>>>>> unsigned long load_size, >>>>>> enum efistub_event_type event) >>>>>> { >>>>>> + union { >>>>>> + efi_status_t >>>>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, >>>>>> + u64, const union efistub_event *); >>>>>> + struct { u32 hash_log_extend_event; } mixed_mode; >>>>>> + } method; >>>>>> 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; >>>>>> + union efistub_event ev; >>>>>> efi_status_t status; >>>>>> + void *protocol; >>>>>> >>>>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); >>>>>> if (tcg2) { >>>>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, >>>>>> - (void **)&evt); >>>>>> - if (status != EFI_SUCCESS) >>>>>> - goto fail; >>>>>> - >>>>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){ >>>>>> + ev.tcg2_data = (struct efi_tcg2_event){ >>>>>> .event_size = size, >>>>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), >>>>>> + .event_header.header_size = sizeof(ev.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, >>>>>> }; >>>>>> + protocol = tcg2; >>>>>> + method.hash_log_extend_event = >>>>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event); >>>>>> + } else { >>>>> +Qinkun Bao >>>>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the >>>>> TCG protocol breaks backwards compatibility, it'd be preferable to >>>>> measure into all the measurement protocols that are present. >>>> How so? Older kernels will use TCG2 if it exists, and so will new >>>> kernels. The only difference is that on new kernels, the CC protocol >>>> will be used in case TCG2 is not implemented. >>>> >>>> So the only affected scenario here is a system that today implements >>>> TCG but not CC, and intends to implement CC later and receive >>>> measurements into both protocols. Does that really qualify as backward >>>> compatibility? I'd rather not accommodate future systems that >>>> implement something that the UEFI spec says they should not. >>>> >>>>> The UEFI >>>>> v2.10 standard says that firmware "should not" provide both, but it is >>>>> not MUST NOT. Given this and our desire to provide service continuity, >>>>> I ask that you remove the "else" guard. >>>>> >>>> Ignoring the newer protocol if the established one exists is an >>>> excellent way of making sure this does not happen. >>>> >>> The problem is that the protocols are not equivalent, and we disagree >>> with the standard's claim of "should not" to the point that we're >>> taking it to the USWG. The "should not" advisement is predicated on >>> not trusting boot layers to use both protocols when they're both >>> present, such that you could accidentally miss measuring a >>> security-critical event. That's a strawman though, since you already >>> need to develop trust in those boot layers. We have software supply >>> chain endorsements for tracking that kind of property for use in >>> attestation verification. >>> >>> The CC protocol is useful for hardware-rooted boot measurement, but it >>> does nothing about the rest of TPM 2.0. There are plenty of users that >>> want to use a vTPM that's hosted by the VMM but also get an extra >>> integrity assurance that measurements into TDX RTMRs and attested by >>> an Intel-rooted key pass an extra level of scrutiny. >>> >> If you check the EDK2 part of this support, it also uses if else model. > Yes, we've been discussing this with Intel and they agreed to allow a > default false build option to measure into both. To make it clear, any plans to update the spec with this requirement? > >> It does not measure both. If there a complete vTPM support, why >> can't user trust measurements from it? I think the CC vendors will > There are folks who want to do a double-check with TEE quotes, but yes > I agree in general this is not the best situation. It's a stepping > stones model rather than scaling Everest in one bound. > Ideally you'd have a measured and protected TPM implementation with > adequate security for persistent data so that the > CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed. > > But anyway, the standard is what it is for now, so I wouldn't block > this patch based on this request. When there's more alignment from the > UEFI standards working group and an accepted patch into EDK2, then we > can revisit this in the different boot layers. Got it. Personally I think we can consider it, once it is adapted in firmware and updated in spec (to make it consistent with firmware support). Anyway, since doing it is harmless, I will leave it to the maintainers choice. > >> ensure their vTPM implementation is protected from attack from the >> host (like implementing it part of firmware or launching it as service in >> a separate VM). >> >>>>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; >>>>>> + efi_cc_protocol_t *cc = NULL; >>>>>> >>>>>> - 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); >>>>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); >>>>>> + if (!cc) >>>>>> + return EFI_UNSUPPORTED; >>>>>> >>>>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0, >>>>>> - load_addr, load_size, &evt->event_data.tcg2_data); >>>>>> - efi_bs_call(free_pool, evt); >>>>>> + ev.cc_data = (struct efi_cc_event){ >>>>>> + .event_size = size, >>>>>> + .event_header.header_size = sizeof(ev.cc_data.event_header), >>>>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, >>>>>> + .event_header.event_type = EV_EVENT_TAG, >>>>>> + }; >>>>>> >>>>>> + status = efi_call_proto(cc, map_pcr_to_mr_index, >>>>>> + events[event].pcr_index, >>>>>> + &ev.cc_data.event_header.mr_index); >>>>>> if (status != EFI_SUCCESS) >>>>>> goto fail; >>>>>> - return EFI_SUCCESS; >>>>>> + >>>>>> + protocol = cc; >>>>>> + method.hash_log_extend_event = >>>>>> + (void *)efi_table_attr(cc, hash_log_extend_event); >>>>>> } >>>>>> >>>>>> - return EFI_UNSUPPORTED; >>>>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); >>>>>> + if (status != EFI_SUCCESS) >>>>>> + goto fail; >>>>>> + >>>>>> + evt->event_data = ev; >>>>>> + 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_fn_call(&method, hash_log_extend_event, protocol, 0, >>>>>> + load_addr, load_size, &evt->event_data); >>>>>> + efi_bs_call(free_pool, evt); >>>>>> + >>>>>> + if (status == EFI_SUCCESS) >>>>>> + return EFI_SUCCESS; >>>>>> + >>>>>> fail: >>>>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); >>>>>> return status; >>>>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h >>>>>> index d621bfb719c4..4bf9a76796b7 100644 >>>>>> --- a/drivers/firmware/efi/libstub/efistub.h >>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h >>>>>> @@ -954,8 +954,11 @@ union efi_cc_protocol { >>>>>> } mixed_mode; >>>>>> }; >>>>>> >>>>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); >>>>>> + >>>>>> union efistub_event { >>>>>> efi_tcg2_event_t tcg2_data; >>>>>> + efi_cc_event_t cc_data; >>>>>> }; >>>>>> >>>>>> struct efistub_measured_event { >>>>>> -- >>>>>> 2.44.0.278.ge034bb2e1d-goog >>>>>> >>>>>> >>>>> -- >>>>> -Dionna Glaze, PhD (she/her) >>> >> -- >> Sathyanarayanan Kuppuswamy >> Linux Kernel Developer >> >
On Tue, Mar 5, 2024 at 11:36 AM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > On 3/5/24 10:46 AM, Dionna Amalie Glaze wrote: > > On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan > > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >> > >> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote: > >>> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: > >>>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > >>>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > >>>>>> From: Ard Biesheuvel <ardb@kernel.org> > >>>>>> > >>>>>> To accommodate confidential compute VMs that expose the simplified CC > >>>>>> measurement protocol instead of the full-blown TCG2 one, fall back to > >>>>>> the former if the latter does not exist. > >>>>>> > >>>>>> The CC protocol was designed to be used in this manner, which is why the > >>>>>> types and prototypes have been kept the same where possible. So reuse > >>>>>> the existing code, and only deviate from the TCG2 code path where > >>>>>> needed. > >>>>>> > >>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > >>>>>> --- > >>>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > >>>>>> drivers/firmware/efi/libstub/efistub.h | 3 + > >>>>>> 2 files changed, 53 insertions(+), 17 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>>>> index 0dbc9d3f4abd..21f4567324f6 100644 > >>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > >>>>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > >>>>>> unsigned long load_size, > >>>>>> enum efistub_event_type event) > >>>>>> { > >>>>>> + union { > >>>>>> + efi_status_t > >>>>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > >>>>>> + u64, const union efistub_event *); > >>>>>> + struct { u32 hash_log_extend_event; } mixed_mode; > >>>>>> + } method; > >>>>>> 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; > >>>>>> + union efistub_event ev; > >>>>>> efi_status_t status; > >>>>>> + void *protocol; > >>>>>> > >>>>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > >>>>>> if (tcg2) { > >>>>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > >>>>>> - (void **)&evt); > >>>>>> - if (status != EFI_SUCCESS) > >>>>>> - goto fail; > >>>>>> - > >>>>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > >>>>>> + ev.tcg2_data = (struct efi_tcg2_event){ > >>>>>> .event_size = size, > >>>>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > >>>>>> + .event_header.header_size = sizeof(ev.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, > >>>>>> }; > >>>>>> + protocol = tcg2; > >>>>>> + method.hash_log_extend_event = > >>>>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event); > >>>>>> + } else { > >>>>> +Qinkun Bao > >>>>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > >>>>> TCG protocol breaks backwards compatibility, it'd be preferable to > >>>>> measure into all the measurement protocols that are present. > >>>> How so? Older kernels will use TCG2 if it exists, and so will new > >>>> kernels. The only difference is that on new kernels, the CC protocol > >>>> will be used in case TCG2 is not implemented. > >>>> > >>>> So the only affected scenario here is a system that today implements > >>>> TCG but not CC, and intends to implement CC later and receive > >>>> measurements into both protocols. Does that really qualify as backward > >>>> compatibility? I'd rather not accommodate future systems that > >>>> implement something that the UEFI spec says they should not. > >>>> > >>>>> The UEFI > >>>>> v2.10 standard says that firmware "should not" provide both, but it is > >>>>> not MUST NOT. Given this and our desire to provide service continuity, > >>>>> I ask that you remove the "else" guard. > >>>>> > >>>> Ignoring the newer protocol if the established one exists is an > >>>> excellent way of making sure this does not happen. > >>>> > >>> The problem is that the protocols are not equivalent, and we disagree > >>> with the standard's claim of "should not" to the point that we're > >>> taking it to the USWG. The "should not" advisement is predicated on > >>> not trusting boot layers to use both protocols when they're both > >>> present, such that you could accidentally miss measuring a > >>> security-critical event. That's a strawman though, since you already > >>> need to develop trust in those boot layers. We have software supply > >>> chain endorsements for tracking that kind of property for use in > >>> attestation verification. > >>> > >>> The CC protocol is useful for hardware-rooted boot measurement, but it > >>> does nothing about the rest of TPM 2.0. There are plenty of users that > >>> want to use a vTPM that's hosted by the VMM but also get an extra > >>> integrity assurance that measurements into TDX RTMRs and attested by > >>> an Intel-rooted key pass an extra level of scrutiny. > >>> > >> If you check the EDK2 part of this support, it also uses if else model. > > Yes, we've been discussing this with Intel and they agreed to allow a > > default false build option to measure into both. > > To make it clear, any plans to update the spec with this requirement? > I don't think so. I do agree generally with the "should not" wording as a long term end goal. But given both interfaces anyway, I think they should both be used. If that clarification is required in the spec, I can take it to the USWG. I will note that the "else" interpretation is not universal, as is the case in rhboot/shim and grub2. > > > >> It does not measure both. If there a complete vTPM support, why > >> can't user trust measurements from it? I think the CC vendors will > > There are folks who want to do a double-check with TEE quotes, but yes > > I agree in general this is not the best situation. It's a stepping > > stones model rather than scaling Everest in one bound. > > Ideally you'd have a measured and protected TPM implementation with > > adequate security for persistent data so that the > > CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed. > > > > But anyway, the standard is what it is for now, so I wouldn't block > > this patch based on this request. When there's more alignment from the > > UEFI standards working group and an accepted patch into EDK2, then we > > can revisit this in the different boot layers. > > Got it. Personally I think we can consider it, once it is adapted in > firmware and updated in spec (to make it consistent with firmware > support). Anyway, since doing it is harmless, I will leave it to the > maintainers choice. > > > > >> ensure their vTPM implementation is protected from attack from the > >> host (like implementing it part of firmware or launching it as service in > >> a separate VM). > >> > >>>>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > >>>>>> + efi_cc_protocol_t *cc = NULL; > >>>>>> > >>>>>> - 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); > >>>>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > >>>>>> + if (!cc) > >>>>>> + return EFI_UNSUPPORTED; > >>>>>> > >>>>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > >>>>>> - load_addr, load_size, &evt->event_data.tcg2_data); > >>>>>> - efi_bs_call(free_pool, evt); > >>>>>> + ev.cc_data = (struct efi_cc_event){ > >>>>>> + .event_size = size, > >>>>>> + .event_header.header_size = sizeof(ev.cc_data.event_header), > >>>>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > >>>>>> + .event_header.event_type = EV_EVENT_TAG, > >>>>>> + }; > >>>>>> > >>>>>> + status = efi_call_proto(cc, map_pcr_to_mr_index, > >>>>>> + events[event].pcr_index, > >>>>>> + &ev.cc_data.event_header.mr_index); > >>>>>> if (status != EFI_SUCCESS) > >>>>>> goto fail; > >>>>>> - return EFI_SUCCESS; > >>>>>> + > >>>>>> + protocol = cc; > >>>>>> + method.hash_log_extend_event = > >>>>>> + (void *)efi_table_attr(cc, hash_log_extend_event); > >>>>>> } > >>>>>> > >>>>>> - return EFI_UNSUPPORTED; > >>>>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > >>>>>> + if (status != EFI_SUCCESS) > >>>>>> + goto fail; > >>>>>> + > >>>>>> + evt->event_data = ev; > >>>>>> + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > >>>>>> + load_addr, load_size, &evt->event_data); > >>>>>> + efi_bs_call(free_pool, evt); > >>>>>> + > >>>>>> + if (status == EFI_SUCCESS) > >>>>>> + return EFI_SUCCESS; > >>>>>> + > >>>>>> fail: > >>>>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > >>>>>> return status; > >>>>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > >>>>>> index d621bfb719c4..4bf9a76796b7 100644 > >>>>>> --- a/drivers/firmware/efi/libstub/efistub.h > >>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h > >>>>>> @@ -954,8 +954,11 @@ union efi_cc_protocol { > >>>>>> } mixed_mode; > >>>>>> }; > >>>>>> > >>>>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > >>>>>> + > >>>>>> union efistub_event { > >>>>>> efi_tcg2_event_t tcg2_data; > >>>>>> + efi_cc_event_t cc_data; > >>>>>> }; > >>>>>> > >>>>>> struct efistub_measured_event { > >>>>>> -- > >>>>>> 2.44.0.278.ge034bb2e1d-goog > >>>>>> > >>>>>> > >>>>> -- > >>>>> -Dionna Glaze, PhD (she/her) > >>> > >> -- > >> Sathyanarayanan Kuppuswamy > >> Linux Kernel Developer > >> > > > -- > Sathyanarayanan Kuppuswamy > Linux Kernel Developer >
On Tue, 5 Mar 2024 at 19:46, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > > > > On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote: > > > On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > >> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote: > > >>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote: > > >>>> From: Ard Biesheuvel <ardb@kernel.org> > > >>>> > > >>>> To accommodate confidential compute VMs that expose the simplified CC > > >>>> measurement protocol instead of the full-blown TCG2 one, fall back to > > >>>> the former if the latter does not exist. > > >>>> > > >>>> The CC protocol was designed to be used in this manner, which is why the > > >>>> types and prototypes have been kept the same where possible. So reuse > > >>>> the existing code, and only deviate from the TCG2 code path where > > >>>> needed. > > >>>> > > >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > >>>> --- > > >>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > > >>>> drivers/firmware/efi/libstub/efistub.h | 3 + > > >>>> 2 files changed, 53 insertions(+), 17 deletions(-) > > >>>> > > >>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > > >>>> index 0dbc9d3f4abd..21f4567324f6 100644 > > >>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > > >>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > > >>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > > >>>> unsigned long load_size, > > >>>> enum efistub_event_type event) > > >>>> { > > >>>> + union { > > >>>> + efi_status_t > > >>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > > >>>> + u64, const union efistub_event *); > > >>>> + struct { u32 hash_log_extend_event; } mixed_mode; > > >>>> + } method; > > >>>> 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; > > >>>> + union efistub_event ev; > > >>>> efi_status_t status; > > >>>> + void *protocol; > > >>>> > > >>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > > >>>> if (tcg2) { > > >>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > > >>>> - (void **)&evt); > > >>>> - if (status != EFI_SUCCESS) > > >>>> - goto fail; > > >>>> - > > >>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > > >>>> + ev.tcg2_data = (struct efi_tcg2_event){ > > >>>> .event_size = size, > > >>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > > >>>> + .event_header.header_size = sizeof(ev.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, > > >>>> }; > > >>>> + protocol = tcg2; > > >>>> + method.hash_log_extend_event = > > >>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event); > > >>>> + } else { > > >>> +Qinkun Bao > > >>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the > > >>> TCG protocol breaks backwards compatibility, it'd be preferable to > > >>> measure into all the measurement protocols that are present. > > >> How so? Older kernels will use TCG2 if it exists, and so will new > > >> kernels. The only difference is that on new kernels, the CC protocol > > >> will be used in case TCG2 is not implemented. > > >> > > >> So the only affected scenario here is a system that today implements > > >> TCG but not CC, and intends to implement CC later and receive > > >> measurements into both protocols. Does that really qualify as backward > > >> compatibility? I'd rather not accommodate future systems that > > >> implement something that the UEFI spec says they should not. > > >> > > >>> The UEFI > > >>> v2.10 standard says that firmware "should not" provide both, but it is > > >>> not MUST NOT. Given this and our desire to provide service continuity, > > >>> I ask that you remove the "else" guard. > > >>> > > >> Ignoring the newer protocol if the established one exists is an > > >> excellent way of making sure this does not happen. > > >> > > > The problem is that the protocols are not equivalent, and we disagree > > > with the standard's claim of "should not" to the point that we're > > > taking it to the USWG. The "should not" advisement is predicated on > > > not trusting boot layers to use both protocols when they're both > > > present, such that you could accidentally miss measuring a > > > security-critical event. That's a strawman though, since you already > > > need to develop trust in those boot layers. We have software supply > > > chain endorsements for tracking that kind of property for use in > > > attestation verification. > > > > > > The CC protocol is useful for hardware-rooted boot measurement, but it > > > does nothing about the rest of TPM 2.0. There are plenty of users that > > > want to use a vTPM that's hosted by the VMM but also get an extra > > > integrity assurance that measurements into TDX RTMRs and attested by > > > an Intel-rooted key pass an extra level of scrutiny. > > > > > > > If you check the EDK2 part of this support, it also uses if else model. > > Yes, we've been discussing this with Intel and they agreed to allow a > default false build option to measure into both. > > > It does not measure both. If there a complete vTPM support, why > > can't user trust measurements from it? I think the CC vendors will > > There are folks who want to do a double-check with TEE quotes, but yes > I agree in general this is not the best situation. It's a stepping > stones model rather than scaling Everest in one bound. > Ideally you'd have a measured and protected TPM implementation with > adequate security for persistent data so that the > CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed. > > But anyway, the standard is what it is for now, so I wouldn't block > this patch based on this request. When there's more alignment from the > UEFI standards working group and an accepted patch into EDK2, then we > can revisit this in the different boot layers. > Yeah some spec guidance on when having both protocols makes sense and why would help here. If you trust the VMM to operate the vTPM for you, what is the point of the CC protocol? For now, I'll go with the changes as proposed, also because we're close to the merge window. We can always revisit this and backport if needed.
On 3/4/24 2:44 AM, Ard Biesheuvel wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > To accommodate confidential compute VMs that expose the simplified CC > measurement protocol instead of the full-blown TCG2 one, fall back to > the former if the latter does not exist. > > The CC protocol was designed to be used in this manner, which is why the > types and prototypes have been kept the same where possible. So reuse > the existing code, and only deviate from the TCG2 code path where > needed. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- Looks fine to me. Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++----- > drivers/firmware/efi/libstub/efistub.h | 3 + > 2 files changed, 53 insertions(+), 17 deletions(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c > index 0dbc9d3f4abd..21f4567324f6 100644 > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, > unsigned long load_size, > enum efistub_event_type event) > { > + union { > + efi_status_t > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, > + u64, const union efistub_event *); > + struct { u32 hash_log_extend_event; } mixed_mode; > + } method; > 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; > + union efistub_event ev; > efi_status_t status; > + void *protocol; > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); > if (tcg2) { > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, > - (void **)&evt); > - if (status != EFI_SUCCESS) > - goto fail; > - > - evt->event_data.tcg2_data = (struct efi_tcg2_event){ > + ev.tcg2_data = (struct efi_tcg2_event){ > .event_size = size, > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), > + .event_header.header_size = sizeof(ev.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, > }; > + protocol = tcg2; > + method.hash_log_extend_event = > + (void *)efi_table_attr(tcg2, hash_log_extend_event); > + } else { > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; > + efi_cc_protocol_t *cc = NULL; > > - 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); > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); > + if (!cc) > + return EFI_UNSUPPORTED; > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0, > - load_addr, load_size, &evt->event_data.tcg2_data); > - efi_bs_call(free_pool, evt); > + ev.cc_data = (struct efi_cc_event){ > + .event_size = size, > + .event_header.header_size = sizeof(ev.cc_data.event_header), > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, > + .event_header.event_type = EV_EVENT_TAG, > + }; > > + status = efi_call_proto(cc, map_pcr_to_mr_index, > + events[event].pcr_index, > + &ev.cc_data.event_header.mr_index); > if (status != EFI_SUCCESS) > goto fail; > - return EFI_SUCCESS; > + > + protocol = cc; > + method.hash_log_extend_event = > + (void *)efi_table_attr(cc, hash_log_extend_event); > } > > - return EFI_UNSUPPORTED; > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); > + if (status != EFI_SUCCESS) > + goto fail; > + > + evt->event_data = ev; > + 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_fn_call(&method, hash_log_extend_event, protocol, 0, > + load_addr, load_size, &evt->event_data); > + efi_bs_call(free_pool, evt); > + > + if (status == EFI_SUCCESS) > + return EFI_SUCCESS; > + > fail: > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); > return status; > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index d621bfb719c4..4bf9a76796b7 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -954,8 +954,11 @@ union efi_cc_protocol { > } mixed_mode; > }; > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); > + > union efistub_event { > efi_tcg2_event_t tcg2_data; > + efi_cc_event_t cc_data; > }; > > struct efistub_measured_event {
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index 0dbc9d3f4abd..21f4567324f6 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr, unsigned long load_size, enum efistub_event_type event) { + union { + efi_status_t + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t, + u64, const union efistub_event *); + struct { u32 hash_log_extend_event; } mixed_mode; + } method; 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; + union efistub_event ev; efi_status_t status; + void *protocol; efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2); if (tcg2) { - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, - (void **)&evt); - if (status != EFI_SUCCESS) - goto fail; - - evt->event_data.tcg2_data = (struct efi_tcg2_event){ + ev.tcg2_data = (struct efi_tcg2_event){ .event_size = size, - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header), + .event_header.header_size = sizeof(ev.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, }; + protocol = tcg2; + method.hash_log_extend_event = + (void *)efi_table_attr(tcg2, hash_log_extend_event); + } else { + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID; + efi_cc_protocol_t *cc = NULL; - 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); + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc); + if (!cc) + return EFI_UNSUPPORTED; - status = efi_call_proto(tcg2, hash_log_extend_event, 0, - load_addr, load_size, &evt->event_data.tcg2_data); - efi_bs_call(free_pool, evt); + ev.cc_data = (struct efi_cc_event){ + .event_size = size, + .event_header.header_size = sizeof(ev.cc_data.event_header), + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION, + .event_header.event_type = EV_EVENT_TAG, + }; + status = efi_call_proto(cc, map_pcr_to_mr_index, + events[event].pcr_index, + &ev.cc_data.event_header.mr_index); if (status != EFI_SUCCESS) goto fail; - return EFI_SUCCESS; + + protocol = cc; + method.hash_log_extend_event = + (void *)efi_table_attr(cc, hash_log_extend_event); } - return EFI_UNSUPPORTED; + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt); + if (status != EFI_SUCCESS) + goto fail; + + evt->event_data = ev; + 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_fn_call(&method, hash_log_extend_event, protocol, 0, + load_addr, load_size, &evt->event_data); + efi_bs_call(free_pool, evt); + + if (status == EFI_SUCCESS) + return EFI_SUCCESS; + fail: efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status); return status; diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index d621bfb719c4..4bf9a76796b7 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -954,8 +954,11 @@ union efi_cc_protocol { } mixed_mode; }; +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t)); + union efistub_event { efi_tcg2_event_t tcg2_data; + efi_cc_event_t cc_data; }; struct efistub_measured_event {