| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7bd53417-af59-43a8-965e-f63dfc827f3c@linux.intel.com>
Date: Fri, 23 Feb 2024 23:37:39 -0800
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@...ux.intel.com>
To: Ilias Apalodimas <ilias.apalodimas@...aro.org>
Cc: Ard Biesheuvel <ardb@...nel.org>, linux-kernel@...r.kernel.org,
linux-efi@...r.kernel.org
Subject: Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC)
measurement support
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@...ux.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@...ux.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
Powered by blists - more mailing lists