Received-SPF: pass (google.com: domain of linux-kernel+bounces-79503-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <7bd53417-af59-43a8-965e-f63dfc827f3c@linux.intel.com>
Date: Fri, 23 Feb 2024 23:37:39 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC)
 measurement support
Content-Language: en-US
To: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Ard Biesheuvel <ardb@kernel.org>, linux-kernel@vger.kernel.org,
 linux-efi@vger.kernel.org
References: <20240215030002.281456-1-sathyanarayanan.kuppuswamy@linux.intel.com>
 <20240215030002.281456-2-sathyanarayanan.kuppuswamy@linux.intel.com>
 <CAC_iWj+p6gmcb4-_-f8Kr4Ds6EmSsq2D4GK1XymyybX7i1TMFw@mail.gmail.com>
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In-Reply-To: <CAC_iWj+p6gmcb4-_-f8Kr4Ds6EmSsq2D4GK1XymyybX7i1TMFw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hi Ilias,

Thanks for the review.

On 2/18/24 10:38 PM, Ilias Apalodimas wrote:
> Hi Kuppuswamy,
>
> On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> If the virtual firmware implements TPM support, TCG2 protocol will be
>> used for kernel measurements and event logging support. But in CC
>> environment, not all platforms support or enable the TPM feature. UEFI
>> specification [1] exposes protocol and interfaces used for kernel
>> measurements in CC platforms without TPM support.
>>
>> Currently, the efi-stub only supports the kernel related measurements
>> for the platform that supports TCG2 protocol. So, extend it add
>> CC measurement protocol (EFI_CC_MEASUREMENT_PROTOCOL) and event logging
>> support. Event logging format in the CC environment is the same as
>> TCG2.
>>
>> More details about the EFI CC measurements and logging can be found
>> in [1].
>>
>> Link: https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#efi-cc-measurement-protocol [1]
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>
>> Changes since v1:
>> * Fixed missing tagged event data.
>>
>>  .../firmware/efi/libstub/efi-stub-helper.c    | 127 ++++++++++++++----
>>  drivers/firmware/efi/libstub/efistub.h        |  74 ++++++++++
>>  include/linux/efi.h                           |   1 +
>>  3 files changed, 174 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> index bfa30625f5d0..cc31f8143190 100644
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -219,50 +219,121 @@ static const struct {
>>         },
>>  };
>>
>> +static efi_status_t tcg2_efi_measure(efi_tcg2_protocol_t *tcg2,
>> +                                    unsigned long load_addr,
>> +                                    unsigned long load_size,
>> +                                    enum efistub_event event)
>> +{
>> +       struct efi_measured_event {
>> +               efi_tcg2_event_t        event_data;
>> +               efi_tcg2_tagged_event_t tagged_event;
>> +               u8                      tagged_event_data[];
>> +       } *evt;
>> +       int size = sizeof(*evt) + events[event].event_data_len;
> This is defined as size_t on the cc variant. I guess both are ok, just pick one

Ok

>> +       efi_status_t status;
>> +
>> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>> +                            (void **)&evt);
>> +       if (status != EFI_SUCCESS)
> pr_err() here as done in the cc variant?

I will remove the error message in the CC variant as well. I don't want to
introduce additional logs for existing case (tcg2) as well. May be I can
can use efi_debug just for the map_pcr_to_mr_index call.
>> +               return status;
>> +
>> +       evt->event_data = (struct efi_tcg2_event){
>> +               .event_size                     = size,
>> +               .event_header.header_size       = sizeof(evt->event_data.event_header),
>> +               .event_header.header_version    = EFI_TCG2_EVENT_HEADER_VERSION,
>> +               .event_header.pcr_index         = events[event].pcr_index,
>> +               .event_header.event_type        = EV_EVENT_TAG,
>> +       };
>> +
>> +       evt->tagged_event = (struct efi_tcg2_tagged_event){
>> +               .tagged_event_id                = events[event].event_id,
>> +               .tagged_event_data_size         = events[event].event_data_len,
>> +       };
>> +
>> +       memcpy(evt->tagged_event_data, events[event].event_data,
>> +              events[event].event_data_len);
>> +
>> +       status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>> +                               load_addr, load_size, &evt->event_data);
> The struct filling/memcpying looks similar across the 2 functions.  I
> wonder if it makes sense to have a common function for that, with an
> argument for the event data type.

If we want to use helper function, the updated code looks like below.

Are you fine with this version? (compile-tested only)

+struct efi_tcg2_measured_event {
+       efi_tcg2_event_t        event_data;
+       efi_tcg2_tagged_event_t tagged_event;
+       u8                      tagged_event_data[];
+};
+
+struct efi_cc_measured_event {
+       efi_cc_event_t  event_data;
+       efi_tcg2_tagged_event_t tagged_event;
+       u8                      tagged_event_data[];
+};
+
+static void efi_tcg2_event_init(struct efi_tcg2_measured_event *evt,
+                               size_t size,
+                               enum efistub_event event)
+{
+       evt->event_data = (struct efi_tcg2_event){
+               .event_size                     = size,
+               .event_header.header_size       = sizeof(evt->event_data.event_header),
+               .event_header.header_version    = EFI_TCG2_EVENT_HEADER_VERSION,
+               .event_header.pcr_index         = events[event].pcr_index,
+               .event_header.event_type        = EV_EVENT_TAG,
+       };
+
+       evt->tagged_event = (struct efi_tcg2_tagged_event){
+               .tagged_event_id                = events[event].event_id,
+               .tagged_event_data_size         = events[event].event_data_len,
+       };
+
+       memcpy(evt->tagged_event_data, events[event].event_data,
+              events[event].event_data_len);
+}
+
+static efi_status_t tcg2_efi_measure(efi_tcg2_protocol_t *tcg2,
+                                    unsigned long load_addr,
+                                    unsigned long load_size,
+                                    enum efistub_event event)
+{
+       struct efi_tcg2_measured_event *evt;
+       efi_status_t status;
+       size_t size;
+
+       size = sizeof(*evt) + events[event].event_data_len;
+
+       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
+                            (void **)&evt);
+       if (status != EFI_SUCCESS)
+               return status;
+
+       efi_tcg2_event_init(evt, size, event);
+
+       status = efi_call_proto(tcg2, hash_log_extend_event, 0,
+                               load_addr, load_size, &evt->event_data);
+       efi_bs_call(free_pool, evt);
+
+       return status;
+}

+
+static efi_status_t cc_efi_measure(efi_cc_protocol_t *cc,
+                                  unsigned long load_addr,
+                                  unsigned long load_size,
+                                  enum efistub_event event)
+{
+       struct efi_cc_measured_event *evt;
+       efi_cc_mr_index_t mr;
+       efi_status_t status;
+       size_t size;
+
+       status = efi_call_proto(cc, map_pcr_to_mr_index, events[event].pcr_index, &mr);
+       if (status != EFI_SUCCESS) {
+               efi_debug("CC_MEASURE: PCR to MR mapping failed\n");
+               return status;
+       }
+
+       size = sizeof(*evt) + events[event].event_data_len;
+
+       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
+       if (status != EFI_SUCCESS)
+               return status;
+
+       efi_tcg2_event_init((struct efi_tcg2_measured_event *)evt, size, event);
+
+       evt->event_data = (struct efi_cc_event){
+               .event_header.header_size       = sizeof(evt->event_data.event_header),
+               .event_header.header_version    = EFI_CC_EVENT_HEADER_VERSION,
+               .event_header.mr_index          = mr,
+       };
+
+       status = efi_call_proto(cc, hash_log_extend_event, 0,
+                               load_addr, load_size, &evt->event_data);
+
+       efi_bs_call(free_pool, evt);
+
+       return status;
+}


>
>> +       efi_bs_call(free_pool, evt);
>> +
>> +       return status;
>> +}
>> +
>> +static efi_status_t cc_efi_measure(efi_cc_protocol_t *cc,
>> +                                  unsigned long load_addr,
>> +                                  unsigned long load_size,
>> +                                  enum efistub_event event)
>> +{
>> +       struct efi_measured_event {
>> +               efi_cc_event_t  event_data;
>> +               efi_tcg2_tagged_event_t tagged_event;
>> +               u8                      tagged_event_data[];
>> +       } *evt;
>> +       size_t size = sizeof(*evt) + events[event].event_data_len;
>> +       efi_cc_mr_index_t mr;
>> +       efi_status_t status;
>> +
>> +       status = efi_call_proto(cc, map_pcr_to_mr_index, events[event].pcr_index, &mr);
>> +       if (status != EFI_SUCCESS) {
>> +               efi_err("CC_MEASURE: PCR to MR mapping failed\n");
>> +               return status;
>> +       }
>> +
>> +       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
>> +       if (status != EFI_SUCCESS) {
>> +               efi_err("CC_MEASURE: Allocating event struct failed\n");
>> +               return status;
>> +       }
>> +
>> +       evt->event_data = (struct efi_cc_event){
>> +               .event_size                     = size,
>> +               .event_header.header_size       = sizeof(evt->event_data.event_header),
>> +               .event_header.header_version    = EFI_CC_EVENT_HEADER_VERSION,
>> +               .event_header.mr_index          = mr,
>> +               .event_header.event_type        = EV_EVENT_TAG,
>> +       };
>> +
>> +       evt->tagged_event = (struct efi_tcg2_tagged_event){
>> +               .tagged_event_id                = events[event].event_id,
>> +               .tagged_event_data_size         = events[event].event_data_len,
>> +       };
>> +
>> +       memcpy(evt->tagged_event_data, events[event].event_data,
>> +              events[event].event_data_len);
>> +
>> +       status = efi_call_proto(cc, hash_log_extend_event, 0,
>> +                               load_addr, load_size, &evt->event_data);
>> +
>> +       efi_bs_call(free_pool, evt);
>> +
>> +       return status;
>> +}
>>  static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>>                                              unsigned long load_size,
>>                                              enum efistub_event event)
>>  {
>>         efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
>> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>> +       efi_cc_protocol_t *cc = NULL;
>>         efi_tcg2_protocol_t *tcg2 = NULL;
>>         efi_status_t status;
>>
>>         efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
>>         if (tcg2) {
>> -               struct efi_measured_event {
>> -                       efi_tcg2_event_t        event_data;
>> -                       efi_tcg2_tagged_event_t tagged_event;
>> -                       u8                      tagged_event_data[];
>> -               } *evt;
>> -               int size = sizeof(*evt) + events[event].event_data_len;
>> -
>> -               status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>> -                                    (void **)&evt);
>> +               status = tcg2_efi_measure(tcg2, load_addr, load_size, event);
>>                 if (status != EFI_SUCCESS)
>>                         goto fail;
>>
>> -               evt->event_data = (struct efi_tcg2_event){
>> -                       .event_size                     = size,
>> -                       .event_header.header_size       = sizeof(evt->event_data.event_header),
>> -                       .event_header.header_version    = EFI_TCG2_EVENT_HEADER_VERSION,
>> -                       .event_header.pcr_index         = events[event].pcr_index,
>> -                       .event_header.event_type        = EV_EVENT_TAG,
>> -               };
>> -
>> -               evt->tagged_event = (struct efi_tcg2_tagged_event){
>> -                       .tagged_event_id                = events[event].event_id,
>> -                       .tagged_event_data_size         = events[event].event_data_len,
>> -               };
>> -
>> -               memcpy(evt->tagged_event_data, events[event].event_data,
>> -                      events[event].event_data_len);
>> -
>> -               status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>> -                                       load_addr, load_size, &evt->event_data);
>> -               efi_bs_call(free_pool, evt);
>> +               return EFI_SUCCESS;
>> +       }
>>
>> +       efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>> +       if (cc) {
>> +               status = cc_efi_measure(cc, load_addr, load_size, event);
>>                 if (status != EFI_SUCCESS)
>>                         goto fail;
>> +
>>                 return EFI_SUCCESS;
>>         }
>>
> [...]
>
> Thanks
> /Ilias

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer