On Tue, Mar 5, 2024 at 11:36 AM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> wrote:
> >>
> >> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
> >>> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >>>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@xxxxxxxxxx> wrote:
> >>>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@xxxxxxxxxx> wrote:
> >>>>>> From: Ard Biesheuvel <ardb@xxxxxxxxxx>
> >>>>>>
> >>>>>> 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@xxxxxxxxxx>
> >>>>>> ---
> >>>>>> 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
>
--
-Dionna Glaze, PhD (she/her)
