* [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement
@ 2024-03-04 10:44 Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-04 10:44 UTC (permalink / raw)
To: linux-efi; +Cc: Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
From: Ard Biesheuvel <ardb@kernel.org>
This is a follow-up to Kuppuswamy's series [0] to add TDX based
measurement of the initrd and command line to the EFI stub.
In view of time wrt the merge window, I've taken the liberty of applying
my review feedback to the code directly, rather than going back and
forth with the author to converge on something we can both agree on.
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
[0] https://lore.kernel.org/all/20240215030002.281456-1-sathyanarayanan.kuppuswamy@linux.intel.com/
Ard Biesheuvel (2):
efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event
efi/libstub: Measure into CC protocol if TCG2 protocol is absent
Kuppuswamy Sathyanarayanan (2):
efi/libstub: Add Confidential Computing (CC) measurement typedefs
efi/libstub: Add get_event_log() support for CC platforms
drivers/firmware/efi/libstub/efi-stub-helper.c | 83 ++++++++++------
drivers/firmware/efi/libstub/efi-stub.c | 2 +-
drivers/firmware/efi/libstub/efistub.h | 104 ++++++++++++++++++--
drivers/firmware/efi/libstub/tpm.c | 74 +++++++++-----
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
include/linux/efi.h | 4 +
6 files changed, 206 insertions(+), 63 deletions(-)
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event
2024-03-04 10:44 [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement Ard Biesheuvel
@ 2024-03-04 10:44 ` Ard Biesheuvel
2024-03-05 4:30 ` Kuppuswamy Sathyanarayanan
2024-03-04 10:44 ` [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-04 10:44 UTC (permalink / raw)
To: linux-efi; +Cc: Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
From: Ard Biesheuvel <ardb@kernel.org>
In spite of the efi_ prefix, struct efi_tcg2_tagged_event is specific to
the EFI stub, and so we can tweak it to our liking if needed, e.g., to
accommodate the TDX variant of the TCG2 measurement protocol.
In preparation for that, get rid of it entirely, and combine it with the
efi_measured_event struct used by the measurement code.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/efi-stub-helper.c | 26 ++++++++------------
drivers/firmware/efi/libstub/efistub.h | 18 ++++++++------
2 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index bfa30625f5d0..0dbc9d3f4abd 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -193,7 +193,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
*load_options_size = load_option_unpacked.optional_data_size;
}
-enum efistub_event {
+enum efistub_event_type {
EFISTUB_EVT_INITRD,
EFISTUB_EVT_LOAD_OPTIONS,
EFISTUB_EVT_COUNT,
@@ -221,44 +221,38 @@ static const struct {
static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
unsigned long load_size,
- enum efistub_event event)
+ enum efistub_event_type event)
{
+ struct efistub_measured_event *evt;
+ int size = struct_size(evt, tagged_event_data,
+ events[event].event_data_len);
efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
efi_tcg2_protocol_t *tcg2 = NULL;
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);
if (status != EFI_SUCCESS)
goto fail;
- evt->event_data = (struct efi_tcg2_event){
+ evt->event_data.tcg2_data = (struct efi_tcg2_event){
.event_size = size,
- .event_header.header_size = sizeof(evt->event_data.event_header),
+ .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
.event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
.event_header.pcr_index = events[event].pcr_index,
.event_header.event_type = EV_EVENT_TAG,
};
- evt->tagged_event = (struct efi_tcg2_tagged_event){
- .tagged_event_id = events[event].event_id,
- .tagged_event_data_size = events[event].event_data_len,
- };
+ evt->tagged_event_id = events[event].event_id;
+ evt->tagged_event_data_size = events[event].event_data_len;
memcpy(evt->tagged_event_data, events[event].event_data,
events[event].event_data_len);
status = efi_call_proto(tcg2, hash_log_extend_event, 0,
- load_addr, load_size, &evt->event_data);
+ load_addr, load_size, &evt->event_data.tcg2_data);
efi_bs_call(free_pool, evt);
if (status != EFI_SUCCESS)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index c04b82ea40f2..b2c50dce48b8 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -843,14 +843,7 @@ struct efi_tcg2_event {
/* u8[] event follows here */
} __packed;
-struct efi_tcg2_tagged_event {
- u32 tagged_event_id;
- u32 tagged_event_data_size;
- /* u8 tagged event data follows here */
-} __packed;
-
typedef struct efi_tcg2_event efi_tcg2_event_t;
-typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
union efi_tcg2_protocol {
@@ -882,6 +875,17 @@ union efi_tcg2_protocol {
} mixed_mode;
};
+union efistub_event {
+ efi_tcg2_event_t tcg2_data;
+};
+
+struct efistub_measured_event {
+ union efistub_event event_data;
+ u32 tagged_event_id;
+ u32 tagged_event_data_size;
+ u8 tagged_event_data[];
+} __packed;
+
struct riscv_efi_boot_protocol {
u64 revision;
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs
2024-03-04 10:44 [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event Ard Biesheuvel
@ 2024-03-04 10:44 ` Ard Biesheuvel
2024-03-05 18:00 ` Ilias Apalodimas
2024-03-04 10:44 ` [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 4/4] efi/libstub: Add get_event_log() support for CC platforms Ard Biesheuvel
3 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-04 10:44 UTC (permalink / raw)
To: linux-efi; +Cc: Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
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.
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>
[ardb: Drop code changes, keep typedefs and #define's only]
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/efistub.h | 79 ++++++++++++++++++++
include/linux/efi.h | 1 +
2 files changed, 80 insertions(+)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index b2c50dce48b8..d621bfb719c4 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -875,6 +875,85 @@ 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;
+};
+
union efistub_event {
efi_tcg2_event_t tcg2_data;
};
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.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-04 10:44 [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs Ard Biesheuvel
@ 2024-03-04 10:44 ` Ard Biesheuvel
2024-03-05 17:34 ` Dionna Amalie Glaze
2024-03-05 21:39 ` Kuppuswamy Sathyanarayanan
2024-03-04 10:44 ` [PATCH 4/4] efi/libstub: Add get_event_log() support for CC platforms Ard Biesheuvel
3 siblings, 2 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-04 10:44 UTC (permalink / raw)
To: linux-efi; +Cc: Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
From: Ard Biesheuvel <ardb@kernel.org>
To accommodate confidential compute VMs that expose the simplified CC
measurement protocol instead of the full-blown TCG2 one, fall back to
the former if the latter does not exist.
The CC protocol was designed to be used in this manner, which is why the
types and prototypes have been kept the same where possible. So reuse
the existing code, and only deviate from the TCG2 code path where
needed.
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
drivers/firmware/efi/libstub/efistub.h | 3 +
2 files changed, 53 insertions(+), 17 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 0dbc9d3f4abd..21f4567324f6 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
unsigned long load_size,
enum efistub_event_type event)
{
+ union {
+ efi_status_t
+ (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
+ u64, const union efistub_event *);
+ struct { u32 hash_log_extend_event; } mixed_mode;
+ } method;
struct efistub_measured_event *evt;
int size = struct_size(evt, tagged_event_data,
events[event].event_data_len);
efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
efi_tcg2_protocol_t *tcg2 = NULL;
+ union efistub_event ev;
efi_status_t status;
+ void *protocol;
efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
if (tcg2) {
- status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
- (void **)&evt);
- if (status != EFI_SUCCESS)
- goto fail;
-
- evt->event_data.tcg2_data = (struct efi_tcg2_event){
+ ev.tcg2_data = (struct efi_tcg2_event){
.event_size = size,
- .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
+ .event_header.header_size = sizeof(ev.tcg2_data.event_header),
.event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
.event_header.pcr_index = events[event].pcr_index,
.event_header.event_type = EV_EVENT_TAG,
};
+ protocol = tcg2;
+ method.hash_log_extend_event =
+ (void *)efi_table_attr(tcg2, hash_log_extend_event);
+ } else {
+ efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
+ efi_cc_protocol_t *cc = NULL;
- evt->tagged_event_id = events[event].event_id;
- evt->tagged_event_data_size = events[event].event_data_len;
-
- memcpy(evt->tagged_event_data, events[event].event_data,
- events[event].event_data_len);
+ efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
+ if (!cc)
+ return EFI_UNSUPPORTED;
- status = efi_call_proto(tcg2, hash_log_extend_event, 0,
- load_addr, load_size, &evt->event_data.tcg2_data);
- efi_bs_call(free_pool, evt);
+ ev.cc_data = (struct efi_cc_event){
+ .event_size = size,
+ .event_header.header_size = sizeof(ev.cc_data.event_header),
+ .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
+ .event_header.event_type = EV_EVENT_TAG,
+ };
+ status = efi_call_proto(cc, map_pcr_to_mr_index,
+ events[event].pcr_index,
+ &ev.cc_data.event_header.mr_index);
if (status != EFI_SUCCESS)
goto fail;
- return EFI_SUCCESS;
+
+ protocol = cc;
+ method.hash_log_extend_event =
+ (void *)efi_table_attr(cc, hash_log_extend_event);
}
- return EFI_UNSUPPORTED;
+ status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
+ if (status != EFI_SUCCESS)
+ goto fail;
+
+ evt->event_data = ev;
+ evt->tagged_event_id = events[event].event_id;
+ evt->tagged_event_data_size = events[event].event_data_len;
+
+ memcpy(evt->tagged_event_data, events[event].event_data,
+ events[event].event_data_len);
+
+ status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
+ load_addr, load_size, &evt->event_data);
+ efi_bs_call(free_pool, evt);
+
+ if (status == EFI_SUCCESS)
+ return EFI_SUCCESS;
+
fail:
efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
return status;
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index d621bfb719c4..4bf9a76796b7 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -954,8 +954,11 @@ union efi_cc_protocol {
} mixed_mode;
};
+static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
+
union efistub_event {
efi_tcg2_event_t tcg2_data;
+ efi_cc_event_t cc_data;
};
struct efistub_measured_event {
--
2.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/4] efi/libstub: Add get_event_log() support for CC platforms
2024-03-04 10:44 [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement Ard Biesheuvel
` (2 preceding siblings ...)
2024-03-04 10:44 ` [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent Ard Biesheuvel
@ 2024-03-04 10:44 ` Ard Biesheuvel
3 siblings, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-04 10:44 UTC (permalink / raw)
To: linux-efi; +Cc: Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
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>
[ardb: reorganize logic slightly to share more code]
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
drivers/firmware/efi/libstub/efi-stub.c | 2 +-
drivers/firmware/efi/libstub/efistub.h | 4 +-
drivers/firmware/efi/libstub/tpm.c | 74 +++++++++++++-------
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
include/linux/efi.h | 3 +
5 files changed, 57 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 4bf9a76796b7..e0a2766a70c0 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -922,6 +922,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 {
@@ -1147,7 +1149,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..c35f99f259c1 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,43 @@ 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 tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
+ int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+ efi_tcg2_protocol_t *tpm2 = 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);
+ }
+ } else {
+ efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
+ efi_cc_protocol_t *cc = NULL;
+
+ status = efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
+ if (status != EFI_SUCCESS)
+ return;
+
+ 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);
+}
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 99429bc4b0c7..d09aa13c7ff0 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -923,7 +923,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.44.0.278.ge034bb2e1d-goog
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event
2024-03-04 10:44 ` [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event Ard Biesheuvel
@ 2024-03-05 4:30 ` Kuppuswamy Sathyanarayanan
2024-03-05 8:21 ` Ard Biesheuvel
0 siblings, 1 reply; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 4:30 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi; +Cc: Ard Biesheuvel, Ilias Apalodimas
On 3/4/24 2:44 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
>
> In spite of the efi_ prefix, struct efi_tcg2_tagged_event is specific to
> the EFI stub, and so we can tweak it to our liking if needed, e.g., to
> accommodate the TDX variant of the TCG2 measurement protocol.
>
> In preparation for that, get rid of it entirely, and combine it with the
> efi_measured_event struct used by the measurement code.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 26 ++++++++------------
> drivers/firmware/efi/libstub/efistub.h | 18 ++++++++------
> 2 files changed, 21 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index bfa30625f5d0..0dbc9d3f4abd 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -193,7 +193,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
> *load_options_size = load_option_unpacked.optional_data_size;
> }
>
> -enum efistub_event {
> +enum efistub_event_type {
> EFISTUB_EVT_INITRD,
> EFISTUB_EVT_LOAD_OPTIONS,
> EFISTUB_EVT_COUNT,
> @@ -221,44 +221,38 @@ static const struct {
>
> static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> unsigned long load_size,
> - enum efistub_event event)
> + enum efistub_event_type event)
> {
> + struct efistub_measured_event *evt;
> + int size = struct_size(evt, tagged_event_data,
> + events[event].event_data_len);
Include linux/overflow.h explicitly?
> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> 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);
It looks like in patch 3 you have converted evt as stack variable. Since that
change is not specific to CC fallback, can it be moved here?
> if (status != EFI_SUCCESS)
> goto fail;
>
> - evt->event_data = (struct efi_tcg2_event){
> + evt->event_data.tcg2_data = (struct efi_tcg2_event){
> .event_size = size,
> - .event_header.header_size = sizeof(evt->event_data.event_header),
> + .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> .event_header.pcr_index = events[event].pcr_index,
> .event_header.event_type = EV_EVENT_TAG,
> };
>
> - evt->tagged_event = (struct efi_tcg2_tagged_event){
> - .tagged_event_id = events[event].event_id,
> - .tagged_event_data_size = events[event].event_data_len,
> - };
> + evt->tagged_event_id = events[event].event_id;
> + evt->tagged_event_data_size = events[event].event_data_len;
>
> memcpy(evt->tagged_event_data, events[event].event_data,
> events[event].event_data_len);
>
> status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> - load_addr, load_size, &evt->event_data);
> + load_addr, load_size, &evt->event_data.tcg2_data);
> efi_bs_call(free_pool, evt);
>
> if (status != EFI_SUCCESS)
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index c04b82ea40f2..b2c50dce48b8 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -843,14 +843,7 @@ struct efi_tcg2_event {
> /* u8[] event follows here */
> } __packed;
>
> -struct efi_tcg2_tagged_event {
> - u32 tagged_event_id;
> - u32 tagged_event_data_size;
> - /* u8 tagged event data follows here */
> -} __packed;
> -
> typedef struct efi_tcg2_event efi_tcg2_event_t;
> -typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
> typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
>
> union efi_tcg2_protocol {
> @@ -882,6 +875,17 @@ union efi_tcg2_protocol {
> } mixed_mode;
> };
>
> +union efistub_event {
> + efi_tcg2_event_t tcg2_data;
> +};
> +
> +struct efistub_measured_event {
> + union efistub_event event_data;
> + u32 tagged_event_id;
> + u32 tagged_event_data_size;
> + u8 tagged_event_data[];
> +} __packed;
> +
Since efistub_measured_event is only used efi-stub-helper.c, why
not leave it there?
> struct riscv_efi_boot_protocol {
> u64 revision;
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event
2024-03-05 4:30 ` Kuppuswamy Sathyanarayanan
@ 2024-03-05 8:21 ` Ard Biesheuvel
2024-03-05 19:19 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-05 8:21 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: Ard Biesheuvel, linux-efi, Ilias Apalodimas
On Tue, 5 Mar 2024 at 05:30, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 3/4/24 2:44 AM, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > In spite of the efi_ prefix, struct efi_tcg2_tagged_event is specific to
> > the EFI stub, and so we can tweak it to our liking if needed, e.g., to
> > accommodate the TDX variant of the TCG2 measurement protocol.
> >
> > In preparation for that, get rid of it entirely, and combine it with the
> > efi_measured_event struct used by the measurement code.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > drivers/firmware/efi/libstub/efi-stub-helper.c | 26 ++++++++------------
> > drivers/firmware/efi/libstub/efistub.h | 18 ++++++++------
> > 2 files changed, 21 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index bfa30625f5d0..0dbc9d3f4abd 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -193,7 +193,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
> > *load_options_size = load_option_unpacked.optional_data_size;
> > }
> >
> > -enum efistub_event {
> > +enum efistub_event_type {
> > EFISTUB_EVT_INITRD,
> > EFISTUB_EVT_LOAD_OPTIONS,
> > EFISTUB_EVT_COUNT,
> > @@ -221,44 +221,38 @@ static const struct {
> >
> > static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> > unsigned long load_size,
> > - enum efistub_event event)
> > + enum efistub_event_type event)
> > {
> > + struct efistub_measured_event *evt;
> > + int size = struct_size(evt, tagged_event_data,
> > + events[event].event_data_len);
>
> Include linux/overflow.h explicitly?
>
Yes, good point.
> > efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> > 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);
>
> It looks like in patch 3 you have converted evt as stack variable. Since that
> change is not specific to CC fallback, can it be moved here?
>
Not sure what you mean here. evt is still there after parch #3
> > if (status != EFI_SUCCESS)
> > goto fail;
> >
> > - evt->event_data = (struct efi_tcg2_event){
> > + evt->event_data.tcg2_data = (struct efi_tcg2_event){
> > .event_size = size,
> > - .event_header.header_size = sizeof(evt->event_data.event_header),
> > + .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> > .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> > .event_header.pcr_index = events[event].pcr_index,
> > .event_header.event_type = EV_EVENT_TAG,
> > };
> >
> > - evt->tagged_event = (struct efi_tcg2_tagged_event){
> > - .tagged_event_id = events[event].event_id,
> > - .tagged_event_data_size = events[event].event_data_len,
> > - };
> > + evt->tagged_event_id = events[event].event_id;
> > + evt->tagged_event_data_size = events[event].event_data_len;
> >
> > memcpy(evt->tagged_event_data, events[event].event_data,
> > events[event].event_data_len);
> >
> > status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> > - load_addr, load_size, &evt->event_data);
> > + load_addr, load_size, &evt->event_data.tcg2_data);
> > efi_bs_call(free_pool, evt);
> >
> > if (status != EFI_SUCCESS)
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index c04b82ea40f2..b2c50dce48b8 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -843,14 +843,7 @@ struct efi_tcg2_event {
> > /* u8[] event follows here */
> > } __packed;
> >
> > -struct efi_tcg2_tagged_event {
> > - u32 tagged_event_id;
> > - u32 tagged_event_data_size;
> > - /* u8 tagged event data follows here */
> > -} __packed;
> > -
> > typedef struct efi_tcg2_event efi_tcg2_event_t;
> > -typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
> > typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
> >
> > union efi_tcg2_protocol {
> > @@ -882,6 +875,17 @@ union efi_tcg2_protocol {
> > } mixed_mode;
> > };
> >
> > +union efistub_event {
> > + efi_tcg2_event_t tcg2_data;
> > +};
> > +
> > +struct efistub_measured_event {
> > + union efistub_event event_data;
> > + u32 tagged_event_id;
> > + u32 tagged_event_data_size;
> > + u8 tagged_event_data[];
> > +} __packed;
> > +
>
> Since efistub_measured_event is only used efi-stub-helper.c, why
> not leave it there?
>
Indeed. I will move it back.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-04 10:44 ` [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent Ard Biesheuvel
@ 2024-03-05 17:34 ` Dionna Amalie Glaze
2024-03-05 17:47 ` Ard Biesheuvel
2024-03-05 21:39 ` Kuppuswamy Sathyanarayanan
1 sibling, 1 reply; 21+ messages in thread
From: Dionna Amalie Glaze @ 2024-03-05 17:34 UTC (permalink / raw)
To: Ard Biesheuvel, Qinkun Bao
Cc: linux-efi, Ard Biesheuvel, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Ard Biesheuvel <ardb@kernel.org>
>
> To accommodate confidential compute VMs that expose the simplified CC
> measurement protocol instead of the full-blown TCG2 one, fall back to
> the former if the latter does not exist.
>
> The CC protocol was designed to be used in this manner, which is why the
> types and prototypes have been kept the same where possible. So reuse
> the existing code, and only deviate from the TCG2 code path where
> needed.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> drivers/firmware/efi/libstub/efistub.h | 3 +
> 2 files changed, 53 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 0dbc9d3f4abd..21f4567324f6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> unsigned long load_size,
> enum efistub_event_type event)
> {
> + union {
> + efi_status_t
> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> + u64, const union efistub_event *);
> + struct { u32 hash_log_extend_event; } mixed_mode;
> + } method;
> struct efistub_measured_event *evt;
> int size = struct_size(evt, tagged_event_data,
> events[event].event_data_len);
> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> efi_tcg2_protocol_t *tcg2 = NULL;
> + union efistub_event ev;
> efi_status_t status;
> + void *protocol;
>
> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> if (tcg2) {
> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> - (void **)&evt);
> - if (status != EFI_SUCCESS)
> - goto fail;
> -
> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> + ev.tcg2_data = (struct efi_tcg2_event){
> .event_size = size,
> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> .event_header.pcr_index = events[event].pcr_index,
> .event_header.event_type = EV_EVENT_TAG,
> };
> + protocol = tcg2;
> + method.hash_log_extend_event =
> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> + } else {
+Qinkun Bao
Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
TCG protocol breaks backwards compatibility, it'd be preferable to
measure into all the measurement protocols that are present. The UEFI
v2.10 standard says that firmware "should not" provide both, but it is
not MUST NOT. Given this and our desire to provide service continuity,
I ask that you remove the "else" guard.
> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> + efi_cc_protocol_t *cc = NULL;
>
> - evt->tagged_event_id = events[event].event_id;
> - evt->tagged_event_data_size = events[event].event_data_len;
> -
> - memcpy(evt->tagged_event_data, events[event].event_data,
> - events[event].event_data_len);
> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> + if (!cc)
> + return EFI_UNSUPPORTED;
>
> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> - load_addr, load_size, &evt->event_data.tcg2_data);
> - efi_bs_call(free_pool, evt);
> + ev.cc_data = (struct efi_cc_event){
> + .event_size = size,
> + .event_header.header_size = sizeof(ev.cc_data.event_header),
> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> + .event_header.event_type = EV_EVENT_TAG,
> + };
>
> + status = efi_call_proto(cc, map_pcr_to_mr_index,
> + events[event].pcr_index,
> + &ev.cc_data.event_header.mr_index);
> if (status != EFI_SUCCESS)
> goto fail;
> - return EFI_SUCCESS;
> +
> + protocol = cc;
> + method.hash_log_extend_event =
> + (void *)efi_table_attr(cc, hash_log_extend_event);
> }
>
> - return EFI_UNSUPPORTED;
> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> + if (status != EFI_SUCCESS)
> + goto fail;
> +
> + evt->event_data = ev;
> + evt->tagged_event_id = events[event].event_id;
> + evt->tagged_event_data_size = events[event].event_data_len;
> +
> + memcpy(evt->tagged_event_data, events[event].event_data,
> + events[event].event_data_len);
> +
> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> + load_addr, load_size, &evt->event_data);
> + efi_bs_call(free_pool, evt);
> +
> + if (status == EFI_SUCCESS)
> + return EFI_SUCCESS;
> +
> fail:
> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> return status;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index d621bfb719c4..4bf9a76796b7 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -954,8 +954,11 @@ union efi_cc_protocol {
> } mixed_mode;
> };
>
> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> +
> union efistub_event {
> efi_tcg2_event_t tcg2_data;
> + efi_cc_event_t cc_data;
> };
>
> struct efistub_measured_event {
> --
> 2.44.0.278.ge034bb2e1d-goog
>
>
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 17:34 ` Dionna Amalie Glaze
@ 2024-03-05 17:47 ` Ard Biesheuvel
2024-03-05 17:55 ` Ilias Apalodimas
2024-03-05 18:00 ` Dionna Amalie Glaze
0 siblings, 2 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-05 17:47 UTC (permalink / raw)
To: Dionna Amalie Glaze
Cc: Qinkun Bao, linux-efi, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
>
> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> >
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > To accommodate confidential compute VMs that expose the simplified CC
> > measurement protocol instead of the full-blown TCG2 one, fall back to
> > the former if the latter does not exist.
> >
> > The CC protocol was designed to be used in this manner, which is why the
> > types and prototypes have been kept the same where possible. So reuse
> > the existing code, and only deviate from the TCG2 code path where
> > needed.
> >
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> > drivers/firmware/efi/libstub/efistub.h | 3 +
> > 2 files changed, 53 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > index 0dbc9d3f4abd..21f4567324f6 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> > unsigned long load_size,
> > enum efistub_event_type event)
> > {
> > + union {
> > + efi_status_t
> > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> > + u64, const union efistub_event *);
> > + struct { u32 hash_log_extend_event; } mixed_mode;
> > + } method;
> > struct efistub_measured_event *evt;
> > int size = struct_size(evt, tagged_event_data,
> > events[event].event_data_len);
> > efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> > efi_tcg2_protocol_t *tcg2 = NULL;
> > + union efistub_event ev;
> > efi_status_t status;
> > + void *protocol;
> >
> > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> > if (tcg2) {
> > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> > - (void **)&evt);
> > - if (status != EFI_SUCCESS)
> > - goto fail;
> > -
> > - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> > + ev.tcg2_data = (struct efi_tcg2_event){
> > .event_size = size,
> > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> > + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> > .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> > .event_header.pcr_index = events[event].pcr_index,
> > .event_header.event_type = EV_EVENT_TAG,
> > };
> > + protocol = tcg2;
> > + method.hash_log_extend_event =
> > + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> > + } else {
>
> +Qinkun Bao
> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> TCG protocol breaks backwards compatibility, it'd be preferable to
> measure into all the measurement protocols that are present.
How so? Older kernels will use TCG2 if it exists, and so will new
kernels. The only difference is that on new kernels, the CC protocol
will be used in case TCG2 is not implemented.
So the only affected scenario here is a system that today implements
TCG but not CC, and intends to implement CC later and receive
measurements into both protocols. Does that really qualify as backward
compatibility? I'd rather not accommodate future systems that
implement something that the UEFI spec says they should not.
> The UEFI
> v2.10 standard says that firmware "should not" provide both, but it is
> not MUST NOT. Given this and our desire to provide service continuity,
> I ask that you remove the "else" guard.
>
Ignoring the newer protocol if the established one exists is an
excellent way of making sure this does not happen.
> > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> > + efi_cc_protocol_t *cc = NULL;
> >
> > - evt->tagged_event_id = events[event].event_id;
> > - evt->tagged_event_data_size = events[event].event_data_len;
> > -
> > - memcpy(evt->tagged_event_data, events[event].event_data,
> > - events[event].event_data_len);
> > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> > + if (!cc)
> > + return EFI_UNSUPPORTED;
> >
> > - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> > - load_addr, load_size, &evt->event_data.tcg2_data);
> > - efi_bs_call(free_pool, evt);
> > + ev.cc_data = (struct efi_cc_event){
> > + .event_size = size,
> > + .event_header.header_size = sizeof(ev.cc_data.event_header),
> > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> > + .event_header.event_type = EV_EVENT_TAG,
> > + };
> >
> > + status = efi_call_proto(cc, map_pcr_to_mr_index,
> > + events[event].pcr_index,
> > + &ev.cc_data.event_header.mr_index);
> > if (status != EFI_SUCCESS)
> > goto fail;
> > - return EFI_SUCCESS;
> > +
> > + protocol = cc;
> > + method.hash_log_extend_event =
> > + (void *)efi_table_attr(cc, hash_log_extend_event);
> > }
> >
> > - return EFI_UNSUPPORTED;
> > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> > + if (status != EFI_SUCCESS)
> > + goto fail;
> > +
> > + evt->event_data = ev;
> > + evt->tagged_event_id = events[event].event_id;
> > + evt->tagged_event_data_size = events[event].event_data_len;
> > +
> > + memcpy(evt->tagged_event_data, events[event].event_data,
> > + events[event].event_data_len);
> > +
> > + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> > + load_addr, load_size, &evt->event_data);
> > + efi_bs_call(free_pool, evt);
> > +
> > + if (status == EFI_SUCCESS)
> > + return EFI_SUCCESS;
> > +
> > fail:
> > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> > return status;
> > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > index d621bfb719c4..4bf9a76796b7 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -954,8 +954,11 @@ union efi_cc_protocol {
> > } mixed_mode;
> > };
> >
> > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> > +
> > union efistub_event {
> > efi_tcg2_event_t tcg2_data;
> > + efi_cc_event_t cc_data;
> > };
> >
> > struct efistub_measured_event {
> > --
> > 2.44.0.278.ge034bb2e1d-goog
> >
> >
>
>
> --
> -Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 17:47 ` Ard Biesheuvel
@ 2024-03-05 17:55 ` Ilias Apalodimas
2024-03-05 18:00 ` Dionna Amalie Glaze
1 sibling, 0 replies; 21+ messages in thread
From: Ilias Apalodimas @ 2024-03-05 17:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Dionna Amalie Glaze, Qinkun Bao, linux-efi, Kuppuswamy Sathyanarayanan
On Tue, 5 Mar 2024 at 19:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
> >
> > On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > To accommodate confidential compute VMs that expose the simplified CC
> > > measurement protocol instead of the full-blown TCG2 one, fall back to
> > > the former if the latter does not exist.
> > >
> > > The CC protocol was designed to be used in this manner, which is why the
> > > types and prototypes have been kept the same where possible. So reuse
> > > the existing code, and only deviate from the TCG2 code path where
> > > needed.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> > > drivers/firmware/efi/libstub/efistub.h | 3 +
> > > 2 files changed, 53 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > index 0dbc9d3f4abd..21f4567324f6 100644
> > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> > > unsigned long load_size,
> > > enum efistub_event_type event)
> > > {
> > > + union {
> > > + efi_status_t
> > > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> > > + u64, const union efistub_event *);
> > > + struct { u32 hash_log_extend_event; } mixed_mode;
> > > + } method;
> > > struct efistub_measured_event *evt;
> > > int size = struct_size(evt, tagged_event_data,
> > > events[event].event_data_len);
> > > efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> > > efi_tcg2_protocol_t *tcg2 = NULL;
> > > + union efistub_event ev;
> > > efi_status_t status;
> > > + void *protocol;
> > >
> > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> > > if (tcg2) {
> > > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> > > - (void **)&evt);
> > > - if (status != EFI_SUCCESS)
> > > - goto fail;
> > > -
> > > - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> > > + ev.tcg2_data = (struct efi_tcg2_event){
> > > .event_size = size,
> > > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> > > + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> > > .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> > > .event_header.pcr_index = events[event].pcr_index,
> > > .event_header.event_type = EV_EVENT_TAG,
> > > };
> > > + protocol = tcg2;
> > > + method.hash_log_extend_event =
> > > + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> > > + } else {
> >
> > +Qinkun Bao
> > Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> > TCG protocol breaks backwards compatibility, it'd be preferable to
> > measure into all the measurement protocols that are present.
>
> How so? Older kernels will use TCG2 if it exists, and so will new
> kernels. The only difference is that on new kernels, the CC protocol
> will be used in case TCG2 is not implemented.
>
> So the only affected scenario here is a system that today implements
> TCG but not CC, and intends to implement CC later and receive
> measurements into both protocols. Does that really qualify as backward
> compatibility? I'd rather not accommodate future systems that
> implement something that the UEFI spec says they should not.
>
> > The UEFI
> > v2.10 standard says that firmware "should not" provide both, but it is
> > not MUST NOT. Given this and our desire to provide service continuity,
> > I ask that you remove the "else" guard.
> >
>
> Ignoring the newer protocol if the established one exists is an
> excellent way of making sure this does not happen.
Apart from what the EFI spec says, why would anyone want to use both
protocols? Won't we end up measuring things 2x ?
Thanks
/Ilias
>
>
> > > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> > > + efi_cc_protocol_t *cc = NULL;
> > >
> > > - evt->tagged_event_id = events[event].event_id;
> > > - evt->tagged_event_data_size = events[event].event_data_len;
> > > -
> > > - memcpy(evt->tagged_event_data, events[event].event_data,
> > > - events[event].event_data_len);
> > > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> > > + if (!cc)
> > > + return EFI_UNSUPPORTED;
> > >
> > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> > > - load_addr, load_size, &evt->event_data.tcg2_data);
> > > - efi_bs_call(free_pool, evt);
> > > + ev.cc_data = (struct efi_cc_event){
> > > + .event_size = size,
> > > + .event_header.header_size = sizeof(ev.cc_data.event_header),
> > > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> > > + .event_header.event_type = EV_EVENT_TAG,
> > > + };
> > >
> > > + status = efi_call_proto(cc, map_pcr_to_mr_index,
> > > + events[event].pcr_index,
> > > + &ev.cc_data.event_header.mr_index);
> > > if (status != EFI_SUCCESS)
> > > goto fail;
> > > - return EFI_SUCCESS;
> > > +
> > > + protocol = cc;
> > > + method.hash_log_extend_event =
> > > + (void *)efi_table_attr(cc, hash_log_extend_event);
> > > }
> > >
> > > - return EFI_UNSUPPORTED;
> > > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> > > + if (status != EFI_SUCCESS)
> > > + goto fail;
> > > +
> > > + evt->event_data = ev;
> > > + evt->tagged_event_id = events[event].event_id;
> > > + evt->tagged_event_data_size = events[event].event_data_len;
> > > +
> > > + memcpy(evt->tagged_event_data, events[event].event_data,
> > > + events[event].event_data_len);
> > > +
> > > + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> > > + load_addr, load_size, &evt->event_data);
> > > + efi_bs_call(free_pool, evt);
> > > +
> > > + if (status == EFI_SUCCESS)
> > > + return EFI_SUCCESS;
> > > +
> > > fail:
> > > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> > > return status;
> > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > > index d621bfb719c4..4bf9a76796b7 100644
> > > --- a/drivers/firmware/efi/libstub/efistub.h
> > > +++ b/drivers/firmware/efi/libstub/efistub.h
> > > @@ -954,8 +954,11 @@ union efi_cc_protocol {
> > > } mixed_mode;
> > > };
> > >
> > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> > > +
> > > union efistub_event {
> > > efi_tcg2_event_t tcg2_data;
> > > + efi_cc_event_t cc_data;
> > > };
> > >
> > > struct efistub_measured_event {
> > > --
> > > 2.44.0.278.ge034bb2e1d-goog
> > >
> > >
> >
> >
> > --
> > -Dionna Glaze, PhD (she/her)
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 17:47 ` Ard Biesheuvel
2024-03-05 17:55 ` Ilias Apalodimas
@ 2024-03-05 18:00 ` Dionna Amalie Glaze
2024-03-05 18:33 ` Kuppuswamy Sathyanarayanan
1 sibling, 1 reply; 21+ messages in thread
From: Dionna Amalie Glaze @ 2024-03-05 18:00 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Qinkun Bao, linux-efi, Kuppuswamy Sathyanarayanan, Ilias Apalodimas
On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
> >
> > On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> > >
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > To accommodate confidential compute VMs that expose the simplified CC
> > > measurement protocol instead of the full-blown TCG2 one, fall back to
> > > the former if the latter does not exist.
> > >
> > > The CC protocol was designed to be used in this manner, which is why the
> > > types and prototypes have been kept the same where possible. So reuse
> > > the existing code, and only deviate from the TCG2 code path where
> > > needed.
> > >
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > > drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> > > drivers/firmware/efi/libstub/efistub.h | 3 +
> > > 2 files changed, 53 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > index 0dbc9d3f4abd..21f4567324f6 100644
> > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> > > unsigned long load_size,
> > > enum efistub_event_type event)
> > > {
> > > + union {
> > > + efi_status_t
> > > + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> > > + u64, const union efistub_event *);
> > > + struct { u32 hash_log_extend_event; } mixed_mode;
> > > + } method;
> > > struct efistub_measured_event *evt;
> > > int size = struct_size(evt, tagged_event_data,
> > > events[event].event_data_len);
> > > efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> > > efi_tcg2_protocol_t *tcg2 = NULL;
> > > + union efistub_event ev;
> > > efi_status_t status;
> > > + void *protocol;
> > >
> > > efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> > > if (tcg2) {
> > > - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> > > - (void **)&evt);
> > > - if (status != EFI_SUCCESS)
> > > - goto fail;
> > > -
> > > - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> > > + ev.tcg2_data = (struct efi_tcg2_event){
> > > .event_size = size,
> > > - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> > > + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> > > .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> > > .event_header.pcr_index = events[event].pcr_index,
> > > .event_header.event_type = EV_EVENT_TAG,
> > > };
> > > + protocol = tcg2;
> > > + method.hash_log_extend_event =
> > > + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> > > + } else {
> >
> > +Qinkun Bao
> > Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> > TCG protocol breaks backwards compatibility, it'd be preferable to
> > measure into all the measurement protocols that are present.
>
> How so? Older kernels will use TCG2 if it exists, and so will new
> kernels. The only difference is that on new kernels, the CC protocol
> will be used in case TCG2 is not implemented.
>
> So the only affected scenario here is a system that today implements
> TCG but not CC, and intends to implement CC later and receive
> measurements into both protocols. Does that really qualify as backward
> compatibility? I'd rather not accommodate future systems that
> implement something that the UEFI spec says they should not.
>
> > The UEFI
> > v2.10 standard says that firmware "should not" provide both, but it is
> > not MUST NOT. Given this and our desire to provide service continuity,
> > I ask that you remove the "else" guard.
> >
>
> Ignoring the newer protocol if the established one exists is an
> excellent way of making sure this does not happen.
>
The problem is that the protocols are not equivalent, and we disagree
with the standard's claim of "should not" to the point that we're
taking it to the USWG. The "should not" advisement is predicated on
not trusting boot layers to use both protocols when they're both
present, such that you could accidentally miss measuring a
security-critical event. That's a strawman though, since you already
need to develop trust in those boot layers. We have software supply
chain endorsements for tracking that kind of property for use in
attestation verification.
The CC protocol is useful for hardware-rooted boot measurement, but it
does nothing about the rest of TPM 2.0. There are plenty of users that
want to use a vTPM that's hosted by the VMM but also get an extra
integrity assurance that measurements into TDX RTMRs and attested by
an Intel-rooted key pass an extra level of scrutiny.
>
> > > + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> > > + efi_cc_protocol_t *cc = NULL;
> > >
> > > - evt->tagged_event_id = events[event].event_id;
> > > - evt->tagged_event_data_size = events[event].event_data_len;
> > > -
> > > - memcpy(evt->tagged_event_data, events[event].event_data,
> > > - events[event].event_data_len);
> > > + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> > > + if (!cc)
> > > + return EFI_UNSUPPORTED;
> > >
> > > - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> > > - load_addr, load_size, &evt->event_data.tcg2_data);
> > > - efi_bs_call(free_pool, evt);
> > > + ev.cc_data = (struct efi_cc_event){
> > > + .event_size = size,
> > > + .event_header.header_size = sizeof(ev.cc_data.event_header),
> > > + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> > > + .event_header.event_type = EV_EVENT_TAG,
> > > + };
> > >
> > > + status = efi_call_proto(cc, map_pcr_to_mr_index,
> > > + events[event].pcr_index,
> > > + &ev.cc_data.event_header.mr_index);
> > > if (status != EFI_SUCCESS)
> > > goto fail;
> > > - return EFI_SUCCESS;
> > > +
> > > + protocol = cc;
> > > + method.hash_log_extend_event =
> > > + (void *)efi_table_attr(cc, hash_log_extend_event);
> > > }
> > >
> > > - return EFI_UNSUPPORTED;
> > > + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> > > + if (status != EFI_SUCCESS)
> > > + goto fail;
> > > +
> > > + evt->event_data = ev;
> > > + evt->tagged_event_id = events[event].event_id;
> > > + evt->tagged_event_data_size = events[event].event_data_len;
> > > +
> > > + memcpy(evt->tagged_event_data, events[event].event_data,
> > > + events[event].event_data_len);
> > > +
> > > + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> > > + load_addr, load_size, &evt->event_data);
> > > + efi_bs_call(free_pool, evt);
> > > +
> > > + if (status == EFI_SUCCESS)
> > > + return EFI_SUCCESS;
> > > +
> > > fail:
> > > efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> > > return status;
> > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> > > index d621bfb719c4..4bf9a76796b7 100644
> > > --- a/drivers/firmware/efi/libstub/efistub.h
> > > +++ b/drivers/firmware/efi/libstub/efistub.h
> > > @@ -954,8 +954,11 @@ union efi_cc_protocol {
> > > } mixed_mode;
> > > };
> > >
> > > +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> > > +
> > > union efistub_event {
> > > efi_tcg2_event_t tcg2_data;
> > > + efi_cc_event_t cc_data;
> > > };
> > >
> > > struct efistub_measured_event {
> > > --
> > > 2.44.0.278.ge034bb2e1d-goog
> > >
> > >
> >
> >
> > --
> > -Dionna Glaze, PhD (she/her)
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs
2024-03-04 10:44 ` [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs Ard Biesheuvel
@ 2024-03-05 18:00 ` Ilias Apalodimas
2024-03-05 19:27 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 21+ messages in thread
From: Ilias Apalodimas @ 2024-03-05 18:00 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-efi, Ard Biesheuvel, Kuppuswamy Sathyanarayanan
Hi Ard,
On Mon, 4 Mar 2024 at 12:44, Ard Biesheuvel <ardb+git@google.com> wrote:
>
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> 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.
>
> 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>
> [ardb: Drop code changes, keep typedefs and #define's only]
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> drivers/firmware/efi/libstub/efistub.h | 79 ++++++++++++++++++++
> include/linux/efi.h | 1 +
> 2 files changed, 80 insertions(+)
>
[...]
> +
> +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];
We should define this as a flexible array member instead of a zero-length array?
The spec is funny and defines this as event_data[1]. I think we aren't
using the sizeof(struct efi_cc_event) anywhere, so if this struct is
not used as a member of another struct or an array we can omit it
entirely.
[...]
Cheers
/Ilias
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 18:00 ` Dionna Amalie Glaze
@ 2024-03-05 18:33 ` Kuppuswamy Sathyanarayanan
2024-03-05 18:46 ` Dionna Amalie Glaze
0 siblings, 1 reply; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 18:33 UTC (permalink / raw)
To: Dionna Amalie Glaze, Ard Biesheuvel
Cc: Qinkun Bao, linux-efi, Ilias Apalodimas
On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>
>>>> To accommodate confidential compute VMs that expose the simplified CC
>>>> measurement protocol instead of the full-blown TCG2 one, fall back to
>>>> the former if the latter does not exist.
>>>>
>>>> The CC protocol was designed to be used in this manner, which is why the
>>>> types and prototypes have been kept the same where possible. So reuse
>>>> the existing code, and only deviate from the TCG2 code path where
>>>> needed.
>>>>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
>>>> drivers/firmware/efi/libstub/efistub.h | 3 +
>>>> 2 files changed, 53 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> index 0dbc9d3f4abd..21f4567324f6 100644
>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>>>> unsigned long load_size,
>>>> enum efistub_event_type event)
>>>> {
>>>> + union {
>>>> + efi_status_t
>>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
>>>> + u64, const union efistub_event *);
>>>> + struct { u32 hash_log_extend_event; } mixed_mode;
>>>> + } method;
>>>> struct efistub_measured_event *evt;
>>>> int size = struct_size(evt, tagged_event_data,
>>>> events[event].event_data_len);
>>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
>>>> efi_tcg2_protocol_t *tcg2 = NULL;
>>>> + union efistub_event ev;
>>>> efi_status_t status;
>>>> + void *protocol;
>>>>
>>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
>>>> if (tcg2) {
>>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>>>> - (void **)&evt);
>>>> - if (status != EFI_SUCCESS)
>>>> - goto fail;
>>>> -
>>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
>>>> + ev.tcg2_data = (struct efi_tcg2_event){
>>>> .event_size = size,
>>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
>>>> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
>>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
>>>> .event_header.pcr_index = events[event].pcr_index,
>>>> .event_header.event_type = EV_EVENT_TAG,
>>>> };
>>>> + protocol = tcg2;
>>>> + method.hash_log_extend_event =
>>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
>>>> + } else {
>>> +Qinkun Bao
>>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
>>> TCG protocol breaks backwards compatibility, it'd be preferable to
>>> measure into all the measurement protocols that are present.
>> How so? Older kernels will use TCG2 if it exists, and so will new
>> kernels. The only difference is that on new kernels, the CC protocol
>> will be used in case TCG2 is not implemented.
>>
>> So the only affected scenario here is a system that today implements
>> TCG but not CC, and intends to implement CC later and receive
>> measurements into both protocols. Does that really qualify as backward
>> compatibility? I'd rather not accommodate future systems that
>> implement something that the UEFI spec says they should not.
>>
>>> The UEFI
>>> v2.10 standard says that firmware "should not" provide both, but it is
>>> not MUST NOT. Given this and our desire to provide service continuity,
>>> I ask that you remove the "else" guard.
>>>
>> Ignoring the newer protocol if the established one exists is an
>> excellent way of making sure this does not happen.
>>
> The problem is that the protocols are not equivalent, and we disagree
> with the standard's claim of "should not" to the point that we're
> taking it to the USWG. The "should not" advisement is predicated on
> not trusting boot layers to use both protocols when they're both
> present, such that you could accidentally miss measuring a
> security-critical event. That's a strawman though, since you already
> need to develop trust in those boot layers. We have software supply
> chain endorsements for tracking that kind of property for use in
> attestation verification.
>
> The CC protocol is useful for hardware-rooted boot measurement, but it
> does nothing about the rest of TPM 2.0. There are plenty of users that
> want to use a vTPM that's hosted by the VMM but also get an extra
> integrity assurance that measurements into TDX RTMRs and attested by
> an Intel-rooted key pass an extra level of scrutiny.
>
If you check the EDK2 part of this support, it also uses if else model.
It does not measure both. If there a complete vTPM support, why
can't user trust measurements from it? I think the CC vendors will
ensure their vTPM implementation is protected from attack from the
host (like implementing it part of firmware or launching it as service in
a separate VM).
>>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>>>> + efi_cc_protocol_t *cc = NULL;
>>>>
>>>> - evt->tagged_event_id = events[event].event_id;
>>>> - evt->tagged_event_data_size = events[event].event_data_len;
>>>> -
>>>> - memcpy(evt->tagged_event_data, events[event].event_data,
>>>> - events[event].event_data_len);
>>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>>>> + if (!cc)
>>>> + return EFI_UNSUPPORTED;
>>>>
>>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>>>> - load_addr, load_size, &evt->event_data.tcg2_data);
>>>> - efi_bs_call(free_pool, evt);
>>>> + ev.cc_data = (struct efi_cc_event){
>>>> + .event_size = size,
>>>> + .event_header.header_size = sizeof(ev.cc_data.event_header),
>>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
>>>> + .event_header.event_type = EV_EVENT_TAG,
>>>> + };
>>>>
>>>> + status = efi_call_proto(cc, map_pcr_to_mr_index,
>>>> + events[event].pcr_index,
>>>> + &ev.cc_data.event_header.mr_index);
>>>> if (status != EFI_SUCCESS)
>>>> goto fail;
>>>> - return EFI_SUCCESS;
>>>> +
>>>> + protocol = cc;
>>>> + method.hash_log_extend_event =
>>>> + (void *)efi_table_attr(cc, hash_log_extend_event);
>>>> }
>>>>
>>>> - return EFI_UNSUPPORTED;
>>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
>>>> + if (status != EFI_SUCCESS)
>>>> + goto fail;
>>>> +
>>>> + evt->event_data = ev;
>>>> + evt->tagged_event_id = events[event].event_id;
>>>> + evt->tagged_event_data_size = events[event].event_data_len;
>>>> +
>>>> + memcpy(evt->tagged_event_data, events[event].event_data,
>>>> + events[event].event_data_len);
>>>> +
>>>> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
>>>> + load_addr, load_size, &evt->event_data);
>>>> + efi_bs_call(free_pool, evt);
>>>> +
>>>> + if (status == EFI_SUCCESS)
>>>> + return EFI_SUCCESS;
>>>> +
>>>> fail:
>>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
>>>> return status;
>>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>>>> index d621bfb719c4..4bf9a76796b7 100644
>>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>>> @@ -954,8 +954,11 @@ union efi_cc_protocol {
>>>> } mixed_mode;
>>>> };
>>>>
>>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
>>>> +
>>>> union efistub_event {
>>>> efi_tcg2_event_t tcg2_data;
>>>> + efi_cc_event_t cc_data;
>>>> };
>>>>
>>>> struct efistub_measured_event {
>>>> --
>>>> 2.44.0.278.ge034bb2e1d-goog
>>>>
>>>>
>>>
>>> --
>>> -Dionna Glaze, PhD (she/her)
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 18:33 ` Kuppuswamy Sathyanarayanan
@ 2024-03-05 18:46 ` Dionna Amalie Glaze
2024-03-05 19:36 ` Kuppuswamy Sathyanarayanan
2024-03-05 21:28 ` Ard Biesheuvel
0 siblings, 2 replies; 21+ messages in thread
From: Dionna Amalie Glaze @ 2024-03-05 18:46 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Ard Biesheuvel, Qinkun Bao, linux-efi, Ilias Apalodimas
On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
> > On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
> >>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> >>>> From: Ard Biesheuvel <ardb@kernel.org>
> >>>>
> >>>> To accommodate confidential compute VMs that expose the simplified CC
> >>>> measurement protocol instead of the full-blown TCG2 one, fall back to
> >>>> the former if the latter does not exist.
> >>>>
> >>>> The CC protocol was designed to be used in this manner, which is why the
> >>>> types and prototypes have been kept the same where possible. So reuse
> >>>> the existing code, and only deviate from the TCG2 code path where
> >>>> needed.
> >>>>
> >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>>> ---
> >>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> >>>> drivers/firmware/efi/libstub/efistub.h | 3 +
> >>>> 2 files changed, 53 insertions(+), 17 deletions(-)
> >>>>
> >>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>> index 0dbc9d3f4abd..21f4567324f6 100644
> >>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> >>>> unsigned long load_size,
> >>>> enum efistub_event_type event)
> >>>> {
> >>>> + union {
> >>>> + efi_status_t
> >>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> >>>> + u64, const union efistub_event *);
> >>>> + struct { u32 hash_log_extend_event; } mixed_mode;
> >>>> + } method;
> >>>> struct efistub_measured_event *evt;
> >>>> int size = struct_size(evt, tagged_event_data,
> >>>> events[event].event_data_len);
> >>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> >>>> efi_tcg2_protocol_t *tcg2 = NULL;
> >>>> + union efistub_event ev;
> >>>> efi_status_t status;
> >>>> + void *protocol;
> >>>>
> >>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> >>>> if (tcg2) {
> >>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> >>>> - (void **)&evt);
> >>>> - if (status != EFI_SUCCESS)
> >>>> - goto fail;
> >>>> -
> >>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> >>>> + ev.tcg2_data = (struct efi_tcg2_event){
> >>>> .event_size = size,
> >>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> >>>> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> >>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> >>>> .event_header.pcr_index = events[event].pcr_index,
> >>>> .event_header.event_type = EV_EVENT_TAG,
> >>>> };
> >>>> + protocol = tcg2;
> >>>> + method.hash_log_extend_event =
> >>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> >>>> + } else {
> >>> +Qinkun Bao
> >>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> >>> TCG protocol breaks backwards compatibility, it'd be preferable to
> >>> measure into all the measurement protocols that are present.
> >> How so? Older kernels will use TCG2 if it exists, and so will new
> >> kernels. The only difference is that on new kernels, the CC protocol
> >> will be used in case TCG2 is not implemented.
> >>
> >> So the only affected scenario here is a system that today implements
> >> TCG but not CC, and intends to implement CC later and receive
> >> measurements into both protocols. Does that really qualify as backward
> >> compatibility? I'd rather not accommodate future systems that
> >> implement something that the UEFI spec says they should not.
> >>
> >>> The UEFI
> >>> v2.10 standard says that firmware "should not" provide both, but it is
> >>> not MUST NOT. Given this and our desire to provide service continuity,
> >>> I ask that you remove the "else" guard.
> >>>
> >> Ignoring the newer protocol if the established one exists is an
> >> excellent way of making sure this does not happen.
> >>
> > The problem is that the protocols are not equivalent, and we disagree
> > with the standard's claim of "should not" to the point that we're
> > taking it to the USWG. The "should not" advisement is predicated on
> > not trusting boot layers to use both protocols when they're both
> > present, such that you could accidentally miss measuring a
> > security-critical event. That's a strawman though, since you already
> > need to develop trust in those boot layers. We have software supply
> > chain endorsements for tracking that kind of property for use in
> > attestation verification.
> >
> > The CC protocol is useful for hardware-rooted boot measurement, but it
> > does nothing about the rest of TPM 2.0. There are plenty of users that
> > want to use a vTPM that's hosted by the VMM but also get an extra
> > integrity assurance that measurements into TDX RTMRs and attested by
> > an Intel-rooted key pass an extra level of scrutiny.
> >
>
> If you check the EDK2 part of this support, it also uses if else model.
Yes, we've been discussing this with Intel and they agreed to allow a
default false build option to measure into both.
> It does not measure both. If there a complete vTPM support, why
> can't user trust measurements from it? I think the CC vendors will
There are folks who want to do a double-check with TEE quotes, but yes
I agree in general this is not the best situation. It's a stepping
stones model rather than scaling Everest in one bound.
Ideally you'd have a measured and protected TPM implementation with
adequate security for persistent data so that the
CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed.
But anyway, the standard is what it is for now, so I wouldn't block
this patch based on this request. When there's more alignment from the
UEFI standards working group and an accepted patch into EDK2, then we
can revisit this in the different boot layers.
> ensure their vTPM implementation is protected from attack from the
> host (like implementing it part of firmware or launching it as service in
> a separate VM).
>
> >>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> >>>> + efi_cc_protocol_t *cc = NULL;
> >>>>
> >>>> - evt->tagged_event_id = events[event].event_id;
> >>>> - evt->tagged_event_data_size = events[event].event_data_len;
> >>>> -
> >>>> - memcpy(evt->tagged_event_data, events[event].event_data,
> >>>> - events[event].event_data_len);
> >>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> >>>> + if (!cc)
> >>>> + return EFI_UNSUPPORTED;
> >>>>
> >>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> >>>> - load_addr, load_size, &evt->event_data.tcg2_data);
> >>>> - efi_bs_call(free_pool, evt);
> >>>> + ev.cc_data = (struct efi_cc_event){
> >>>> + .event_size = size,
> >>>> + .event_header.header_size = sizeof(ev.cc_data.event_header),
> >>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> >>>> + .event_header.event_type = EV_EVENT_TAG,
> >>>> + };
> >>>>
> >>>> + status = efi_call_proto(cc, map_pcr_to_mr_index,
> >>>> + events[event].pcr_index,
> >>>> + &ev.cc_data.event_header.mr_index);
> >>>> if (status != EFI_SUCCESS)
> >>>> goto fail;
> >>>> - return EFI_SUCCESS;
> >>>> +
> >>>> + protocol = cc;
> >>>> + method.hash_log_extend_event =
> >>>> + (void *)efi_table_attr(cc, hash_log_extend_event);
> >>>> }
> >>>>
> >>>> - return EFI_UNSUPPORTED;
> >>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> >>>> + if (status != EFI_SUCCESS)
> >>>> + goto fail;
> >>>> +
> >>>> + evt->event_data = ev;
> >>>> + evt->tagged_event_id = events[event].event_id;
> >>>> + evt->tagged_event_data_size = events[event].event_data_len;
> >>>> +
> >>>> + memcpy(evt->tagged_event_data, events[event].event_data,
> >>>> + events[event].event_data_len);
> >>>> +
> >>>> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> >>>> + load_addr, load_size, &evt->event_data);
> >>>> + efi_bs_call(free_pool, evt);
> >>>> +
> >>>> + if (status == EFI_SUCCESS)
> >>>> + return EFI_SUCCESS;
> >>>> +
> >>>> fail:
> >>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> >>>> return status;
> >>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> >>>> index d621bfb719c4..4bf9a76796b7 100644
> >>>> --- a/drivers/firmware/efi/libstub/efistub.h
> >>>> +++ b/drivers/firmware/efi/libstub/efistub.h
> >>>> @@ -954,8 +954,11 @@ union efi_cc_protocol {
> >>>> } mixed_mode;
> >>>> };
> >>>>
> >>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> >>>> +
> >>>> union efistub_event {
> >>>> efi_tcg2_event_t tcg2_data;
> >>>> + efi_cc_event_t cc_data;
> >>>> };
> >>>>
> >>>> struct efistub_measured_event {
> >>>> --
> >>>> 2.44.0.278.ge034bb2e1d-goog
> >>>>
> >>>>
> >>>
> >>> --
> >>> -Dionna Glaze, PhD (she/her)
> >
> >
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event
2024-03-05 8:21 ` Ard Biesheuvel
@ 2024-03-05 19:19 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 19:19 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Ard Biesheuvel, linux-efi, Ilias Apalodimas
On 3/5/24 12:21 AM, Ard Biesheuvel wrote:
> On Tue, 5 Mar 2024 at 05:30, Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 3/4/24 2:44 AM, Ard Biesheuvel wrote:
>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>
>>> In spite of the efi_ prefix, struct efi_tcg2_tagged_event is specific to
>>> the EFI stub, and so we can tweak it to our liking if needed, e.g., to
>>> accommodate the TDX variant of the TCG2 measurement protocol.
>>>
>>> In preparation for that, get rid of it entirely, and combine it with the
>>> efi_measured_event struct used by the measurement code.
>>>
>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
With nits fixed,
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 26 ++++++++------------
>>> drivers/firmware/efi/libstub/efistub.h | 18 ++++++++------
>>> 2 files changed, 21 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> index bfa30625f5d0..0dbc9d3f4abd 100644
>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>> @@ -193,7 +193,7 @@ void efi_apply_loadoptions_quirk(const void **load_options, u32 *load_options_si
>>> *load_options_size = load_option_unpacked.optional_data_size;
>>> }
>>>
>>> -enum efistub_event {
>>> +enum efistub_event_type {
>>> EFISTUB_EVT_INITRD,
>>> EFISTUB_EVT_LOAD_OPTIONS,
>>> EFISTUB_EVT_COUNT,
>>> @@ -221,44 +221,38 @@ static const struct {
>>>
>>> static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>>> unsigned long load_size,
>>> - enum efistub_event event)
>>> + enum efistub_event_type event)
>>> {
>>> + struct efistub_measured_event *evt;
>>> + int size = struct_size(evt, tagged_event_data,
>>> + events[event].event_data_len);
>> Include linux/overflow.h explicitly?
>>
> Yes, good point.
>
>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
>>> 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);
>> It looks like in patch 3 you have converted evt as stack variable. Since that
>> change is not specific to CC fallback, can it be moved here?
>>
> Not sure what you mean here. evt is still there after parch #3
Sorry, it looks like I misread the patch # 3. Please ignore this comment.
>
>>> if (status != EFI_SUCCESS)
>>> goto fail;
>>>
>>> - evt->event_data = (struct efi_tcg2_event){
>>> + evt->event_data.tcg2_data = (struct efi_tcg2_event){
>>> .event_size = size,
>>> - .event_header.header_size = sizeof(evt->event_data.event_header),
>>> + .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
>>> .event_header.pcr_index = events[event].pcr_index,
>>> .event_header.event_type = EV_EVENT_TAG,
>>> };
>>>
>>> - evt->tagged_event = (struct efi_tcg2_tagged_event){
>>> - .tagged_event_id = events[event].event_id,
>>> - .tagged_event_data_size = events[event].event_data_len,
>>> - };
>>> + evt->tagged_event_id = events[event].event_id;
>>> + evt->tagged_event_data_size = events[event].event_data_len;
>>>
>>> memcpy(evt->tagged_event_data, events[event].event_data,
>>> events[event].event_data_len);
>>>
>>> status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>>> - load_addr, load_size, &evt->event_data);
>>> + load_addr, load_size, &evt->event_data.tcg2_data);
>>> efi_bs_call(free_pool, evt);
>>>
>>> if (status != EFI_SUCCESS)
>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>>> index c04b82ea40f2..b2c50dce48b8 100644
>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>> @@ -843,14 +843,7 @@ struct efi_tcg2_event {
>>> /* u8[] event follows here */
>>> } __packed;
>>>
>>> -struct efi_tcg2_tagged_event {
>>> - u32 tagged_event_id;
>>> - u32 tagged_event_data_size;
>>> - /* u8 tagged event data follows here */
>>> -} __packed;
>>> -
>>> typedef struct efi_tcg2_event efi_tcg2_event_t;
>>> -typedef struct efi_tcg2_tagged_event efi_tcg2_tagged_event_t;
>>> typedef union efi_tcg2_protocol efi_tcg2_protocol_t;
>>>
>>> union efi_tcg2_protocol {
>>> @@ -882,6 +875,17 @@ union efi_tcg2_protocol {
>>> } mixed_mode;
>>> };
>>>
>>> +union efistub_event {
>>> + efi_tcg2_event_t tcg2_data;
>>> +};
>>> +
>>> +struct efistub_measured_event {
>>> + union efistub_event event_data;
>>> + u32 tagged_event_id;
>>> + u32 tagged_event_data_size;
>>> + u8 tagged_event_data[];
>>> +} __packed;
>>> +
>> Since efistub_measured_event is only used efi-stub-helper.c, why
>> not leave it there?
>>
> Indeed. I will move it back.
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs
2024-03-05 18:00 ` Ilias Apalodimas
@ 2024-03-05 19:27 ` Kuppuswamy Sathyanarayanan
2024-03-05 19:59 ` Ilias Apalodimas
0 siblings, 1 reply; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 19:27 UTC (permalink / raw)
To: Ilias Apalodimas, Ard Biesheuvel; +Cc: linux-efi, Ard Biesheuvel
On 3/5/24 10:00 AM, Ilias Apalodimas wrote:
> Hi Ard,
>
> On Mon, 4 Mar 2024 at 12:44, Ard Biesheuvel <ardb+git@google.com> wrote:
>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>
>> 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.
>>
>> 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>
>> [ardb: Drop code changes, keep typedefs and #define's only]
>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>> drivers/firmware/efi/libstub/efistub.h | 79 ++++++++++++++++++++
>> include/linux/efi.h | 1 +
>> 2 files changed, 80 insertions(+)
>>
> [...]
>
>> +
>> +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];
> We should define this as a flexible array member instead of a zero-length array?
> The spec is funny and defines this as event_data[1]. I think we aren't
> using the sizeof(struct efi_cc_event) anywhere, so if this struct is
> not used as a member of another struct or an array we can omit it
> entirely.
Flexible array is also fine or we can just add a comment like "u8[] event follows here"
like in struct efi_tcg2_event .
>
> [...]
>
> Cheers
> /Ilias
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 18:46 ` Dionna Amalie Glaze
@ 2024-03-05 19:36 ` Kuppuswamy Sathyanarayanan
2024-03-05 21:28 ` Dionna Amalie Glaze
2024-03-05 21:28 ` Ard Biesheuvel
1 sibling, 1 reply; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 19:36 UTC (permalink / raw)
To: Dionna Amalie Glaze
Cc: Ard Biesheuvel, Qinkun Bao, linux-efi, Ilias Apalodimas
On 3/5/24 10:46 AM, Dionna Amalie Glaze wrote:
> On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
>>> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>>>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
>>>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
>>>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>>>
>>>>>> To accommodate confidential compute VMs that expose the simplified CC
>>>>>> measurement protocol instead of the full-blown TCG2 one, fall back to
>>>>>> the former if the latter does not exist.
>>>>>>
>>>>>> The CC protocol was designed to be used in this manner, which is why the
>>>>>> types and prototypes have been kept the same where possible. So reuse
>>>>>> the existing code, and only deviate from the TCG2 code path where
>>>>>> needed.
>>>>>>
>>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>>>> ---
>>>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
>>>>>> drivers/firmware/efi/libstub/efistub.h | 3 +
>>>>>> 2 files changed, 53 insertions(+), 17 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>>> index 0dbc9d3f4abd..21f4567324f6 100644
>>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>>>>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
>>>>>> unsigned long load_size,
>>>>>> enum efistub_event_type event)
>>>>>> {
>>>>>> + union {
>>>>>> + efi_status_t
>>>>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
>>>>>> + u64, const union efistub_event *);
>>>>>> + struct { u32 hash_log_extend_event; } mixed_mode;
>>>>>> + } method;
>>>>>> struct efistub_measured_event *evt;
>>>>>> int size = struct_size(evt, tagged_event_data,
>>>>>> events[event].event_data_len);
>>>>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
>>>>>> efi_tcg2_protocol_t *tcg2 = NULL;
>>>>>> + union efistub_event ev;
>>>>>> efi_status_t status;
>>>>>> + void *protocol;
>>>>>>
>>>>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
>>>>>> if (tcg2) {
>>>>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
>>>>>> - (void **)&evt);
>>>>>> - if (status != EFI_SUCCESS)
>>>>>> - goto fail;
>>>>>> -
>>>>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
>>>>>> + ev.tcg2_data = (struct efi_tcg2_event){
>>>>>> .event_size = size,
>>>>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
>>>>>> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
>>>>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
>>>>>> .event_header.pcr_index = events[event].pcr_index,
>>>>>> .event_header.event_type = EV_EVENT_TAG,
>>>>>> };
>>>>>> + protocol = tcg2;
>>>>>> + method.hash_log_extend_event =
>>>>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
>>>>>> + } else {
>>>>> +Qinkun Bao
>>>>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
>>>>> TCG protocol breaks backwards compatibility, it'd be preferable to
>>>>> measure into all the measurement protocols that are present.
>>>> How so? Older kernels will use TCG2 if it exists, and so will new
>>>> kernels. The only difference is that on new kernels, the CC protocol
>>>> will be used in case TCG2 is not implemented.
>>>>
>>>> So the only affected scenario here is a system that today implements
>>>> TCG but not CC, and intends to implement CC later and receive
>>>> measurements into both protocols. Does that really qualify as backward
>>>> compatibility? I'd rather not accommodate future systems that
>>>> implement something that the UEFI spec says they should not.
>>>>
>>>>> The UEFI
>>>>> v2.10 standard says that firmware "should not" provide both, but it is
>>>>> not MUST NOT. Given this and our desire to provide service continuity,
>>>>> I ask that you remove the "else" guard.
>>>>>
>>>> Ignoring the newer protocol if the established one exists is an
>>>> excellent way of making sure this does not happen.
>>>>
>>> The problem is that the protocols are not equivalent, and we disagree
>>> with the standard's claim of "should not" to the point that we're
>>> taking it to the USWG. The "should not" advisement is predicated on
>>> not trusting boot layers to use both protocols when they're both
>>> present, such that you could accidentally miss measuring a
>>> security-critical event. That's a strawman though, since you already
>>> need to develop trust in those boot layers. We have software supply
>>> chain endorsements for tracking that kind of property for use in
>>> attestation verification.
>>>
>>> The CC protocol is useful for hardware-rooted boot measurement, but it
>>> does nothing about the rest of TPM 2.0. There are plenty of users that
>>> want to use a vTPM that's hosted by the VMM but also get an extra
>>> integrity assurance that measurements into TDX RTMRs and attested by
>>> an Intel-rooted key pass an extra level of scrutiny.
>>>
>> If you check the EDK2 part of this support, it also uses if else model.
> Yes, we've been discussing this with Intel and they agreed to allow a
> default false build option to measure into both.
To make it clear, any plans to update the spec with this requirement?
>
>> It does not measure both. If there a complete vTPM support, why
>> can't user trust measurements from it? I think the CC vendors will
> There are folks who want to do a double-check with TEE quotes, but yes
> I agree in general this is not the best situation. It's a stepping
> stones model rather than scaling Everest in one bound.
> Ideally you'd have a measured and protected TPM implementation with
> adequate security for persistent data so that the
> CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed.
>
> But anyway, the standard is what it is for now, so I wouldn't block
> this patch based on this request. When there's more alignment from the
> UEFI standards working group and an accepted patch into EDK2, then we
> can revisit this in the different boot layers.
Got it. Personally I think we can consider it, once it is adapted in
firmware and updated in spec (to make it consistent with firmware
support). Anyway, since doing it is harmless, I will leave it to the
maintainers choice.
>
>> ensure their vTPM implementation is protected from attack from the
>> host (like implementing it part of firmware or launching it as service in
>> a separate VM).
>>
>>>>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
>>>>>> + efi_cc_protocol_t *cc = NULL;
>>>>>>
>>>>>> - evt->tagged_event_id = events[event].event_id;
>>>>>> - evt->tagged_event_data_size = events[event].event_data_len;
>>>>>> -
>>>>>> - memcpy(evt->tagged_event_data, events[event].event_data,
>>>>>> - events[event].event_data_len);
>>>>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
>>>>>> + if (!cc)
>>>>>> + return EFI_UNSUPPORTED;
>>>>>>
>>>>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
>>>>>> - load_addr, load_size, &evt->event_data.tcg2_data);
>>>>>> - efi_bs_call(free_pool, evt);
>>>>>> + ev.cc_data = (struct efi_cc_event){
>>>>>> + .event_size = size,
>>>>>> + .event_header.header_size = sizeof(ev.cc_data.event_header),
>>>>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
>>>>>> + .event_header.event_type = EV_EVENT_TAG,
>>>>>> + };
>>>>>>
>>>>>> + status = efi_call_proto(cc, map_pcr_to_mr_index,
>>>>>> + events[event].pcr_index,
>>>>>> + &ev.cc_data.event_header.mr_index);
>>>>>> if (status != EFI_SUCCESS)
>>>>>> goto fail;
>>>>>> - return EFI_SUCCESS;
>>>>>> +
>>>>>> + protocol = cc;
>>>>>> + method.hash_log_extend_event =
>>>>>> + (void *)efi_table_attr(cc, hash_log_extend_event);
>>>>>> }
>>>>>>
>>>>>> - return EFI_UNSUPPORTED;
>>>>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
>>>>>> + if (status != EFI_SUCCESS)
>>>>>> + goto fail;
>>>>>> +
>>>>>> + evt->event_data = ev;
>>>>>> + evt->tagged_event_id = events[event].event_id;
>>>>>> + evt->tagged_event_data_size = events[event].event_data_len;
>>>>>> +
>>>>>> + memcpy(evt->tagged_event_data, events[event].event_data,
>>>>>> + events[event].event_data_len);
>>>>>> +
>>>>>> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
>>>>>> + load_addr, load_size, &evt->event_data);
>>>>>> + efi_bs_call(free_pool, evt);
>>>>>> +
>>>>>> + if (status == EFI_SUCCESS)
>>>>>> + return EFI_SUCCESS;
>>>>>> +
>>>>>> fail:
>>>>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
>>>>>> return status;
>>>>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
>>>>>> index d621bfb719c4..4bf9a76796b7 100644
>>>>>> --- a/drivers/firmware/efi/libstub/efistub.h
>>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h
>>>>>> @@ -954,8 +954,11 @@ union efi_cc_protocol {
>>>>>> } mixed_mode;
>>>>>> };
>>>>>>
>>>>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
>>>>>> +
>>>>>> union efistub_event {
>>>>>> efi_tcg2_event_t tcg2_data;
>>>>>> + efi_cc_event_t cc_data;
>>>>>> };
>>>>>>
>>>>>> struct efistub_measured_event {
>>>>>> --
>>>>>> 2.44.0.278.ge034bb2e1d-goog
>>>>>>
>>>>>>
>>>>> --
>>>>> -Dionna Glaze, PhD (she/her)
>>>
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs
2024-03-05 19:27 ` Kuppuswamy Sathyanarayanan
@ 2024-03-05 19:59 ` Ilias Apalodimas
0 siblings, 0 replies; 21+ messages in thread
From: Ilias Apalodimas @ 2024-03-05 19:59 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan; +Cc: Ard Biesheuvel, linux-efi, Ard Biesheuvel
On Tue, 5 Mar 2024 at 21:27, Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 3/5/24 10:00 AM, Ilias Apalodimas wrote:
> > Hi Ard,
> >
> > On Mon, 4 Mar 2024 at 12:44, Ard Biesheuvel <ardb+git@google.com> wrote:
> >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>
> >> 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.
> >>
> >> 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>
> >> [ardb: Drop code changes, keep typedefs and #define's only]
> >> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >> ---
> >> drivers/firmware/efi/libstub/efistub.h | 79 ++++++++++++++++++++
> >> include/linux/efi.h | 1 +
> >> 2 files changed, 80 insertions(+)
> >>
> > [...]
> >
> >> +
> >> +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];
> > We should define this as a flexible array member instead of a zero-length array?
> > The spec is funny and defines this as event_data[1]. I think we aren't
> > using the sizeof(struct efi_cc_event) anywhere, so if this struct is
> > not used as a member of another struct or an array we can omit it
> > entirely.
>
> Flexible array is also fine or we can just add a comment like "u8[] event follows here"
> like in struct efi_tcg2_event .
Yes. I just noticed I had a typo above. I meant to write "if this
struct *is* used as a member of another struct or an array we can omit
it entirely."
>
> >
> > [...]
> >
> > Cheers
> > /Ilias
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 18:46 ` Dionna Amalie Glaze
2024-03-05 19:36 ` Kuppuswamy Sathyanarayanan
@ 2024-03-05 21:28 ` Ard Biesheuvel
1 sibling, 0 replies; 21+ messages in thread
From: Ard Biesheuvel @ 2024-03-05 21:28 UTC (permalink / raw)
To: Dionna Amalie Glaze
Cc: Kuppuswamy Sathyanarayanan, Qinkun Bao, linux-efi, Ilias Apalodimas
On Tue, 5 Mar 2024 at 19:46, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
>
> On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >
> >
> > On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
> > > On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
> > >>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> > >>>> From: Ard Biesheuvel <ardb@kernel.org>
> > >>>>
> > >>>> To accommodate confidential compute VMs that expose the simplified CC
> > >>>> measurement protocol instead of the full-blown TCG2 one, fall back to
> > >>>> the former if the latter does not exist.
> > >>>>
> > >>>> The CC protocol was designed to be used in this manner, which is why the
> > >>>> types and prototypes have been kept the same where possible. So reuse
> > >>>> the existing code, and only deviate from the TCG2 code path where
> > >>>> needed.
> > >>>>
> > >>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > >>>> ---
> > >>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> > >>>> drivers/firmware/efi/libstub/efistub.h | 3 +
> > >>>> 2 files changed, 53 insertions(+), 17 deletions(-)
> > >>>>
> > >>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > >>>> index 0dbc9d3f4abd..21f4567324f6 100644
> > >>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > >>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > >>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> > >>>> unsigned long load_size,
> > >>>> enum efistub_event_type event)
> > >>>> {
> > >>>> + union {
> > >>>> + efi_status_t
> > >>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> > >>>> + u64, const union efistub_event *);
> > >>>> + struct { u32 hash_log_extend_event; } mixed_mode;
> > >>>> + } method;
> > >>>> struct efistub_measured_event *evt;
> > >>>> int size = struct_size(evt, tagged_event_data,
> > >>>> events[event].event_data_len);
> > >>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> > >>>> efi_tcg2_protocol_t *tcg2 = NULL;
> > >>>> + union efistub_event ev;
> > >>>> efi_status_t status;
> > >>>> + void *protocol;
> > >>>>
> > >>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> > >>>> if (tcg2) {
> > >>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> > >>>> - (void **)&evt);
> > >>>> - if (status != EFI_SUCCESS)
> > >>>> - goto fail;
> > >>>> -
> > >>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> > >>>> + ev.tcg2_data = (struct efi_tcg2_event){
> > >>>> .event_size = size,
> > >>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> > >>>> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> > >>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> > >>>> .event_header.pcr_index = events[event].pcr_index,
> > >>>> .event_header.event_type = EV_EVENT_TAG,
> > >>>> };
> > >>>> + protocol = tcg2;
> > >>>> + method.hash_log_extend_event =
> > >>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> > >>>> + } else {
> > >>> +Qinkun Bao
> > >>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> > >>> TCG protocol breaks backwards compatibility, it'd be preferable to
> > >>> measure into all the measurement protocols that are present.
> > >> How so? Older kernels will use TCG2 if it exists, and so will new
> > >> kernels. The only difference is that on new kernels, the CC protocol
> > >> will be used in case TCG2 is not implemented.
> > >>
> > >> So the only affected scenario here is a system that today implements
> > >> TCG but not CC, and intends to implement CC later and receive
> > >> measurements into both protocols. Does that really qualify as backward
> > >> compatibility? I'd rather not accommodate future systems that
> > >> implement something that the UEFI spec says they should not.
> > >>
> > >>> The UEFI
> > >>> v2.10 standard says that firmware "should not" provide both, but it is
> > >>> not MUST NOT. Given this and our desire to provide service continuity,
> > >>> I ask that you remove the "else" guard.
> > >>>
> > >> Ignoring the newer protocol if the established one exists is an
> > >> excellent way of making sure this does not happen.
> > >>
> > > The problem is that the protocols are not equivalent, and we disagree
> > > with the standard's claim of "should not" to the point that we're
> > > taking it to the USWG. The "should not" advisement is predicated on
> > > not trusting boot layers to use both protocols when they're both
> > > present, such that you could accidentally miss measuring a
> > > security-critical event. That's a strawman though, since you already
> > > need to develop trust in those boot layers. We have software supply
> > > chain endorsements for tracking that kind of property for use in
> > > attestation verification.
> > >
> > > The CC protocol is useful for hardware-rooted boot measurement, but it
> > > does nothing about the rest of TPM 2.0. There are plenty of users that
> > > want to use a vTPM that's hosted by the VMM but also get an extra
> > > integrity assurance that measurements into TDX RTMRs and attested by
> > > an Intel-rooted key pass an extra level of scrutiny.
> > >
> >
> > If you check the EDK2 part of this support, it also uses if else model.
>
> Yes, we've been discussing this with Intel and they agreed to allow a
> default false build option to measure into both.
>
> > It does not measure both. If there a complete vTPM support, why
> > can't user trust measurements from it? I think the CC vendors will
>
> There are folks who want to do a double-check with TEE quotes, but yes
> I agree in general this is not the best situation. It's a stepping
> stones model rather than scaling Everest in one bound.
> Ideally you'd have a measured and protected TPM implementation with
> adequate security for persistent data so that the
> CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed.
>
> But anyway, the standard is what it is for now, so I wouldn't block
> this patch based on this request. When there's more alignment from the
> UEFI standards working group and an accepted patch into EDK2, then we
> can revisit this in the different boot layers.
>
Yeah some spec guidance on when having both protocols makes sense and
why would help here. If you trust the VMM to operate the vTPM for you,
what is the point of the CC protocol?
For now, I'll go with the changes as proposed, also because we're
close to the merge window. We can always revisit this and backport if
needed.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-05 19:36 ` Kuppuswamy Sathyanarayanan
@ 2024-03-05 21:28 ` Dionna Amalie Glaze
0 siblings, 0 replies; 21+ messages in thread
From: Dionna Amalie Glaze @ 2024-03-05 21:28 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Ard Biesheuvel, Qinkun Bao, linux-efi, Ilias Apalodimas
On Tue, Mar 5, 2024 at 11:36 AM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
>
> On 3/5/24 10:46 AM, Dionna Amalie Glaze wrote:
> > On Tue, Mar 5, 2024 at 10:33 AM Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >>
> >> On 3/5/24 10:00 AM, Dionna Amalie Glaze wrote:
> >>> On Tue, Mar 5, 2024 at 9:47 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >>>> On Tue, 5 Mar 2024 at 18:34, Dionna Amalie Glaze <dionnaglaze@google.com> wrote:
> >>>>> On Mon, Mar 4, 2024 at 2:44 AM Ard Biesheuvel <ardb+git@google.com> wrote:
> >>>>>> From: Ard Biesheuvel <ardb@kernel.org>
> >>>>>>
> >>>>>> To accommodate confidential compute VMs that expose the simplified CC
> >>>>>> measurement protocol instead of the full-blown TCG2 one, fall back to
> >>>>>> the former if the latter does not exist.
> >>>>>>
> >>>>>> The CC protocol was designed to be used in this manner, which is why the
> >>>>>> types and prototypes have been kept the same where possible. So reuse
> >>>>>> the existing code, and only deviate from the TCG2 code path where
> >>>>>> needed.
> >>>>>>
> >>>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >>>>>> ---
> >>>>>> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> >>>>>> drivers/firmware/efi/libstub/efistub.h | 3 +
> >>>>>> 2 files changed, 53 insertions(+), 17 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>>>> index 0dbc9d3f4abd..21f4567324f6 100644
> >>>>>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>>>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> >>>>>> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> >>>>>> unsigned long load_size,
> >>>>>> enum efistub_event_type event)
> >>>>>> {
> >>>>>> + union {
> >>>>>> + efi_status_t
> >>>>>> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> >>>>>> + u64, const union efistub_event *);
> >>>>>> + struct { u32 hash_log_extend_event; } mixed_mode;
> >>>>>> + } method;
> >>>>>> struct efistub_measured_event *evt;
> >>>>>> int size = struct_size(evt, tagged_event_data,
> >>>>>> events[event].event_data_len);
> >>>>>> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> >>>>>> efi_tcg2_protocol_t *tcg2 = NULL;
> >>>>>> + union efistub_event ev;
> >>>>>> efi_status_t status;
> >>>>>> + void *protocol;
> >>>>>>
> >>>>>> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> >>>>>> if (tcg2) {
> >>>>>> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> >>>>>> - (void **)&evt);
> >>>>>> - if (status != EFI_SUCCESS)
> >>>>>> - goto fail;
> >>>>>> -
> >>>>>> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> >>>>>> + ev.tcg2_data = (struct efi_tcg2_event){
> >>>>>> .event_size = size,
> >>>>>> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> >>>>>> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> >>>>>> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> >>>>>> .event_header.pcr_index = events[event].pcr_index,
> >>>>>> .event_header.event_type = EV_EVENT_TAG,
> >>>>>> };
> >>>>>> + protocol = tcg2;
> >>>>>> + method.hash_log_extend_event =
> >>>>>> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> >>>>>> + } else {
> >>>>> +Qinkun Bao
> >>>>> Given that the exclusive or between CC_MEASUREMENT_PROTOCOL and the
> >>>>> TCG protocol breaks backwards compatibility, it'd be preferable to
> >>>>> measure into all the measurement protocols that are present.
> >>>> How so? Older kernels will use TCG2 if it exists, and so will new
> >>>> kernels. The only difference is that on new kernels, the CC protocol
> >>>> will be used in case TCG2 is not implemented.
> >>>>
> >>>> So the only affected scenario here is a system that today implements
> >>>> TCG but not CC, and intends to implement CC later and receive
> >>>> measurements into both protocols. Does that really qualify as backward
> >>>> compatibility? I'd rather not accommodate future systems that
> >>>> implement something that the UEFI spec says they should not.
> >>>>
> >>>>> The UEFI
> >>>>> v2.10 standard says that firmware "should not" provide both, but it is
> >>>>> not MUST NOT. Given this and our desire to provide service continuity,
> >>>>> I ask that you remove the "else" guard.
> >>>>>
> >>>> Ignoring the newer protocol if the established one exists is an
> >>>> excellent way of making sure this does not happen.
> >>>>
> >>> The problem is that the protocols are not equivalent, and we disagree
> >>> with the standard's claim of "should not" to the point that we're
> >>> taking it to the USWG. The "should not" advisement is predicated on
> >>> not trusting boot layers to use both protocols when they're both
> >>> present, such that you could accidentally miss measuring a
> >>> security-critical event. That's a strawman though, since you already
> >>> need to develop trust in those boot layers. We have software supply
> >>> chain endorsements for tracking that kind of property for use in
> >>> attestation verification.
> >>>
> >>> The CC protocol is useful for hardware-rooted boot measurement, but it
> >>> does nothing about the rest of TPM 2.0. There are plenty of users that
> >>> want to use a vTPM that's hosted by the VMM but also get an extra
> >>> integrity assurance that measurements into TDX RTMRs and attested by
> >>> an Intel-rooted key pass an extra level of scrutiny.
> >>>
> >> If you check the EDK2 part of this support, it also uses if else model.
> > Yes, we've been discussing this with Intel and they agreed to allow a
> > default false build option to measure into both.
>
> To make it clear, any plans to update the spec with this requirement?
>
I don't think so. I do agree generally with the "should not" wording
as a long term end goal.
But given both interfaces anyway, I think they should both be used. If
that clarification is required in the spec, I can take it to the USWG.
I will note that the "else" interpretation is not universal, as is the
case in rhboot/shim and grub2.
> >
> >> It does not measure both. If there a complete vTPM support, why
> >> can't user trust measurements from it? I think the CC vendors will
> > There are folks who want to do a double-check with TEE quotes, but yes
> > I agree in general this is not the best situation. It's a stepping
> > stones model rather than scaling Everest in one bound.
> > Ideally you'd have a measured and protected TPM implementation with
> > adequate security for persistent data so that the
> > CC_MEASUREMENT_PROTOCOL is fully redundant and therefore not needed.
> >
> > But anyway, the standard is what it is for now, so I wouldn't block
> > this patch based on this request. When there's more alignment from the
> > UEFI standards working group and an accepted patch into EDK2, then we
> > can revisit this in the different boot layers.
>
> Got it. Personally I think we can consider it, once it is adapted in
> firmware and updated in spec (to make it consistent with firmware
> support). Anyway, since doing it is harmless, I will leave it to the
> maintainers choice.
>
> >
> >> ensure their vTPM implementation is protected from attack from the
> >> host (like implementing it part of firmware or launching it as service in
> >> a separate VM).
> >>
> >>>>>> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> >>>>>> + efi_cc_protocol_t *cc = NULL;
> >>>>>>
> >>>>>> - evt->tagged_event_id = events[event].event_id;
> >>>>>> - evt->tagged_event_data_size = events[event].event_data_len;
> >>>>>> -
> >>>>>> - memcpy(evt->tagged_event_data, events[event].event_data,
> >>>>>> - events[event].event_data_len);
> >>>>>> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> >>>>>> + if (!cc)
> >>>>>> + return EFI_UNSUPPORTED;
> >>>>>>
> >>>>>> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> >>>>>> - load_addr, load_size, &evt->event_data.tcg2_data);
> >>>>>> - efi_bs_call(free_pool, evt);
> >>>>>> + ev.cc_data = (struct efi_cc_event){
> >>>>>> + .event_size = size,
> >>>>>> + .event_header.header_size = sizeof(ev.cc_data.event_header),
> >>>>>> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> >>>>>> + .event_header.event_type = EV_EVENT_TAG,
> >>>>>> + };
> >>>>>>
> >>>>>> + status = efi_call_proto(cc, map_pcr_to_mr_index,
> >>>>>> + events[event].pcr_index,
> >>>>>> + &ev.cc_data.event_header.mr_index);
> >>>>>> if (status != EFI_SUCCESS)
> >>>>>> goto fail;
> >>>>>> - return EFI_SUCCESS;
> >>>>>> +
> >>>>>> + protocol = cc;
> >>>>>> + method.hash_log_extend_event =
> >>>>>> + (void *)efi_table_attr(cc, hash_log_extend_event);
> >>>>>> }
> >>>>>>
> >>>>>> - return EFI_UNSUPPORTED;
> >>>>>> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> >>>>>> + if (status != EFI_SUCCESS)
> >>>>>> + goto fail;
> >>>>>> +
> >>>>>> + evt->event_data = ev;
> >>>>>> + evt->tagged_event_id = events[event].event_id;
> >>>>>> + evt->tagged_event_data_size = events[event].event_data_len;
> >>>>>> +
> >>>>>> + memcpy(evt->tagged_event_data, events[event].event_data,
> >>>>>> + events[event].event_data_len);
> >>>>>> +
> >>>>>> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> >>>>>> + load_addr, load_size, &evt->event_data);
> >>>>>> + efi_bs_call(free_pool, evt);
> >>>>>> +
> >>>>>> + if (status == EFI_SUCCESS)
> >>>>>> + return EFI_SUCCESS;
> >>>>>> +
> >>>>>> fail:
> >>>>>> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> >>>>>> return status;
> >>>>>> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> >>>>>> index d621bfb719c4..4bf9a76796b7 100644
> >>>>>> --- a/drivers/firmware/efi/libstub/efistub.h
> >>>>>> +++ b/drivers/firmware/efi/libstub/efistub.h
> >>>>>> @@ -954,8 +954,11 @@ union efi_cc_protocol {
> >>>>>> } mixed_mode;
> >>>>>> };
> >>>>>>
> >>>>>> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> >>>>>> +
> >>>>>> union efistub_event {
> >>>>>> efi_tcg2_event_t tcg2_data;
> >>>>>> + efi_cc_event_t cc_data;
> >>>>>> };
> >>>>>>
> >>>>>> struct efistub_measured_event {
> >>>>>> --
> >>>>>> 2.44.0.278.ge034bb2e1d-goog
> >>>>>>
> >>>>>>
> >>>>> --
> >>>>> -Dionna Glaze, PhD (she/her)
> >>>
> >> --
> >> Sathyanarayanan Kuppuswamy
> >> Linux Kernel Developer
> >>
> >
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
>
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent
2024-03-04 10:44 ` [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent Ard Biesheuvel
2024-03-05 17:34 ` Dionna Amalie Glaze
@ 2024-03-05 21:39 ` Kuppuswamy Sathyanarayanan
1 sibling, 0 replies; 21+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2024-03-05 21:39 UTC (permalink / raw)
To: Ard Biesheuvel, linux-efi; +Cc: Ard Biesheuvel, Ilias Apalodimas
On 3/4/24 2:44 AM, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
>
> To accommodate confidential compute VMs that expose the simplified CC
> measurement protocol instead of the full-blown TCG2 one, fall back to
> the former if the latter does not exist.
>
> The CC protocol was designed to be used in this manner, which is why the
> types and prototypes have been kept the same where possible. So reuse
> the existing code, and only deviate from the TCG2 code path where
> needed.
>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
Looks fine to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> drivers/firmware/efi/libstub/efi-stub-helper.c | 67 +++++++++++++++-----
> drivers/firmware/efi/libstub/efistub.h | 3 +
> 2 files changed, 53 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 0dbc9d3f4abd..21f4567324f6 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -223,44 +223,77 @@ static efi_status_t efi_measure_tagged_event(unsigned long load_addr,
> unsigned long load_size,
> enum efistub_event_type event)
> {
> + union {
> + efi_status_t
> + (__efiapi *hash_log_extend_event)(void *, u64, efi_physical_addr_t,
> + u64, const union efistub_event *);
> + struct { u32 hash_log_extend_event; } mixed_mode;
> + } method;
> struct efistub_measured_event *evt;
> int size = struct_size(evt, tagged_event_data,
> events[event].event_data_len);
> efi_guid_t tcg2_guid = EFI_TCG2_PROTOCOL_GUID;
> efi_tcg2_protocol_t *tcg2 = NULL;
> + union efistub_event ev;
> efi_status_t status;
> + void *protocol;
>
> efi_bs_call(locate_protocol, &tcg2_guid, NULL, (void **)&tcg2);
> if (tcg2) {
> - status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size,
> - (void **)&evt);
> - if (status != EFI_SUCCESS)
> - goto fail;
> -
> - evt->event_data.tcg2_data = (struct efi_tcg2_event){
> + ev.tcg2_data = (struct efi_tcg2_event){
> .event_size = size,
> - .event_header.header_size = sizeof(evt->event_data.tcg2_data.event_header),
> + .event_header.header_size = sizeof(ev.tcg2_data.event_header),
> .event_header.header_version = EFI_TCG2_EVENT_HEADER_VERSION,
> .event_header.pcr_index = events[event].pcr_index,
> .event_header.event_type = EV_EVENT_TAG,
> };
> + protocol = tcg2;
> + method.hash_log_extend_event =
> + (void *)efi_table_attr(tcg2, hash_log_extend_event);
> + } else {
> + efi_guid_t cc_guid = EFI_CC_MEASUREMENT_PROTOCOL_GUID;
> + efi_cc_protocol_t *cc = NULL;
>
> - evt->tagged_event_id = events[event].event_id;
> - evt->tagged_event_data_size = events[event].event_data_len;
> -
> - memcpy(evt->tagged_event_data, events[event].event_data,
> - events[event].event_data_len);
> + efi_bs_call(locate_protocol, &cc_guid, NULL, (void **)&cc);
> + if (!cc)
> + return EFI_UNSUPPORTED;
>
> - status = efi_call_proto(tcg2, hash_log_extend_event, 0,
> - load_addr, load_size, &evt->event_data.tcg2_data);
> - efi_bs_call(free_pool, evt);
> + ev.cc_data = (struct efi_cc_event){
> + .event_size = size,
> + .event_header.header_size = sizeof(ev.cc_data.event_header),
> + .event_header.header_version = EFI_CC_EVENT_HEADER_VERSION,
> + .event_header.event_type = EV_EVENT_TAG,
> + };
>
> + status = efi_call_proto(cc, map_pcr_to_mr_index,
> + events[event].pcr_index,
> + &ev.cc_data.event_header.mr_index);
> if (status != EFI_SUCCESS)
> goto fail;
> - return EFI_SUCCESS;
> +
> + protocol = cc;
> + method.hash_log_extend_event =
> + (void *)efi_table_attr(cc, hash_log_extend_event);
> }
>
> - return EFI_UNSUPPORTED;
> + status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, size, (void **)&evt);
> + if (status != EFI_SUCCESS)
> + goto fail;
> +
> + evt->event_data = ev;
> + evt->tagged_event_id = events[event].event_id;
> + evt->tagged_event_data_size = events[event].event_data_len;
> +
> + memcpy(evt->tagged_event_data, events[event].event_data,
> + events[event].event_data_len);
> +
> + status = efi_fn_call(&method, hash_log_extend_event, protocol, 0,
> + load_addr, load_size, &evt->event_data);
> + efi_bs_call(free_pool, evt);
> +
> + if (status == EFI_SUCCESS)
> + return EFI_SUCCESS;
> +
> fail:
> efi_warn("Failed to measure data for event %d: 0x%lx\n", event, status);
> return status;
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index d621bfb719c4..4bf9a76796b7 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -954,8 +954,11 @@ union efi_cc_protocol {
> } mixed_mode;
> };
>
> +static_assert(sizeof(efi_tcg2_event_t) == sizeof(efi_cc_event_t));
> +
> union efistub_event {
> efi_tcg2_event_t tcg2_data;
> + efi_cc_event_t cc_data;
> };
>
> struct efistub_measured_event {
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2024-03-05 21:39 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-04 10:44 [PATCH 0/4] efi/libstub: Fall back to CC proto for measurement Ard Biesheuvel
2024-03-04 10:44 ` [PATCH 1/4] efi/libstub: Fold efi_tcg2_tagged_event into efi_measured_event Ard Biesheuvel
2024-03-05 4:30 ` Kuppuswamy Sathyanarayanan
2024-03-05 8:21 ` Ard Biesheuvel
2024-03-05 19:19 ` Kuppuswamy Sathyanarayanan
2024-03-04 10:44 ` [PATCH 2/4] efi/libstub: Add Confidential Computing (CC) measurement typedefs Ard Biesheuvel
2024-03-05 18:00 ` Ilias Apalodimas
2024-03-05 19:27 ` Kuppuswamy Sathyanarayanan
2024-03-05 19:59 ` Ilias Apalodimas
2024-03-04 10:44 ` [PATCH 3/4] efi/libstub: Measure into CC protocol if TCG2 protocol is absent Ard Biesheuvel
2024-03-05 17:34 ` Dionna Amalie Glaze
2024-03-05 17:47 ` Ard Biesheuvel
2024-03-05 17:55 ` Ilias Apalodimas
2024-03-05 18:00 ` Dionna Amalie Glaze
2024-03-05 18:33 ` Kuppuswamy Sathyanarayanan
2024-03-05 18:46 ` Dionna Amalie Glaze
2024-03-05 19:36 ` Kuppuswamy Sathyanarayanan
2024-03-05 21:28 ` Dionna Amalie Glaze
2024-03-05 21:28 ` Ard Biesheuvel
2024-03-05 21:39 ` Kuppuswamy Sathyanarayanan
2024-03-04 10:44 ` [PATCH 4/4] efi/libstub: Add get_event_log() support for CC platforms Ard Biesheuvel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.