Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

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
>>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer