[PATCH v2 0/2] Add measurement and event log support for CC platforms

[PATCH v2 0/2] Add measurement and event log support for CC platforms

[Kuppuswamy Sathyanarayanan]

In a Confidential Computing (CC) environment, not all platforms
implement TPM support. Instead, it may support measurement and event
logging based upon the hardware Trusted Execution Environment (TEE)
capability. For such cases, UEFI specification [1] defines an
alternative measurement protocol and interfaces. This patch set
enables this support in EFI bootstub.

https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#efi-cc-measurement-protocol [1]

Changes since v1:

* Add missing tagged event data.
* Add support for get_event_log().

Kuppuswamy Sathyanarayanan (2):
  efi/libstub: Add Confidential Computing (CC) measurement support
  efi/libstub: Add get_event_log() support for CC platforms

 .../firmware/efi/libstub/efi-stub-helper.c    | 127 ++++++++++++++----
 drivers/firmware/efi/libstub/efi-stub.c       |   2 +-
 drivers/firmware/efi/libstub/efistub.h        |  78 ++++++++++-
 drivers/firmware/efi/libstub/tpm.c            |  78 +++++++----
 drivers/firmware/efi/libstub/x86-stub.c       |   2 +-
 include/linux/efi.h                           |   4 +
 6 files changed, 235 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC) measurement support

[Kuppuswamy Sathyanarayanan]

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;
+	efi_status_t status;
+
+	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
+			     (void **)&evt);
+	if (status != EFI_SUCCESS)
+		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);
+	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;
 	}
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 212687c30d79..2c43d04e2b86 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -882,6 +882,80 @@ union efi_tcg2_protocol {
 	} mixed_mode;
 };
 
+typedef struct {
+	u8 major;
+	u8 minor;
+} efi_cc_version_t;
+
+typedef struct {
+	u8 type;
+	u8 sub_type;
+} efi_cc_type_t;
+
+/* EFI CC type/subtype defines */
+#define EFI_CC_TYPE_NONE		0
+#define EFI_CC_TYPE_AMD_SEV		1
+#define EFI_CC_TYPE_INTEL_TDX		2
+
+typedef u32 efi_cc_mr_index_t;
+
+struct efi_cc_event {
+	u32 event_size;
+	struct {
+		u32 header_size;
+		u16 header_version;
+		u32 mr_index;
+		u32 event_type;
+	} __packed event_header;
+	u8 event_data[0];
+} __packed;
+
+typedef struct efi_cc_event efi_cc_event_t;
+typedef u32 efi_cc_event_log_bitmap_t;
+typedef u32 efi_cc_event_log_format_t;
+typedef u32 efi_cc_event_algorithm_bitmap_t;
+
+typedef struct {
+	u8 size;
+	efi_cc_version_t structure_version;
+	efi_cc_version_t protocol_version;
+	efi_cc_event_algorithm_bitmap_t hash_algorithm_bitmap;
+	efi_cc_event_log_bitmap_t supported_event_logs;
+	efi_cc_type_t cc_type;
+} efi_cc_boot_service_cap_t;
+
+#define EFI_CC_EVENT_HEADER_VERSION  1
+
+#define EFI_CC_BOOT_HASH_ALG_SHA384     0x00000004
+
+typedef union efi_cc_protocol efi_cc_protocol_t;
+
+union efi_cc_protocol {
+	struct {
+		efi_status_t (__efiapi *get_capability)(efi_cc_protocol_t *,
+							efi_cc_boot_service_cap_t *);
+		efi_status_t (__efiapi *get_event_log)(efi_cc_protocol_t *,
+						       efi_cc_event_log_format_t,
+						       efi_physical_addr_t *,
+						       efi_physical_addr_t *,
+						       efi_bool_t *);
+		efi_status_t (__efiapi *hash_log_extend_event)(efi_cc_protocol_t *,
+							       u64,
+							       efi_physical_addr_t,
+							       u64,
+							       const efi_cc_event_t *);
+		efi_status_t (__efiapi *map_pcr_to_mr_index)(efi_cc_protocol_t *,
+							     u32,
+							     efi_cc_mr_index_t *);
+	};
+	struct {
+		u32 get_capability;
+		u32 get_event_log;
+		u32 hash_log_extend_event;
+		u32 map_pcr_to_mr_index;
+	} mixed_mode;
+};
+
 struct riscv_efi_boot_protocol {
 	u64 revision;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c74f47711f0b..2f57fec2e629 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -400,6 +400,7 @@ void efi_native_runtime_setup(void);
 #define EFI_CERT_X509_GUID			EFI_GUID(0xa5c059a1, 0x94e4, 0x4aa7, 0x87, 0xb5, 0xab, 0x15, 0x5c, 0x2b, 0xf0, 0x72)
 #define EFI_CERT_X509_SHA256_GUID		EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
 #define EFI_CC_BLOB_GUID			EFI_GUID(0x067b1f5f, 0xcf26, 0x44c5, 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42)
+#define EFI_CC_MEASUREMENT_PROTOCOL_GUID	EFI_GUID(0x96751a3d, 0x72f4, 0x41a6, 0xa7, 0x94, 0xed, 0x5d, 0x0e, 0x67, 0xae, 0x6b)
 
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
-- 
2.25.1

Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC) measurement support

[Ilias Apalodimas]

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

> +       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?

> +               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.

> +       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

Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC) measurement support

[Kuppuswamy Sathyanarayanan]

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

Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC) measurement support

[Ilias Apalodimas]

Hi,

Thanks for taking a shot at this.

[...]

> >> +               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;
> +}
>

Yes, I think looks cleaner. Ard thoughts?

Thanks
/Ilias
>
> >
> >> +       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
>

Re: [PATCH v2 1/2] efi/libstub: Add Confidential Computing (CC) measurement support

[Ard Biesheuvel]

On Tue, 27 Feb 2024 at 14:23, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi,
>
> Thanks for taking a shot at this.
>
> [...]
>
> > >> +               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;
> > +}
> >
>
> Yes, I think looks cleaner. Ard thoughts?
>

I'd prefer to radically unify this code much further.

AFAICT, the *only* difference is the need to call
map_pcr_to_mr_index(), beyond that, everything is the same:
- efi_tcg2_event is identical to efi_cc_event
- the hash_log_extend_event() protocol member lives at the same offset
in the protocol struct, and has the same prototype

If we weren't as far along in the merge window, I'd ask you to respin
with this in mind. However, we're at -rc7 and so to avoid missing the
merge window, I went ahead and reworked the code. I'll send those out
momentarily.

[PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Kuppuswamy Sathyanarayanan]

To allow event log info access after boot, EFI boot stub extracts
the event log information and installs it in an EFI configuration
table. Currently, EFI boot stub only supports installation of event
log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
support code as much as possible.

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>
---
 drivers/firmware/efi/libstub/efi-stub.c |  2 +-
 drivers/firmware/efi/libstub/efistub.h  |  4 +-
 drivers/firmware/efi/libstub/tpm.c      | 78 +++++++++++++++++--------
 drivers/firmware/efi/libstub/x86-stub.c |  2 +-
 include/linux/efi.h                     |  3 +
 5 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index f9c1e8a2bd1d..958a680e0660 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -167,7 +167,7 @@ efi_status_t efi_stub_common(efi_handle_t handle,
 
 	si = setup_graphics();
 
-	efi_retrieve_tpm2_eventlog();
+	efi_retrieve_eventlog();
 
 	/* Ask the firmware to clear memory on unclean shutdown */
 	efi_enable_reset_attack_mitigation();
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 2c43d04e2b86..a47d226b632e 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -928,6 +928,8 @@ typedef struct {
 
 #define EFI_CC_BOOT_HASH_ALG_SHA384     0x00000004
 
+#define EFI_CC_EVENT_LOG_FORMAT_TCG_2	0x00000002
+
 typedef union efi_cc_protocol efi_cc_protocol_t;
 
 union efi_cc_protocol {
@@ -1134,7 +1136,7 @@ static inline void
 efi_enable_reset_attack_mitigation(void) { }
 #endif
 
-void efi_retrieve_tpm2_eventlog(void);
+void efi_retrieve_eventlog(void);
 
 struct screen_info *alloc_screen_info(void);
 struct screen_info *__alloc_screen_info(void);
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 7acbac16eae0..bcea7d520dad 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -47,39 +47,18 @@ void efi_enable_reset_attack_mitigation(void)
 
 #endif
 
-void efi_retrieve_tpm2_eventlog(void)
+static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_location,
+				       efi_physical_addr_t log_last_entry,
+				       efi_bool_t truncated)
 {
-	efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
 	efi_guid_t linux_eventlog_guid = LINUX_EFI_TPM_EVENT_LOG_GUID;
 	efi_status_t status;
-	efi_physical_addr_t log_location = 0, log_last_entry = 0;
 	struct linux_efi_tpm_eventlog *log_tbl = NULL;
 	struct efi_tcg2_final_events_table *final_events_table = NULL;
 	unsigned long first_entry_addr, last_entry_addr;
 	size_t log_size, last_entry_size;
-	efi_bool_t truncated;
-	int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
-	efi_tcg2_protocol_t *tcg2_protocol = NULL;
 	int final_events_size = 0;
 
-	status = efi_bs_call(locate_protocol, &tcg2_guid, NULL,
-			     (void **)&tcg2_protocol);
-	if (status != EFI_SUCCESS)
-		return;
-
-	status = efi_call_proto(tcg2_protocol, get_event_log, version,
-				&log_location, &log_last_entry, &truncated);
-
-	if (status != EFI_SUCCESS || !log_location) {
-		version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
-		status = efi_call_proto(tcg2_protocol, get_event_log, version,
-					&log_location, &log_last_entry,
-					&truncated);
-		if (status != EFI_SUCCESS || !log_location)
-			return;
-
-	}
-
 	first_entry_addr = (unsigned long) log_location;
 
 	/*
@@ -93,8 +72,11 @@ void efi_retrieve_tpm2_eventlog(void)
 		 * get_event_log only returns the address of the last entry.
 		 * We need to calculate its size to deduce the full size of
 		 * the logs.
+		 *
+		 * CC Event log also uses TCG2 format, handle it same as TPM2.
 		 */
-		if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+		if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2 ||
+		    version == EFI_CC_EVENT_LOG_FORMAT_TCG_2) {
 			/*
 			 * The TCG2 log format has variable length entries,
 			 * and the information to decode the hash algorithms
@@ -129,6 +111,8 @@ void efi_retrieve_tpm2_eventlog(void)
 	 */
 	if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
 		final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
+	else if (version == EFI_CC_EVENT_LOG_FORMAT_TCG_2)
+		final_events_table = get_efi_config_table(LINUX_EFI_CC_FINAL_LOG_GUID);
 	if (final_events_table && final_events_table->nr_events) {
 		struct tcg_pcr_event2_head *header;
 		int offset;
@@ -165,3 +149,47 @@ void efi_retrieve_tpm2_eventlog(void)
 err_free:
 	efi_bs_call(free_pool, log_tbl);
 }
+
+void efi_retrieve_eventlog(void)
+{
+	efi_physical_addr_t log_location = 0, log_last_entry = 0;
+	efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
+	efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
+	int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+	efi_tcg2_protocol_t *tpm2 = NULL;
+	efi_cc_protocol_t *cc = NULL;
+	efi_bool_t truncated;
+	efi_status_t status;
+
+	status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
+	if (status == EFI_SUCCESS) {
+		status = efi_call_proto(tpm2, get_event_log, version, &log_location,
+					&log_last_entry, &truncated);
+
+		if (status != EFI_SUCCESS || !log_location) {
+			version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+			status = efi_call_proto(tpm2, get_event_log, version,
+						&log_location, &log_last_entry,
+						&truncated);
+			if (status != EFI_SUCCESS || !log_location)
+				return;
+		}
+
+		efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
+					   truncated);
+		return;
+	}
+
+	status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
+	if (status == EFI_SUCCESS) {
+		version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
+		status = efi_call_proto(cc, get_event_log, version, &log_location,
+					&log_last_entry, &truncated);
+		if (status != EFI_SUCCESS || !log_location)
+			return;
+
+		efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
+					   truncated);
+		return;
+	}
+}
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 0d510c9a06a4..22641146970a 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -918,7 +918,7 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 
 	efi_random_get_seed();
 
-	efi_retrieve_tpm2_eventlog();
+	efi_retrieve_eventlog();
 
 	setup_graphics(boot_params);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 2f57fec2e629..a69c08b90e74 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -440,6 +440,9 @@ void efi_native_runtime_setup(void);
 /* OVMF protocol GUIDs */
 #define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID	EFI_GUID(0xc5a010fe, 0x38a7, 0x4531,  0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49)
 
+/* CC GUIDs */
+#define LINUX_EFI_CC_FINAL_LOG_GUID		EFI_GUID(0xdd4a4648, 0x2de7, 0x4665, 0x96, 0x4d, 0x21, 0xd9, 0xef, 0x5f, 0xb4, 0x46)
+
 typedef struct {
 	efi_guid_t guid;
 	u64 table;
-- 
2.25.1

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Ilias Apalodimas]

On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> To allow event log info access after boot, EFI boot stub extracts
> the event log information and installs it in an EFI configuration
> table. Currently, EFI boot stub only supports installation of event
> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
> support code as much as possible.
>
> 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>

[...]

> +void efi_retrieve_eventlog(void)
> +{
> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> +       efi_tcg2_protocol_t *tpm2 = NULL;
> +       efi_cc_protocol_t *cc = NULL;
> +       efi_bool_t truncated;
> +       efi_status_t status;
> +
> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
> +       if (status == EFI_SUCCESS) {
> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
> +                                       &log_last_entry, &truncated);
> +
> +               if (status != EFI_SUCCESS || !log_location) {
> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> +                       status = efi_call_proto(tpm2, get_event_log, version,
> +                                               &log_location, &log_last_entry,
> +                                               &truncated);
> +                       if (status != EFI_SUCCESS || !log_location)
> +                               return;
> +               }
> +
> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> +                                          truncated);
> +               return;
> +       }
> +
> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> +       if (status == EFI_SUCCESS) {
> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
> +                                       &log_last_entry, &truncated);
> +               if (status != EFI_SUCCESS || !log_location)
> +                       return;
> +
> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> +                                          truncated);
> +               return;
> +       }
> +}

[...]

I haven't looked into CC measurements much, but do we always want to
prioritize the tcg2 protocol? IOW if you have firmware that implements
both, shouldn't we prefer the CC protocol for VMs?

Thanks
/Ilias

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Kuppuswamy Sathyanarayanan]

Hi Ilias,

On 2/18/24 11:03 PM, Ilias Apalodimas wrote:
> On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> To allow event log info access after boot, EFI boot stub extracts
>> the event log information and installs it in an EFI configuration
>> table. Currently, EFI boot stub only supports installation of event
>> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
>> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
>> support code as much as possible.
>>
>> 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>
> [...]
>
>> +void efi_retrieve_eventlog(void)
>> +{
>> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
>> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
>> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
>> +       efi_tcg2_protocol_t *tpm2 = NULL;
>> +       efi_cc_protocol_t *cc = NULL;
>> +       efi_bool_t truncated;
>> +       efi_status_t status;
>> +
>> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
>> +       if (status == EFI_SUCCESS) {
>> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
>> +                                       &log_last_entry, &truncated);
>> +
>> +               if (status != EFI_SUCCESS || !log_location) {
>> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>> +                       status = efi_call_proto(tpm2, get_event_log, version,
>> +                                               &log_location, &log_last_entry,
>> +                                               &truncated);
>> +                       if (status != EFI_SUCCESS || !log_location)
>> +                               return;
>> +               }
>> +
>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>> +                                          truncated);
>> +               return;
>> +       }
>> +
>> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>> +       if (status == EFI_SUCCESS) {
>> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
>> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
>> +                                       &log_last_entry, &truncated);
>> +               if (status != EFI_SUCCESS || !log_location)
>> +                       return;
>> +
>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>> +                                          truncated);
>> +               return;
>> +       }
>> +}
> [...]
>
> I haven't looked into CC measurements much, but do we always want to
> prioritize the tcg2 protocol? IOW if you have firmware that implements
> both, shouldn't we prefer the CC protocol for VMs?

According the UEFI specification, sec "Conidential computing", if a firmware implements
the TPM, then it should be used and CC interfaces should not be published. So I think
we should check for TPM first, if it does not exist then try for CC.

https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#confidential-computing

> Thanks
> /Ilias

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Ilias Apalodimas]

Apologies for the late reply,


On Mon, 19 Feb 2024 at 09:34, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Hi Ilias,
>
> On 2/18/24 11:03 PM, Ilias Apalodimas wrote:
> > On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >> To allow event log info access after boot, EFI boot stub extracts
> >> the event log information and installs it in an EFI configuration
> >> table. Currently, EFI boot stub only supports installation of event
> >> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
> >> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
> >> support code as much as possible.
> >>
> >> 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>
> > [...]
> >
> >> +void efi_retrieve_eventlog(void)
> >> +{
> >> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
> >> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> >> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
> >> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> >> +       efi_tcg2_protocol_t *tpm2 = NULL;
> >> +       efi_cc_protocol_t *cc = NULL;
> >> +       efi_bool_t truncated;
> >> +       efi_status_t status;
> >> +
> >> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
> >> +       if (status == EFI_SUCCESS) {
> >> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
> >> +                                       &log_last_entry, &truncated);
> >> +
> >> +               if (status != EFI_SUCCESS || !log_location) {
> >> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >> +                       status = efi_call_proto(tpm2, get_event_log, version,
> >> +                                               &log_location, &log_last_entry,
> >> +                                               &truncated);
> >> +                       if (status != EFI_SUCCESS || !log_location)
> >> +                               return;
> >> +               }
> >> +
> >> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> >> +                                          truncated);
> >> +               return;
> >> +       }
> >> +
> >> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> >> +       if (status == EFI_SUCCESS) {
> >> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
> >> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
> >> +                                       &log_last_entry, &truncated);
> >> +               if (status != EFI_SUCCESS || !log_location)
> >> +                       return;
> >> +
> >> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> >> +                                          truncated);
> >> +               return;
> >> +       }
> >> +}
> > [...]
> >
> > I haven't looked into CC measurements much, but do we always want to
> > prioritize the tcg2 protocol? IOW if you have firmware that implements
> > both, shouldn't we prefer the CC protocol for VMs?
>
> According the UEFI specification, sec "Conidential computing", if a firmware implements
> the TPM, then it should be used and CC interfaces should not be published. So I think
> we should check for TPM first, if it does not exist then try for CC.

Ok thanks, that makes sense. That document also says the services
should be implemented on a virtual firmware.
I am unsure at the moment though if it's worth checking that and
reporting an error otherwise. Thoughts?

Thanks
/Ilias
>
> https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#confidential-computing
>
> > Thanks
> > /Ilias
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Kuppuswamy Sathyanarayanan]

On 2/23/24 5:24 AM, Ilias Apalodimas wrote:
> Apologies for the late reply,
>
>
> On Mon, 19 Feb 2024 at 09:34, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>> Hi Ilias,
>>
>> On 2/18/24 11:03 PM, Ilias Apalodimas wrote:
>>> On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>> To allow event log info access after boot, EFI boot stub extracts
>>>> the event log information and installs it in an EFI configuration
>>>> table. Currently, EFI boot stub only supports installation of event
>>>> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
>>>> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
>>>> support code as much as possible.
>>>>
>>>> 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>
>>> [...]
>>>
>>>> +void efi_retrieve_eventlog(void)
>>>> +{
>>>> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
>>>> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>>>> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
>>>> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
>>>> +       efi_tcg2_protocol_t *tpm2 = NULL;
>>>> +       efi_cc_protocol_t *cc = NULL;
>>>> +       efi_bool_t truncated;
>>>> +       efi_status_t status;
>>>> +
>>>> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
>>>> +       if (status == EFI_SUCCESS) {
>>>> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
>>>> +                                       &log_last_entry, &truncated);
>>>> +
>>>> +               if (status != EFI_SUCCESS || !log_location) {
>>>> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>> +                       status = efi_call_proto(tpm2, get_event_log, version,
>>>> +                                               &log_location, &log_last_entry,
>>>> +                                               &truncated);
>>>> +                       if (status != EFI_SUCCESS || !log_location)
>>>> +                               return;
>>>> +               }
>>>> +
>>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>>>> +                                          truncated);
>>>> +               return;
>>>> +       }
>>>> +
>>>> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>>>> +       if (status == EFI_SUCCESS) {
>>>> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
>>>> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
>>>> +                                       &log_last_entry, &truncated);
>>>> +               if (status != EFI_SUCCESS || !log_location)
>>>> +                       return;
>>>> +
>>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>>>> +                                          truncated);
>>>> +               return;
>>>> +       }
>>>> +}
>>> [...]
>>>
>>> I haven't looked into CC measurements much, but do we always want to
>>> prioritize the tcg2 protocol? IOW if you have firmware that implements
>>> both, shouldn't we prefer the CC protocol for VMs?
>> According the UEFI specification, sec "Conidential computing", if a firmware implements
>> the TPM, then it should be used and CC interfaces should not be published. So I think
>> we should check for TPM first, if it does not exist then try for CC.
> Ok thanks, that makes sense. That document also says the services
> should be implemented on a virtual firmware.
> I am unsure at the moment though if it's worth checking that and
> reporting an error otherwise. Thoughts?

IMO, it is not fatal for the firmware to implement both protocols. Although, it
violates the specification, does it makes sense to return error and skip
measurements? I think for such case, we can add a warning and proceed
with TPM if it exists.

>
> Thanks
> /Ilias
>> https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#confidential-computing
>>
>>> Thanks
>>> /Ilias
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Ilias Apalodimas]

On Sat, 24 Feb 2024 at 09:31, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 2/23/24 5:24 AM, Ilias Apalodimas wrote:
> > Apologies for the late reply,
> >
> >
> > On Mon, 19 Feb 2024 at 09:34, Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >> Hi Ilias,
> >>
> >> On 2/18/24 11:03 PM, Ilias Apalodimas wrote:
> >>> On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
> >>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >>>> To allow event log info access after boot, EFI boot stub extracts
> >>>> the event log information and installs it in an EFI configuration
> >>>> table. Currently, EFI boot stub only supports installation of event
> >>>> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
> >>>> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
> >>>> support code as much as possible.
> >>>>
> >>>> 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>
> >>> [...]
> >>>
> >>>> +void efi_retrieve_eventlog(void)
> >>>> +{
> >>>> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
> >>>> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> >>>> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
> >>>> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> >>>> +       efi_tcg2_protocol_t *tpm2 = NULL;
> >>>> +       efi_cc_protocol_t *cc = NULL;
> >>>> +       efi_bool_t truncated;
> >>>> +       efi_status_t status;
> >>>> +
> >>>> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
> >>>> +       if (status == EFI_SUCCESS) {
> >>>> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
> >>>> +                                       &log_last_entry, &truncated);
> >>>> +
> >>>> +               if (status != EFI_SUCCESS || !log_location) {
> >>>> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> >>>> +                       status = efi_call_proto(tpm2, get_event_log, version,
> >>>> +                                               &log_location, &log_last_entry,
> >>>> +                                               &truncated);
> >>>> +                       if (status != EFI_SUCCESS || !log_location)
> >>>> +                               return;
> >>>> +               }
> >>>> +
> >>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> >>>> +                                          truncated);
> >>>> +               return;
> >>>> +       }
> >>>> +
> >>>> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> >>>> +       if (status == EFI_SUCCESS) {
> >>>> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
> >>>> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
> >>>> +                                       &log_last_entry, &truncated);
> >>>> +               if (status != EFI_SUCCESS || !log_location)
> >>>> +                       return;
> >>>> +
> >>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> >>>> +                                          truncated);
> >>>> +               return;
> >>>> +       }
> >>>> +}
> >>> [...]
> >>>
> >>> I haven't looked into CC measurements much, but do we always want to
> >>> prioritize the tcg2 protocol? IOW if you have firmware that implements
> >>> both, shouldn't we prefer the CC protocol for VMs?
> >> According the UEFI specification, sec "Conidential computing", if a firmware implements
> >> the TPM, then it should be used and CC interfaces should not be published. So I think
> >> we should check for TPM first, if it does not exist then try for CC.
> > Ok thanks, that makes sense. That document also says the services
> > should be implemented on a virtual firmware.
> > I am unsure at the moment though if it's worth checking that and
> > reporting an error otherwise. Thoughts?
>
> IMO, it is not fatal for the firmware to implement both protocols. Although, it
> violates the specification, does it makes sense to return error and skip
> measurements? I think for such case, we can add a warning and proceed
> with TPM if it exists.

If you have a TPM, the current code wouldn't even look for CC (which
we agreed is correct).
The question is, should we care if a firmware exposes the CC protocol,
but isn't virtualized

Thanks
/Ilias
>
> >
> > Thanks
> > /Ilias
> >> https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#confidential-computing
> >>
> >>> Thanks
> >>> /Ilias
> >> --
> >> Sathyanarayanan Kuppuswamy
> >> Linux Kernel Developer
> >>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>

Re: [PATCH v2 2/2] efi/libstub: Add get_event_log() support for CC platforms

[Kuppuswamy Sathyanarayanan]

On 2/27/24 5:19 AM, Ilias Apalodimas wrote:
> On Sat, 24 Feb 2024 at 09:31, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 2/23/24 5:24 AM, Ilias Apalodimas wrote:
>>> Apologies for the late reply,
>>>
>>>
>>> On Mon, 19 Feb 2024 at 09:34, Kuppuswamy Sathyanarayanan
>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>> Hi Ilias,
>>>>
>>>> On 2/18/24 11:03 PM, Ilias Apalodimas wrote:
>>>>> On Thu, 15 Feb 2024 at 05:02, Kuppuswamy Sathyanarayanan
>>>>> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>>>>> To allow event log info access after boot, EFI boot stub extracts
>>>>>> the event log information and installs it in an EFI configuration
>>>>>> table. Currently, EFI boot stub only supports installation of event
>>>>>> log only for TPM 1.2 and TPM 2.0 protocols. Extend the same support
>>>>>> for CC protocol. Since CC platform also uses TCG2 format, reuse TPM2
>>>>>> support code as much as possible.
>>>>>>
>>>>>> 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>
>>>>> [...]
>>>>>
>>>>>> +void efi_retrieve_eventlog(void)
>>>>>> +{
>>>>>> +       efi_physical_addr_t log_location = 0, log_last_entry = 0;
>>>>>> +       efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>>>>>> +       efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
>>>>>> +       int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
>>>>>> +       efi_tcg2_protocol_t *tpm2 = NULL;
>>>>>> +       efi_cc_protocol_t *cc = NULL;
>>>>>> +       efi_bool_t truncated;
>>>>>> +       efi_status_t status;
>>>>>> +
>>>>>> +       status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
>>>>>> +       if (status == EFI_SUCCESS) {
>>>>>> +               status = efi_call_proto(tpm2, get_event_log, version, &log_location,
>>>>>> +                                       &log_last_entry, &truncated);
>>>>>> +
>>>>>> +               if (status != EFI_SUCCESS || !log_location) {
>>>>>> +                       version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
>>>>>> +                       status = efi_call_proto(tpm2, get_event_log, version,
>>>>>> +                                               &log_location, &log_last_entry,
>>>>>> +                                               &truncated);
>>>>>> +                       if (status != EFI_SUCCESS || !log_location)
>>>>>> +                               return;
>>>>>> +               }
>>>>>> +
>>>>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>>>>>> +                                          truncated);
>>>>>> +               return;
>>>>>> +       }
>>>>>> +
>>>>>> +       status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>>>>>> +       if (status == EFI_SUCCESS) {
>>>>>> +               version = EFI_CC_EVENT_LOG_FORMAT_TCG_2;
>>>>>> +               status = efi_call_proto(cc, get_event_log, version, &log_location,
>>>>>> +                                       &log_last_entry, &truncated);
>>>>>> +               if (status != EFI_SUCCESS || !log_location)
>>>>>> +                       return;
>>>>>> +
>>>>>> +               efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>>>>>> +                                          truncated);
>>>>>> +               return;
>>>>>> +       }
>>>>>> +}
>>>>> [...]
>>>>>
>>>>> I haven't looked into CC measurements much, but do we always want to
>>>>> prioritize the tcg2 protocol? IOW if you have firmware that implements
>>>>> both, shouldn't we prefer the CC protocol for VMs?
>>>> According the UEFI specification, sec "Conidential computing", if a firmware implements
>>>> the TPM, then it should be used and CC interfaces should not be published. So I think
>>>> we should check for TPM first, if it does not exist then try for CC.
>>> Ok thanks, that makes sense. That document also says the services
>>> should be implemented on a virtual firmware.
>>> I am unsure at the moment though if it's worth checking that and
>>> reporting an error otherwise. Thoughts?
>> IMO, it is not fatal for the firmware to implement both protocols. Although, it
>> violates the specification, does it makes sense to return error and skip
>> measurements? I think for such case, we can add a warning and proceed
>> with TPM if it exists.
> If you have a TPM, the current code wouldn't even look for CC (which
> we agreed is correct).
> The question is, should we care if a firmware exposes the CC protocol,
> but isn't virtualized

AFAIK, even if a firmware improperly uses this protocol (in a non-virtual
environment), it should not be a fatal issue. So, if we add such a check,
it will be just a spec compliance check. Also, a firmware can improperly
use any existing EFI interfaces in n other ways. But, we cannot check for
all such cases, right? So personally I think it is not needed. But I am fine
either way.

If we want to add such check, I think we should either cc_platform_has()
or CPU feature flag check for it.

>
> Thanks
> /Ilias
>>> Thanks
>>> /Ilias
>>>> https://uefi.org/specs/UEFI/2.10/38_Confidential_Computing.html#confidential-computing
>>>>
>>>>> Thanks
>>>>> /Ilias
>>>> --
>>>> Sathyanarayanan Kuppuswamy
>>>> Linux Kernel Developer
>>>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer