[PATCH v4 0/3] Allow guest access to EFI confidential computing secret area

[PATCH v4 0/3] Allow guest access to EFI confidential computing secret area

[Dov Murik]

Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
Virtualization) allows guest owners to inject secrets into the VMs
memory without the host/hypervisor being able to read them.  In SEV,
secret injection is performed early in the VM launch process, before the
guest starts running.

OVMF already reserves designated area for secret injection (in its
AmdSev package; see edk2 commit 01726b6d23d4 "OvmfPkg/AmdSev: Expose the
Sev Secret area using a configuration table" [1]), but the secrets were
not available in the guest kernel.

The patch series copies the secrets from the EFI-provided memory to
kernel reserved memory, and optionally exposes them to userspace via
securityfs using a new efi_secret kernel module.

The first patch in efi/libstub copies the secret area from the EFI
memory to specially allocated memory; the second patch reserves that
memory block; and the third patch introduces the new efi_secret module
that exposes the content of the secret entries as securityfs files, and
allows clearing out secrets with a file unlink interface.

As a usage example, consider a guest performing computations on
encrypted files.  The Guest Owner provides the decryption key (= secret)
using the secret injection mechanism.  The guest application reads the
secret from the efi_secret filesystem and proceeds to decrypt the files
into memory and then performs the needed computations on the content.

In this example, the host can't read the files from the disk image
because they are encrypted.  Host can't read the decryption key because
it is passed using the secret injection mechanism (= secure channel).
Host can't read the decrypted content from memory because it's a
confidential (memory-encrypted) guest.

This has been tested with AMD SEV and SEV-ES guests, but the kernel side
of handling the secret area has no SEV-specific dependencies, and
therefore might be usable (perhaps with minor changes) for any
confidential computing hardware that can publish the secret area via the
standard EFI config table entry.

To enable this functionality, set CONFIG_EFI_SECRET=m when building the
guest kernel.

Here is a simple example for usage of the efi_secret module in a guest
to which an EFI secret area with 4 secrets was injected during launch:

# modprobe efi_secret
# ls -la /sys/kernel/security/coco/efi_secret
total 0
drwxr-xr-x 2 root root 0 Jun 28 11:54 .
drwxr-xr-x 3 root root 0 Jun 28 11:54 ..
-r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
-r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
-r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
-r--r----- 1 root root 0 Jun 28 11:54 e6f5a162-d67f-4750-a67c-5d065f2a9910

# xxd /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
00000000: 7468 6573 652d 6172 652d 7468 652d 6b61  these-are-the-ka
00000010: 7461 2d73 6563 7265 7473 0001 0203 0405  ta-secrets......
00000020: 0607                                     ..

# rm /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910

# ls -la /sys/kernel/security/coco/efi_secret
total 0
drwxr-xr-x 2 root root 0 Jun 28 11:55 .
drwxr-xr-x 3 root root 0 Jun 28 11:54 ..
-r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
-r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
-r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2


[1] https://github.com/tianocore/edk2/commit/01726b6d23d4

---

v4 changes:
 - Guard all the new EFI and efi-stub code (patches 1+2) with #ifdef
   CONFIG_EFI_COCO_SECRET (thanks Greg KH).  Selecting
   CONFIG_EFI_SECRET=m (patch 3) will enable the EFI parts as well.
 - Guard call to clflush_cache_range() with #ifdef CONFIG_X86
   (Reported-by: kernel test robot <lkp@intel.com>)

v3: https://lore.kernel.org/linux-coco/20211014130848.592611-1-dovmurik@linux.ibm.com/
v3 changes:
 - Rename the module to efi_secret
 - Remove the exporting of clean_cache_range
 - Use clflush_cache_range in wipe_memory
 - Document function wipe_memory
 - Initialize efi.coco_secret to EFI_INVALID_TABLE_ADDR to correctly detect
   when there's no secret area published in the EFI configuration tables

v2: https://lore.kernel.org/linux-coco/20211007061838.1381129-1-dovmurik@linux.ibm.com
v2 changes:
 - Export clean_cache_range()
 - When deleteing a secret, call clean_cache_range() after explicit_memzero
 - Add Documentation/ABI/testing/securityfs-coco-sev_secret

v1: https://lore.kernel.org/linux-coco/20210809190157.279332-1-dovmurik@linux.ibm.com/

RFC: https://lore.kernel.org/linux-coco/20210628183431.953934-1-dovmurik@linux.ibm.com/



Dov Murik (3):
  efi/libstub: Copy confidential computing secret area
  efi: Reserve confidential computing secret area
  virt: Add efi_secret module to expose confidential computing secrets

 .../ABI/testing/securityfs-coco-efi_secret    |  50 +++
 arch/x86/platform/efi/efi.c                   |   3 +
 drivers/firmware/efi/Kconfig                  |  12 +
 drivers/firmware/efi/Makefile                 |   1 +
 drivers/firmware/efi/coco.c                   |  41 +++
 drivers/firmware/efi/efi.c                    |   8 +
 drivers/firmware/efi/libstub/Makefile         |   1 +
 drivers/firmware/efi/libstub/coco.c           |  68 ++++
 drivers/firmware/efi/libstub/efi-stub.c       |   2 +
 drivers/firmware/efi/libstub/efistub.h        |   6 +
 drivers/firmware/efi/libstub/x86-stub.c       |   2 +
 drivers/virt/Kconfig                          |   3 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/coco/efi_secret/Kconfig          |  11 +
 drivers/virt/coco/efi_secret/Makefile         |   2 +
 drivers/virt/coco/efi_secret/efi_secret.c     | 326 ++++++++++++++++++
 include/linux/efi.h                           |  16 +
 17 files changed, 553 insertions(+)
 create mode 100644 Documentation/ABI/testing/securityfs-coco-efi_secret
 create mode 100644 drivers/firmware/efi/coco.c
 create mode 100644 drivers/firmware/efi/libstub/coco.c
 create mode 100644 drivers/virt/coco/efi_secret/Kconfig
 create mode 100644 drivers/virt/coco/efi_secret/Makefile
 create mode 100644 drivers/virt/coco/efi_secret/efi_secret.c


base-commit: 348949d9a4440abdab3b1dc99a9bb660e8c7da7c
-- 
2.25.1

[PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Dov Murik]

Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
Virtualization) allows a guest owner to inject secrets into the VMs
memory without the host/hypervisor being able to read them.

Firmware support for secret injection is available in OVMF, which
reserves a memory area for secret injection and includes a pointer to it
the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
However, OVMF doesn't force the guest OS to keep this memory area
reserved.

If EFI exposes such a table entry, efi/libstub will copy this area to a
reserved memory for future use inside the kernel.

A pointer to the new copy is kept in the EFI table under
LINUX_EFI_COCO_SECRET_AREA_GUID.

The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/Kconfig            | 12 +++++
 drivers/firmware/efi/libstub/Makefile   |  1 +
 drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c |  2 +
 drivers/firmware/efi/libstub/efistub.h  |  6 +++
 drivers/firmware/efi/libstub/x86-stub.c |  2 +
 include/linux/efi.h                     |  6 +++
 7 files changed, 97 insertions(+)
 create mode 100644 drivers/firmware/efi/libstub/coco.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2c3dac5ecb36..68d1c5e6a7b5 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
 
 	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
 	  information.
+
+config EFI_COCO_SECRET
+	bool "Copy and reserve EFI Confidential Computing secret area"
+	depends on EFI
+	default n
+	help
+	  Copy memory reserved by EFI for Confidential Computing (coco)
+	  injected secrets, if EFI exposes such a table entry.
+
+	  If you say Y here, the EFI stub copy the EFI secret area (if
+	  available) and reserve it for use inside the kernel.  This will
+	  allow the virt/coo/efi_secret module to access the secrets.
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d0537573501e..fdada3fd5d9b 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
 lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
 				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
 
+lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
 lib-$(CONFIG_ARM)		+= arm32-stub.o
 lib-$(CONFIG_ARM64)		+= arm64-stub.o
 lib-$(CONFIG_X86)		+= x86-stub.o
diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
new file mode 100644
index 000000000000..bf546b6a3f72
--- /dev/null
+++ b/drivers/firmware/efi/libstub/coco.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing (coco) secret area handling
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#include <linux/efi.h>
+#include <linux/sizes.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
+	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
+
+/**
+ * struct efi_coco_secret_table - EFI config table that points to the
+ * confidential computing secret area. The guid
+ * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
+ * @base:	Physical address of the EFI secret area
+ * @size:	Size (in bytes) of the EFI secret area
+ */
+struct efi_coco_secret_table {
+	u64 base;
+	u64 size;
+} __attribute((packed));
+
+/*
+ * Create a copy of EFI's confidential computing secret area (if available) so
+ * that the secrets are accessible in the kernel after ExitBootServices.
+ */
+void efi_copy_coco_secret_area(void)
+{
+	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
+	efi_status_t status;
+	struct efi_coco_secret_table *secret_table;
+	struct linux_efi_coco_secret_area *secret_area;
+
+	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
+	if (!secret_table)
+		return;
+
+	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
+		return;
+
+	/* Allocate space for the secret area and copy it */
+	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
+
+	if (status != EFI_SUCCESS) {
+		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
+		return;
+	}
+
+	secret_area->size = secret_table->size;
+	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
+
+	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
+	if (status != EFI_SUCCESS)
+		goto err_free;
+
+	return;
+
+err_free:
+	efi_bs_call(free_pool, secret_area);
+}
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..18b3acd15c85 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -205,6 +205,8 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	/* 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 cde0a2ef507d..a23771547790 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -858,4 +858,10 @@ efi_enable_reset_attack_mitigation(void) { }
 
 void efi_retrieve_tpm2_eventlog(void);
 
+#ifdef CONFIG_EFI_COCO_SECRET
+void efi_copy_coco_secret_area(void);
+#else
+static inline void efi_copy_coco_secret_area(void) { }
+#endif
+
 #endif
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..4ad85e1b6191 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -793,6 +793,8 @@ unsigned long efi_main(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6b5d36babfcc..9021dd521302 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -359,6 +359,7 @@ void efi_native_runtime_setup(void);
 #define LINUX_EFI_MEMRESERVE_TABLE_GUID		EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5,  0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
 #define LINUX_EFI_INITRD_MEDIA_GUID		EFI_GUID(0x5568e427, 0x68fc, 0x4f3d,  0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
 #define LINUX_EFI_MOK_VARIABLE_TABLE_GUID	EFI_GUID(0xc451ed2b, 0x9694, 0x45d3,  0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89)
+#define LINUX_EFI_COCO_SECRET_AREA_GUID		EFI_GUID(0x940ed1e9, 0xd3da, 0x408b,  0xb3, 0x07, 0xe3, 0x2d, 0x25, 0x4a, 0x65, 0x16)
 
 /* OEM GUIDs */
 #define DELLEMC_EFI_RCI2_TABLE_GUID		EFI_GUID(0x2d9f28a2, 0xa886, 0x456a,  0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
@@ -1282,4 +1283,9 @@ static inline struct efi_mokvar_table_entry *efi_mokvar_entry_find(
 }
 #endif
 
+struct linux_efi_coco_secret_area {
+	u32	size;
+	u8	area[];
+};
+
 #endif /* _LINUX_EFI_H */
-- 
2.25.1

[PATCH v4 2/3] efi: Reserve confidential computing secret area

[Dov Murik]

When efi-stub copies an EFI-provided confidential computing (coco)
secret area, reserve that memory block for future use within the kernel.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 arch/x86/platform/efi/efi.c   |  3 +++
 drivers/firmware/efi/Makefile |  1 +
 drivers/firmware/efi/coco.c   | 41 +++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/efi.c    |  8 +++++++
 include/linux/efi.h           | 10 +++++++++
 5 files changed, 63 insertions(+)
 create mode 100644 drivers/firmware/efi/coco.c

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..1591d67e0bcd 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -93,6 +93,9 @@ static const unsigned long * const efi_tables[] = {
 #ifdef CONFIG_LOAD_UEFI_KEYS
 	&efi.mokvar_table,
 #endif
+#ifdef CONFIG_EFI_COCO_SECRET
+	&efi.coco_secret,
+#endif
 };
 
 u64 efi_setup;		/* efi setup_data physical address */
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index c02ff25dd477..49c4a8c0bfc4 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -32,6 +32,7 @@ obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
 obj-$(CONFIG_EFI_RCI2_TABLE)		+= rci2-table.o
 obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
 obj-$(CONFIG_LOAD_UEFI_KEYS)		+= mokvar-table.o
+obj-$(CONFIG_EFI_COCO_SECRET)		+= coco.o
 
 fake_map-y				+= fake_mem.o
 fake_map-$(CONFIG_X86)			+= x86_fake_mem.o
diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c
new file mode 100644
index 000000000000..42f477d6188c
--- /dev/null
+++ b/drivers/firmware/efi/coco.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing (coco) secret area handling
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/memblock.h>
+#include <asm/early_ioremap.h>
+
+/*
+ * Reserve the confidential computing secret area memory
+ */
+int __init efi_coco_secret_area_reserve(void)
+{
+	struct linux_efi_coco_secret_area *secret_area;
+	unsigned long secret_area_size;
+
+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	secret_area = early_memremap(efi.coco_secret, sizeof(*secret_area));
+	if (!secret_area) {
+		pr_err("Failed to map confidential computing secret area\n");
+		efi.coco_secret = EFI_INVALID_TABLE_ADDR;
+		return -ENOMEM;
+	}
+
+	secret_area_size = sizeof(*secret_area) + secret_area->size;
+	memblock_reserve(efi.coco_secret, secret_area_size);
+
+	pr_info("Reserved memory of EFI-provided confidential computing secret area");
+
+	early_memunmap(secret_area, sizeof(*secret_area));
+	return 0;
+}
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 847f33ffc4ae..31bdae2afc47 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -46,6 +46,9 @@ struct efi __read_mostly efi = {
 #ifdef CONFIG_LOAD_UEFI_KEYS
 	.mokvar_table		= EFI_INVALID_TABLE_ADDR,
 #endif
+#ifdef CONFIG_EFI_COCO_SECRET
+	.coco_secret		= EFI_INVALID_TABLE_ADDR,
+#endif
 };
 EXPORT_SYMBOL(efi);
 
@@ -525,6 +528,9 @@ static const efi_config_table_type_t common_tables[] __initconst = {
 #endif
 #ifdef CONFIG_LOAD_UEFI_KEYS
 	{LINUX_EFI_MOK_VARIABLE_TABLE_GUID,	&efi.mokvar_table,	"MOKvar"	},
+#endif
+#ifdef CONFIG_EFI_COCO_SECRET
+	{LINUX_EFI_COCO_SECRET_AREA_GUID,	&efi.coco_secret,	"CocoSecret"	},
 #endif
 	{},
 };
@@ -613,6 +619,8 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 	efi_tpm_eventlog_init();
 
+	efi_coco_secret_area_reserve();
+
 	if (mem_reserve != EFI_INVALID_TABLE_ADDR) {
 		unsigned long prsv = mem_reserve;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9021dd521302..6e57dd083e25 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -550,6 +550,7 @@ extern struct efi {
 	unsigned long			tpm_log;		/* TPM2 Event Log table */
 	unsigned long			tpm_final_log;		/* TPM2 Final Events Log table */
 	unsigned long			mokvar_table;		/* MOK variable config table */
+	unsigned long			coco_secret;		/* Confidential computing secret table */
 
 	efi_get_time_t			*get_time;
 	efi_set_time_t			*set_time;
@@ -1288,4 +1289,13 @@ struct linux_efi_coco_secret_area {
 	u8	area[];
 };
 
+#ifdef CONFIG_EFI_COCO_SECRET
+extern int efi_coco_secret_area_reserve(void);
+#else
+static inline int efi_coco_secret_area_reserve(void)
+{
+	return 0;
+}
+#endif
+
 #endif /* _LINUX_EFI_H */
-- 
2.25.1

[PATCH v4 3/3] virt: Add efi_secret module to expose confidential computing secrets

[Dov Murik]

The new efi_secret module exposes the confidential computing (coco)
EFI secret area via securityfs interface.

When the module is loaded (and securityfs is mounted, typically under
/sys/kernel/security), a "coco/efi_secret" directory is created in
securityfs.  In it, a file is created for each secret entry.  The name
of each such file is the GUID of the secret entry, and its content is
the secret data.

This allows applications running in a confidential computing setting to
read secrets provided by the guest owner via a secure secret injection
mechanism (such as AMD SEV's LAUNCH_SECRET command).

Removing (unlinking) files in the "coco/efi_secret" directory will zero
out the secret in memory, and remove the filesystem entry.  If the
module is removed and loaded again, that secret will not appear in the
filesystem.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 .../ABI/testing/securityfs-coco-efi_secret    |  50 +++
 drivers/virt/Kconfig                          |   3 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/coco/efi_secret/Kconfig          |  11 +
 drivers/virt/coco/efi_secret/Makefile         |   2 +
 drivers/virt/coco/efi_secret/efi_secret.c     | 326 ++++++++++++++++++
 6 files changed, 393 insertions(+)
 create mode 100644 Documentation/ABI/testing/securityfs-coco-efi_secret
 create mode 100644 drivers/virt/coco/efi_secret/Kconfig
 create mode 100644 drivers/virt/coco/efi_secret/Makefile
 create mode 100644 drivers/virt/coco/efi_secret/efi_secret.c

diff --git a/Documentation/ABI/testing/securityfs-coco-efi_secret b/Documentation/ABI/testing/securityfs-coco-efi_secret
new file mode 100644
index 000000000000..ae56976db1bc
--- /dev/null
+++ b/Documentation/ABI/testing/securityfs-coco-efi_secret
@@ -0,0 +1,50 @@
+What:		security/coco/efi_secret
+Date:		October 2021
+Contact:	Dov Murik <dovmurik@linux.ibm.com>
+Description:
+		Exposes confidential computing (coco) EFI secrets to
+		userspace via securityfs.
+
+		EFI can declare memory area used by confidential computing
+		platforms (such as AMD SEV and SEV-ES) for secret injection by
+		the Guest Owner during VM's launch.  The secrets are encrypted
+		by the Guest Owner and decrypted inside the trusted enclave,
+		and therefore are not readable by the untrusted host.
+
+		The efi_secret module exposes the secrets to userspace.  Each
+		secret appears as a file under <securityfs>/coco/efi_secret,
+		where the filename is the GUID of the entry in the secrets
+		table.
+
+		Two operations are supported for the files: read and unlink.
+		Reading the file returns the content of secret entry.
+		Unlinking the file overwrites the secret data with zeroes and
+		removes the entry from the filesystem.  A secret cannot be read
+		after it has been unlinked.
+
+		For example, listing the available secrets::
+
+		  # modprobe efi_secret
+		  # ls -l /sys/kernel/security/coco/efi_secret
+		  -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
+		  -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
+		  -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
+		  -r--r----- 1 root root 0 Jun 28 11:54 e6f5a162-d67f-4750-a67c-5d065f2a9910
+
+		Reading the secret data by reading a file::
+
+		  # cat /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
+		  the-content-of-the-secret-data
+
+		Wiping a secret by unlinking a file::
+
+		  # rm /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
+		  # ls -l /sys/kernel/security/coco/efi_secret
+		  -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
+		  -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
+		  -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
+
+		Note: The binary format of the secrets table injected by the
+		Guest Owner is described in
+		drivers/virt/coco/efi_secret/efi_secret.c under "Structure of
+		the EFI secret area".
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 8061e8ef449f..fe7a6579b974 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig"
 source "drivers/virt/nitro_enclaves/Kconfig"
 
 source "drivers/virt/acrn/Kconfig"
+
+source "drivers/virt/coco/efi_secret/Kconfig"
+
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 3e272ea60cd9..efdb015783f9 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -8,3 +8,4 @@ obj-y				+= vboxguest/
 
 obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
 obj-$(CONFIG_ACRN_HSM)		+= acrn/
+obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
diff --git a/drivers/virt/coco/efi_secret/Kconfig b/drivers/virt/coco/efi_secret/Kconfig
new file mode 100644
index 000000000000..a39a5a90a1e5
--- /dev/null
+++ b/drivers/virt/coco/efi_secret/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config EFI_SECRET
+	tristate "EFI secret area securityfs support"
+	depends on EFI
+	select EFI_COCO_SECRET
+	select SECURITYFS
+	help
+	  This is a driver for accessing the EFI secret area via securityfs.
+
+	  To compile this driver as a module, choose M here.
+	  The module will be called efi_secret.
diff --git a/drivers/virt/coco/efi_secret/Makefile b/drivers/virt/coco/efi_secret/Makefile
new file mode 100644
index 000000000000..c7047ce804f7
--- /dev/null
+++ b/drivers/virt/coco/efi_secret/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_EFI_SECRET) += efi_secret.o
diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
new file mode 100644
index 000000000000..9128623b5e4e
--- /dev/null
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * efi_secret module
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+/**
+ * DOC: efi_secret: Allow reading EFI confidential computing (coco) secret area
+ * via securityfs interface.
+ *
+ * When the module is loaded (and securityfs is mounted, typically under
+ * /sys/kernel/security), a "coco/efi_secret" directory is created in
+ * securityfs.  In it, a file is created for each secret entry.  The name of
+ * each such file is the GUID of the secret entry, and its content is the
+ * secret data.
+ */
+
+#include <linux/seq_file.h>
+#include <linux/fs.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/security.h>
+#include <linux/efi.h>
+#include <asm/cacheflush.h>
+
+#define EFI_SECRET_NUM_FILES 64
+
+#define EFI_SECRET_TABLE_HEADER_GUID \
+	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
+
+struct efi_secret {
+	struct dentry *coco_dir;
+	struct dentry *fs_dir;
+	struct dentry *fs_files[EFI_SECRET_NUM_FILES];
+	struct linux_efi_coco_secret_area *secret_area;
+};
+
+/*
+ * Structure of the EFI secret area
+ *
+ * Offset   Length
+ * (bytes)  (bytes)  Usage
+ * -------  -------  -----
+ *       0       16  Secret table header GUID (must be 1e74f542-71dd-4d66-963e-ef4287ff173b)
+ *      16        4  Length of bytes of the entire secret area
+ *
+ *      20       16  First secret entry's GUID
+ *      36        4  First secret entry's length in bytes (= 16 + 4 + x)
+ *      40        x  First secret entry's data
+ *
+ *    40+x       16  Second secret entry's GUID
+ *    56+x        4  Second secret entry's length in bytes (= 16 + 4 + y)
+ *    60+x        y  Second secret entry's data
+ *
+ * (... and so on for additional entries)
+ *
+ * The GUID of each secret entry designates the usage of the secret data.
+ */
+
+/**
+ * struct secret_header - Header of entire secret area; this should be followed
+ * by instances of struct secret_entry.
+ * @guid:	Must be EFI_SECRET_TABLE_HEADER_GUID
+ * @len:	Length in bytes of entire secret area, including header
+ */
+struct secret_header {
+	efi_guid_t guid;
+	u32 len;
+} __attribute((packed));
+
+/**
+ * struct secret_entry - Holds one secret entry
+ * @guid:	Secret-specific GUID (or NULL_GUID if this secret entry was deleted)
+ * @len:	Length of secret entry, including its guid and len fields
+ * @data:	The secret data (full of zeros if this secret entry was deleted)
+ */
+struct secret_entry {
+	efi_guid_t guid;
+	u32 len;
+	u8 data[];
+} __attribute((packed));
+
+static size_t secret_entry_data_len(struct secret_entry *e)
+{
+	return e->len - sizeof(*e);
+}
+
+static struct efi_secret the_efi_secret;
+
+static inline struct efi_secret *efi_secret_get(void)
+{
+	return &the_efi_secret;
+}
+
+static int efi_secret_bin_file_show(struct seq_file *file, void *data)
+{
+	struct secret_entry *e = file->private;
+
+	if (e)
+		seq_write(file, e->data, secret_entry_data_len(e));
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(efi_secret_bin_file);
+
+/*
+ * Overwrite memory content with zeroes, and ensure that dirty cache lines are
+ * actually written back to memory, to clear out the secret.
+ */
+static void wipe_memory(void *addr, size_t size)
+{
+	memzero_explicit(addr, size);
+#ifdef CONFIG_X86
+	clflush_cache_range(addr, size);
+#endif
+}
+
+static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct efi_secret *s = efi_secret_get();
+	struct inode *inode = d_inode(dentry);
+	struct secret_entry *e = (struct secret_entry *)inode->i_private;
+	int i;
+
+	if (e) {
+		/* Zero out the secret data */
+		wipe_memory(e->data, secret_entry_data_len(e));
+		e->guid = NULL_GUID;
+	}
+
+	inode->i_private = NULL;
+
+	for (i = 0; i < EFI_SECRET_NUM_FILES; i++)
+		if (s->fs_files[i] == dentry)
+			s->fs_files[i] = NULL;
+
+	/*
+	 * securityfs_remove tries to lock the directory's inode, but we reach
+	 * the unlink callback when it's already locked
+	 */
+	inode_unlock(dir);
+	securityfs_remove(dentry);
+	inode_lock(dir);
+
+	return 0;
+}
+
+static const struct inode_operations efi_secret_dir_inode_operations = {
+	.lookup         = simple_lookup,
+	.unlink         = efi_secret_unlink,
+};
+
+static int efi_secret_map_area(void)
+{
+	struct efi_secret *s = efi_secret_get();
+	struct linux_efi_coco_secret_area *secret_area;
+	u32 secret_area_size;
+
+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) {
+		pr_err("Secret area address is not available\n");
+		return -EINVAL;
+	}
+
+	secret_area = memremap(efi.coco_secret, sizeof(*secret_area), MEMREMAP_WB);
+	if (secret_area == NULL) {
+		pr_err("Could not map secret area header\n");
+		return -ENOMEM;
+	}
+
+	secret_area_size = sizeof(*secret_area) + secret_area->size;
+	memunmap(secret_area);
+
+	secret_area = memremap(efi.coco_secret, secret_area_size, MEMREMAP_WB);
+	if (secret_area == NULL) {
+		pr_err("Could not map secret area\n");
+		return -ENOMEM;
+	}
+
+	s->secret_area = secret_area;
+	return 0;
+}
+
+static void efi_secret_securityfs_teardown(void)
+{
+	struct efi_secret *s = efi_secret_get();
+	int i;
+
+	for (i = (EFI_SECRET_NUM_FILES - 1); i >= 0; i--) {
+		securityfs_remove(s->fs_files[i]);
+		s->fs_files[i] = NULL;
+	}
+
+	securityfs_remove(s->fs_dir);
+	s->fs_dir = NULL;
+
+	securityfs_remove(s->coco_dir);
+	s->coco_dir = NULL;
+
+	pr_debug("Removed efi_secret securityfs entries\n");
+}
+
+static int efi_secret_securityfs_setup(void)
+{
+	efi_guid_t tableheader_guid = EFI_SECRET_TABLE_HEADER_GUID;
+	struct efi_secret *s = efi_secret_get();
+	int ret = 0, i = 0, bytes_left;
+	unsigned char *ptr;
+	struct secret_header *h;
+	struct secret_entry *e;
+	struct dentry *dent;
+	char guid_str[EFI_VARIABLE_GUID_LEN + 1];
+
+	s->coco_dir = NULL;
+	s->fs_dir = NULL;
+	memset(s->fs_files, 0, sizeof(s->fs_files));
+
+	dent = securityfs_create_dir("coco", NULL);
+	if (IS_ERR(dent)) {
+		pr_err("Error creating coco securityfs directory entry err=%ld\n", PTR_ERR(dent));
+		return PTR_ERR(dent);
+	}
+	s->coco_dir = dent;
+
+	dent = securityfs_create_dir("efi_secret", s->coco_dir);
+	if (IS_ERR(dent)) {
+		pr_err("Error creating efi_secret securityfs directory entry err=%ld\n",
+		       PTR_ERR(dent));
+		return PTR_ERR(dent);
+	}
+	d_inode(dent)->i_op = &efi_secret_dir_inode_operations;
+	s->fs_dir = dent;
+
+	ptr = s->secret_area->area;
+	h = (struct secret_header *)ptr;
+	if (memcmp(&h->guid, &tableheader_guid, sizeof(h->guid))) {
+		pr_err("EFI secret area does not start with correct GUID\n");
+		ret = -EINVAL;
+		goto err_cleanup;
+	}
+	if (h->len < sizeof(*h)) {
+		pr_err("EFI secret area reported length is too small\n");
+		ret = -EINVAL;
+		goto err_cleanup;
+	}
+
+	bytes_left = h->len - sizeof(*h);
+	ptr += sizeof(*h);
+	while (bytes_left >= (int)sizeof(*e) && i < EFI_SECRET_NUM_FILES) {
+		e = (struct secret_entry *)ptr;
+		if (e->len < sizeof(*e) || e->len > (unsigned int)bytes_left) {
+			pr_err("EFI secret area is corrupted\n");
+			ret = -EINVAL;
+			goto err_cleanup;
+		}
+
+		/* Skip deleted entries (which will have NULL_GUID) */
+		if (efi_guidcmp(e->guid, NULL_GUID)) {
+			efi_guid_to_str(&e->guid, guid_str);
+
+			dent = securityfs_create_file(guid_str, 0440, s->fs_dir, (void *)e,
+						      &efi_secret_bin_file_fops);
+			if (IS_ERR(dent)) {
+				pr_err("Error creating efi_secret securityfs entry\n");
+				ret = PTR_ERR(dent);
+				goto err_cleanup;
+			}
+
+			s->fs_files[i++] = dent;
+		}
+		ptr += e->len;
+		bytes_left -= e->len;
+	}
+
+	pr_debug("Created %d entries in efi_secret securityfs\n", i);
+	return 0;
+
+err_cleanup:
+	efi_secret_securityfs_teardown();
+	return ret;
+}
+
+static void efi_secret_unmap_area(void)
+{
+	struct efi_secret *s = efi_secret_get();
+
+	if (s->secret_area) {
+		memunmap(s->secret_area);
+		s->secret_area = NULL;
+	}
+}
+
+static int __init efi_secret_init(void)
+{
+	int ret;
+
+	ret = efi_secret_map_area();
+	if (ret)
+		return ret;
+
+	ret = efi_secret_securityfs_setup();
+	if (ret)
+		goto err_unmap;
+
+	return ret;
+
+err_unmap:
+	efi_secret_unmap_area();
+	return ret;
+}
+
+static void __exit efi_secret_exit(void)
+{
+	efi_secret_securityfs_teardown();
+	efi_secret_unmap_area();
+}
+
+module_init(efi_secret_init);
+module_exit(efi_secret_exit);
+
+MODULE_DESCRIPTION("Confidential computing EFI secret area access");
+MODULE_AUTHOR("IBM");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Greg KH]

On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
> Virtualization) allows a guest owner to inject secrets into the VMs
> memory without the host/hypervisor being able to read them.
> 
> Firmware support for secret injection is available in OVMF, which
> reserves a memory area for secret injection and includes a pointer to it
> the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
> However, OVMF doesn't force the guest OS to keep this memory area
> reserved.
> 
> If EFI exposes such a table entry, efi/libstub will copy this area to a
> reserved memory for future use inside the kernel.
> 
> A pointer to the new copy is kept in the EFI table under
> LINUX_EFI_COCO_SECRET_AREA_GUID.
> 
> The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  drivers/firmware/efi/Kconfig            | 12 +++++
>  drivers/firmware/efi/libstub/Makefile   |  1 +
>  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
>  drivers/firmware/efi/libstub/efi-stub.c |  2 +
>  drivers/firmware/efi/libstub/efistub.h  |  6 +++
>  drivers/firmware/efi/libstub/x86-stub.c |  2 +
>  include/linux/efi.h                     |  6 +++
>  7 files changed, 97 insertions(+)
>  create mode 100644 drivers/firmware/efi/libstub/coco.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2c3dac5ecb36..68d1c5e6a7b5 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
>  
>  	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
>  	  information.
> +
> +config EFI_COCO_SECRET
> +	bool "Copy and reserve EFI Confidential Computing secret area"
> +	depends on EFI
> +	default n

default is always "n", no need to list this.

> +	help
> +	  Copy memory reserved by EFI for Confidential Computing (coco)
> +	  injected secrets, if EFI exposes such a table entry.

Why would you want to "copy" secret memory?

This sounds really odd here, it sounds like you are opening up a
security hole.  Are you sure this is the correct text that everyone on
the "COCO" group agrees with?

> +
> +	  If you say Y here, the EFI stub copy the EFI secret area (if
> +	  available) and reserve it for use inside the kernel.  This will
> +	  allow the virt/coo/efi_secret module to access the secrets.

What is "virt/coo/efi_secret"?

> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index d0537573501e..fdada3fd5d9b 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>  lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
>  				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
>  
> +lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
>  lib-$(CONFIG_ARM)		+= arm32-stub.o
>  lib-$(CONFIG_ARM64)		+= arm64-stub.o
>  lib-$(CONFIG_X86)		+= x86-stub.o
> diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
> new file mode 100644
> index 000000000000..bf546b6a3f72
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/coco.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Confidential computing (coco) secret area handling
> + *
> + * Copyright (C) 2021 IBM Corporation
> + * Author: Dov Murik <dovmurik@linux.ibm.com>
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/sizes.h>
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
> +	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
> +
> +/**
> + * struct efi_coco_secret_table - EFI config table that points to the
> + * confidential computing secret area. The guid
> + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
> + * @base:	Physical address of the EFI secret area
> + * @size:	Size (in bytes) of the EFI secret area
> + */
> +struct efi_coco_secret_table {
> +	u64 base;
> +	u64 size;

__le64?  Or is this really in host endian format?

> +} __attribute((packed));
> +
> +/*
> + * Create a copy of EFI's confidential computing secret area (if available) so
> + * that the secrets are accessible in the kernel after ExitBootServices.
> + */
> +void efi_copy_coco_secret_area(void)
> +{
> +	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
> +	efi_status_t status;
> +	struct efi_coco_secret_table *secret_table;
> +	struct linux_efi_coco_secret_area *secret_area;
> +
> +	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
> +	if (!secret_table)
> +		return;
> +
> +	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
> +		return;
> +
> +	/* Allocate space for the secret area and copy it */
> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> +			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
> +
> +	if (status != EFI_SUCCESS) {
> +		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
> +		return;
> +	}
> +
> +	secret_area->size = secret_table->size;
> +	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);

Why the double cast?

And you can treat this value as a "raw" pointer directly?  No need to
map it at all?  What could go wrong...

> +
> +	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
> +	if (status != EFI_SUCCESS)
> +		goto err_free;
> +
> +	return;
> +
> +err_free:
> +	efi_bs_call(free_pool, secret_area);

This memory is never freed when shutting down the system?

thanks,

greg k-h

Re: [PATCH v4 2/3] efi: Reserve confidential computing secret area

[Greg KH]

On Wed, Oct 20, 2021 at 06:14:07AM +0000, Dov Murik wrote:
> When efi-stub copies an EFI-provided confidential computing (coco)
> secret area, reserve that memory block for future use within the kernel.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  arch/x86/platform/efi/efi.c   |  3 +++
>  drivers/firmware/efi/Makefile |  1 +
>  drivers/firmware/efi/coco.c   | 41 +++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/efi.c    |  8 +++++++
>  include/linux/efi.h           | 10 +++++++++
>  5 files changed, 63 insertions(+)
>  create mode 100644 drivers/firmware/efi/coco.c
> 
> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
> index 147c30a81f15..1591d67e0bcd 100644
> --- a/arch/x86/platform/efi/efi.c
> +++ b/arch/x86/platform/efi/efi.c
> @@ -93,6 +93,9 @@ static const unsigned long * const efi_tables[] = {
>  #ifdef CONFIG_LOAD_UEFI_KEYS
>  	&efi.mokvar_table,
>  #endif
> +#ifdef CONFIG_EFI_COCO_SECRET
> +	&efi.coco_secret,
> +#endif
>  };
>  
>  u64 efi_setup;		/* efi setup_data physical address */
> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
> index c02ff25dd477..49c4a8c0bfc4 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
>  obj-$(CONFIG_EFI_RCI2_TABLE)		+= rci2-table.o
>  obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
>  obj-$(CONFIG_LOAD_UEFI_KEYS)		+= mokvar-table.o
> +obj-$(CONFIG_EFI_COCO_SECRET)		+= coco.o
>  
>  fake_map-y				+= fake_mem.o
>  fake_map-$(CONFIG_X86)			+= x86_fake_mem.o
> diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c
> new file mode 100644
> index 000000000000..42f477d6188c
> --- /dev/null
> +++ b/drivers/firmware/efi/coco.c
> @@ -0,0 +1,41 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Confidential computing (coco) secret area handling
> + *
> + * Copyright (C) 2021 IBM Corporation
> + * Author: Dov Murik <dovmurik@linux.ibm.com>
> + */
> +
> +#define pr_fmt(fmt) "efi: " fmt
> +
> +#include <linux/efi.h>
> +#include <linux/init.h>
> +#include <linux/memblock.h>
> +#include <asm/early_ioremap.h>
> +
> +/*
> + * Reserve the confidential computing secret area memory
> + */
> +int __init efi_coco_secret_area_reserve(void)
> +{
> +	struct linux_efi_coco_secret_area *secret_area;
> +	unsigned long secret_area_size;
> +
> +	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR)
> +		return 0;
> +
> +	secret_area = early_memremap(efi.coco_secret, sizeof(*secret_area));
> +	if (!secret_area) {
> +		pr_err("Failed to map confidential computing secret area\n");
> +		efi.coco_secret = EFI_INVALID_TABLE_ADDR;
> +		return -ENOMEM;
> +	}
> +
> +	secret_area_size = sizeof(*secret_area) + secret_area->size;
> +	memblock_reserve(efi.coco_secret, secret_area_size);
> +
> +	pr_info("Reserved memory of EFI-provided confidential computing secret area");

When kernel code works properly, it is quiet.  Why do you need to print
this out at every boot?

> +
> +	early_memunmap(secret_area, sizeof(*secret_area));
> +	return 0;
> +}

And again, when is this memory freed when shutting down?

thanks,

greg k-h

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Ard Biesheuvel]

On Wed, 20 Oct 2021 at 08:44, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> > Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
> > Virtualization) allows a guest owner to inject secrets into the VMs
> > memory without the host/hypervisor being able to read them.
> >
> > Firmware support for secret injection is available in OVMF, which
> > reserves a memory area for secret injection and includes a pointer to it
> > the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
> > However, OVMF doesn't force the guest OS to keep this memory area
> > reserved.
> >
> > If EFI exposes such a table entry, efi/libstub will copy this area to a
> > reserved memory for future use inside the kernel.
> >
> > A pointer to the new copy is kept in the EFI table under
> > LINUX_EFI_COCO_SECRET_AREA_GUID.
> >
> > The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
> >
> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> > ---
> >  drivers/firmware/efi/Kconfig            | 12 +++++
> >  drivers/firmware/efi/libstub/Makefile   |  1 +
> >  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
> >  drivers/firmware/efi/libstub/efi-stub.c |  2 +
> >  drivers/firmware/efi/libstub/efistub.h  |  6 +++
> >  drivers/firmware/efi/libstub/x86-stub.c |  2 +
> >  include/linux/efi.h                     |  6 +++
> >  7 files changed, 97 insertions(+)
> >  create mode 100644 drivers/firmware/efi/libstub/coco.c
> >
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 2c3dac5ecb36..68d1c5e6a7b5 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
> >
> >         See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
> >         information.
> > +
> > +config EFI_COCO_SECRET
> > +     bool "Copy and reserve EFI Confidential Computing secret area"
> > +     depends on EFI
> > +     default n
>
> default is always "n", no need to list this.
>
> > +     help
> > +       Copy memory reserved by EFI for Confidential Computing (coco)
> > +       injected secrets, if EFI exposes such a table entry.
>
> Why would you want to "copy" secret memory?
>
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone on
> the "COCO" group agrees with?
>
> > +
> > +       If you say Y here, the EFI stub copy the EFI secret area (if
> > +       available) and reserve it for use inside the kernel.  This will
> > +       allow the virt/coo/efi_secret module to access the secrets.
>
> What is "virt/coo/efi_secret"?
>
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index d0537573501e..fdada3fd5d9b 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
> >  lib-$(CONFIG_EFI_GENERIC_STUB)       += efi-stub.o fdt.o string.o \
> >                                  $(patsubst %.c,lib-%.o,$(efi-deps-y))
> >
> > +lib-$(CONFIG_EFI_COCO_SECRET)        += coco.o
> >  lib-$(CONFIG_ARM)            += arm32-stub.o
> >  lib-$(CONFIG_ARM64)          += arm64-stub.o
> >  lib-$(CONFIG_X86)            += x86-stub.o
> > diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
> > new file mode 100644
> > index 000000000000..bf546b6a3f72
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/coco.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Confidential computing (coco) secret area handling
> > + *
> > + * Copyright (C) 2021 IBM Corporation
> > + * Author: Dov Murik <dovmurik@linux.ibm.com>
> > + */
> > +
> > +#include <linux/efi.h>
> > +#include <linux/sizes.h>
> > +#include <asm/efi.h>
> > +
> > +#include "efistub.h"
> > +
> > +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
> > +     EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
> > +
> > +/**
> > + * struct efi_coco_secret_table - EFI config table that points to the
> > + * confidential computing secret area. The guid
> > + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
> > + * @base:    Physical address of the EFI secret area
> > + * @size:    Size (in bytes) of the EFI secret area
> > + */
> > +struct efi_coco_secret_table {
> > +     u64 base;
> > +     u64 size;
>
> __le64?  Or is this really in host endian format?
>

EFI hosts are always LE so either is fine.

> > +} __attribute((packed));
> > +
> > +/*
> > + * Create a copy of EFI's confidential computing secret area (if available) so
> > + * that the secrets are accessible in the kernel after ExitBootServices.
> > + */
> > +void efi_copy_coco_secret_area(void)
> > +{
> > +     efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
> > +     efi_status_t status;
> > +     struct efi_coco_secret_table *secret_table;
> > +     struct linux_efi_coco_secret_area *secret_area;
> > +
> > +     secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
> > +     if (!secret_table)
> > +             return;
> > +
> > +     if (secret_table->size == 0 || secret_table->size >= SZ_4G)
> > +             return;
> > +
> > +     /* Allocate space for the secret area and copy it */
> > +     status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> > +                          sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
> > +
> > +     if (status != EFI_SUCCESS) {
> > +             efi_err("Unable to allocate memory for confidential computing secret area copy\n");
> > +             return;
> > +     }
> > +
> > +     secret_area->size = secret_table->size;
> > +     memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
>
> Why the double cast?
>

This is generally needed for compatibility with 32-bit hosts, where
casting a u64 to void* causes warnings, even though we know in that
case that only the lower 32 bits will contain anything (even with PAE
etc, as 32-bit UEFI only uses 32-bit addressable memory)

In this particular case, it probably makes little sense, as COCO is
not going to run on 32-bit hosts anyway (famous last words)

> And you can treat this value as a "raw" pointer directly?  No need to
> map it at all?  What could go wrong...
>

Yes. EFI boot services (as well as this code) are guaranteed to run
under a 1:1 mapping of system memory.

> > +
> > +     status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
> > +     if (status != EFI_SUCCESS)
> > +             goto err_free;
> > +
> > +     return;
> > +
> > +err_free:
> > +     efi_bs_call(free_pool, secret_area);
>
> This memory is never freed when shutting down the system?
>

All boot services memory is implicitly freed when the system calls
ExitBootServices() so this is fine.

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Dov Murik]

On 20/10/2021 9:39, Greg KH wrote:
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
>> Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
>> Virtualization) allows a guest owner to inject secrets into the VMs
>> memory without the host/hypervisor being able to read them.
>>
>> Firmware support for secret injection is available in OVMF, which
>> reserves a memory area for secret injection and includes a pointer to it
>> the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
>> However, OVMF doesn't force the guest OS to keep this memory area
>> reserved.
>>
>> If EFI exposes such a table entry, efi/libstub will copy this area to a
>> reserved memory for future use inside the kernel.
>>
>> A pointer to the new copy is kept in the EFI table under
>> LINUX_EFI_COCO_SECRET_AREA_GUID.
>>
>> The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  drivers/firmware/efi/Kconfig            | 12 +++++
>>  drivers/firmware/efi/libstub/Makefile   |  1 +
>>  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
>>  drivers/firmware/efi/libstub/efi-stub.c |  2 +
>>  drivers/firmware/efi/libstub/efistub.h  |  6 +++
>>  drivers/firmware/efi/libstub/x86-stub.c |  2 +
>>  include/linux/efi.h                     |  6 +++
>>  7 files changed, 97 insertions(+)
>>  create mode 100644 drivers/firmware/efi/libstub/coco.c
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index 2c3dac5ecb36..68d1c5e6a7b5 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
>>  
>>  	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
>>  	  information.
>> +
>> +config EFI_COCO_SECRET
>> +	bool "Copy and reserve EFI Confidential Computing secret area"
>> +	depends on EFI
>> +	default n
> 
> default is always "n", no need to list this.
> 

OK, I'll remove.


>> +	help
>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
>> +	  injected secrets, if EFI exposes such a table entry.
> 
> Why would you want to "copy" secret memory?
> 
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone on
> the "COCO" group agrees with?

I understand the security concern: we don't want several copies of the
secrets all around the guest's memory.

I'll try to see if I can just reserve the memory (instruct EFI to leave
it intact) at its current address instead of creating a copy.  I'm open
to suggestions/pointers.


linux-coco list is CC'd on this series; feedback is welcome.


> 
>> +
>> +	  If you say Y here, the EFI stub copy the EFI secret area (if
>> +	  available) and reserve it for use inside the kernel.  This will
>> +	  allow the virt/coo/efi_secret module to access the secrets.
> 
> What is "virt/coo/efi_secret"?
> 

Typo: that should be virt/coco/efi_secret (the module introduced in
patch 3).

-Dov

>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> index d0537573501e..fdada3fd5d9b 100644
>> --- a/drivers/firmware/efi/libstub/Makefile
>> +++ b/drivers/firmware/efi/libstub/Makefile
>> @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>>  lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
>>  				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
>>  
>> +lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
>>  lib-$(CONFIG_ARM)		+= arm32-stub.o
>>  lib-$(CONFIG_ARM64)		+= arm64-stub.o
>>  lib-$(CONFIG_X86)		+= x86-stub.o
>> diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
>> new file mode 100644
>> index 000000000000..bf546b6a3f72
>> --- /dev/null
>> +++ b/drivers/firmware/efi/libstub/coco.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Confidential computing (coco) secret area handling
>> + *
>> + * Copyright (C) 2021 IBM Corporation
>> + * Author: Dov Murik <dovmurik@linux.ibm.com>
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <linux/sizes.h>
>> +#include <asm/efi.h>
>> +
>> +#include "efistub.h"
>> +
>> +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
>> +	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
>> +
>> +/**
>> + * struct efi_coco_secret_table - EFI config table that points to the
>> + * confidential computing secret area. The guid
>> + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
>> + * @base:	Physical address of the EFI secret area
>> + * @size:	Size (in bytes) of the EFI secret area
>> + */
>> +struct efi_coco_secret_table {
>> +	u64 base;
>> +	u64 size;
> 
> __le64?  Or is this really in host endian format?
> 
>> +} __attribute((packed));
>> +
>> +/*
>> + * Create a copy of EFI's confidential computing secret area (if available) so
>> + * that the secrets are accessible in the kernel after ExitBootServices.
>> + */
>> +void efi_copy_coco_secret_area(void)
>> +{
>> +	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
>> +	efi_status_t status;
>> +	struct efi_coco_secret_table *secret_table;
>> +	struct linux_efi_coco_secret_area *secret_area;
>> +
>> +	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
>> +	if (!secret_table)
>> +		return;
>> +
>> +	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
>> +		return;
>> +
>> +	/* Allocate space for the secret area and copy it */
>> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
>> +			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
>> +
>> +	if (status != EFI_SUCCESS) {
>> +		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
>> +		return;
>> +	}
>> +
>> +	secret_area->size = secret_table->size;
>> +	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
> 
> Why the double cast?
> 
> And you can treat this value as a "raw" pointer directly?  No need to
> map it at all?  What could go wrong...
> 
>> +
>> +	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
>> +	if (status != EFI_SUCCESS)
>> +		goto err_free;
>> +
>> +	return;
>> +
>> +err_free:
>> +	efi_bs_call(free_pool, secret_area);
> 
> This memory is never freed when shutting down the system?
> 
> thanks,
> 
> greg k-h
> 

Re: [PATCH v4 2/3] efi: Reserve confidential computing secret area

[Dov Murik]

On 20/10/2021 9:40, Greg KH wrote:
> On Wed, Oct 20, 2021 at 06:14:07AM +0000, Dov Murik wrote:
>> When efi-stub copies an EFI-provided confidential computing (coco)
>> secret area, reserve that memory block for future use within the kernel.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  arch/x86/platform/efi/efi.c   |  3 +++
>>  drivers/firmware/efi/Makefile |  1 +
>>  drivers/firmware/efi/coco.c   | 41 +++++++++++++++++++++++++++++++++++
>>  drivers/firmware/efi/efi.c    |  8 +++++++
>>  include/linux/efi.h           | 10 +++++++++
>>  5 files changed, 63 insertions(+)
>>  create mode 100644 drivers/firmware/efi/coco.c
>>
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index 147c30a81f15..1591d67e0bcd 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -93,6 +93,9 @@ static const unsigned long * const efi_tables[] = {
>>  #ifdef CONFIG_LOAD_UEFI_KEYS
>>  	&efi.mokvar_table,
>>  #endif
>> +#ifdef CONFIG_EFI_COCO_SECRET
>> +	&efi.coco_secret,
>> +#endif
>>  };
>>  
>>  u64 efi_setup;		/* efi setup_data physical address */
>> diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
>> index c02ff25dd477..49c4a8c0bfc4 100644
>> --- a/drivers/firmware/efi/Makefile
>> +++ b/drivers/firmware/efi/Makefile
>> @@ -32,6 +32,7 @@ obj-$(CONFIG_APPLE_PROPERTIES)		+= apple-properties.o
>>  obj-$(CONFIG_EFI_RCI2_TABLE)		+= rci2-table.o
>>  obj-$(CONFIG_EFI_EMBEDDED_FIRMWARE)	+= embedded-firmware.o
>>  obj-$(CONFIG_LOAD_UEFI_KEYS)		+= mokvar-table.o
>> +obj-$(CONFIG_EFI_COCO_SECRET)		+= coco.o
>>  
>>  fake_map-y				+= fake_mem.o
>>  fake_map-$(CONFIG_X86)			+= x86_fake_mem.o
>> diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c
>> new file mode 100644
>> index 000000000000..42f477d6188c
>> --- /dev/null
>> +++ b/drivers/firmware/efi/coco.c
>> @@ -0,0 +1,41 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Confidential computing (coco) secret area handling
>> + *
>> + * Copyright (C) 2021 IBM Corporation
>> + * Author: Dov Murik <dovmurik@linux.ibm.com>
>> + */
>> +
>> +#define pr_fmt(fmt) "efi: " fmt
>> +
>> +#include <linux/efi.h>
>> +#include <linux/init.h>
>> +#include <linux/memblock.h>
>> +#include <asm/early_ioremap.h>
>> +
>> +/*
>> + * Reserve the confidential computing secret area memory
>> + */
>> +int __init efi_coco_secret_area_reserve(void)
>> +{
>> +	struct linux_efi_coco_secret_area *secret_area;
>> +	unsigned long secret_area_size;
>> +
>> +	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR)
>> +		return 0;
>> +
>> +	secret_area = early_memremap(efi.coco_secret, sizeof(*secret_area));
>> +	if (!secret_area) {
>> +		pr_err("Failed to map confidential computing secret area\n");
>> +		efi.coco_secret = EFI_INVALID_TABLE_ADDR;
>> +		return -ENOMEM;
>> +	}
>> +
>> +	secret_area_size = sizeof(*secret_area) + secret_area->size;
>> +	memblock_reserve(efi.coco_secret, secret_area_size);
>> +
>> +	pr_info("Reserved memory of EFI-provided confidential computing secret area");
> 
> When kernel code works properly, it is quiet.  Why do you need to print
> this out at every boot?
> 

My kernel is not so quiet at the info loglevel; specifically from efi I 
see these prints (third log line added by this patch):

[    0.000000] efi: EFI v2.70 by EDK II
[    0.000000] efi: SMBIOS=0x7f541000 ACPI=0x7f77e000 ACPI 2.0=0x7f77e014 MEMATTR=0x7ea0c018 CocoSecret=0x7ea0b018
[    0.000000] efi: Reserved memory of EFI-provided confidential computing secret area

This print is useful to understand that both OVMF (EFI) and kernel support
the confidential computing secret area.



>> +
>> +	early_memunmap(secret_area, sizeof(*secret_area));
>> +	return 0;
>> +}
> 
> And again, when is this memory freed when shutting down?
> 

It is currently not freed.  I tried to look for such memory freeing of
other EFI-provided memory areas (such as efi.tpm_final_log) and couldn't
find them.  Can you please share pointers/examples of how to do that?

Thanks,
-Dov

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[James Bottomley]

On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
[...]
> > +	help
> > +	  Copy memory reserved by EFI for Confidential Computing (coco)
> > +	  injected secrets, if EFI exposes such a table entry.
> 
> Why would you want to "copy" secret memory?
> 
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone
> on the "COCO" group agrees with?

The way this works is that EFI covers the secret area with a boot time
handoff block, which means it gets destroyed as soon as
ExitBootServices is called as a security measure ... if you do nothing
the secret is shredded.  This means you need to make a copy of it
before that happens if there are secrets that need to live beyond the
EFI boot stub.

James

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Greg KH]

On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> > On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> [...]
> > > +	help
> > > +	  Copy memory reserved by EFI for Confidential Computing (coco)
> > > +	  injected secrets, if EFI exposes such a table entry.
> > 
> > Why would you want to "copy" secret memory?
> > 
> > This sounds really odd here, it sounds like you are opening up a
> > security hole.  Are you sure this is the correct text that everyone
> > on the "COCO" group agrees with?
> 
> The way this works is that EFI covers the secret area with a boot time
> handoff block, which means it gets destroyed as soon as
> ExitBootServices is called as a security measure ... if you do nothing
> the secret is shredded.  This means you need to make a copy of it
> before that happens if there are secrets that need to live beyond the
> EFI boot stub.

Ok, but "copy secrets" does sound really odd, so you all need a much
better description here, and hopefully somewhere else in Documentation/
to describe exactly what this new API is and is to be used for.

Otherwise I read this as "hey a backdoor to read the secrets I wasn't
supposed to be able to see!"

thanks,

greg k-h

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Dov Murik]

On 20/10/2021 15:11, Greg KH wrote:
> On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
>> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
>>> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
>> [...]
>>>> +	help
>>>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
>>>> +	  injected secrets, if EFI exposes such a table entry.
>>>
>>> Why would you want to "copy" secret memory?
>>>
>>> This sounds really odd here, it sounds like you are opening up a
>>> security hole.  Are you sure this is the correct text that everyone
>>> on the "COCO" group agrees with?
>>
>> The way this works is that EFI covers the secret area with a boot time
>> handoff block, which means it gets destroyed as soon as
>> ExitBootServices is called as a security measure ... if you do nothing
>> the secret is shredded.  This means you need to make a copy of it
>> before that happens if there are secrets that need to live beyond the
>> EFI boot stub.
> 
> Ok, but "copy secrets" does sound really odd, so you all need a much
> better description here, and hopefully somewhere else in Documentation/
> to describe exactly what this new API is and is to be used for.
> 


So something like:


config EFI_COCO_SECRET
	bool "Keep the EFI Confidential Computing secret area"
	depends on EFI
	help
	  Confidential Computing platforms (such as AMD SEV) allow for
	  secrets injection during guest VM launch.  The secrets are
	  placed in a designated EFI memory area.  EFI destorys
	  the confidential computing secret area when ExitBootServices
	  is called.

	  In order to use the secrets in the kernel, the secret area
	  must be copied to kernel-reserved memory (before it is erased).

	  If you say Y here, the EFI stub will copy the EFI secret area (if
	  available) and reserve it for use inside the kernel.  This will
	  allow the virt/coco/efi_secret module to access the secrets.



and some new file like Documentation/security/coco/efi_secret.rst which
describes this whole protocol (from secret injection at VM launch
into an EFI page, through efistub and efi in linux, to the efi_secret
module which exposes the secrets).


Is that what you're looking for?



> Otherwise I read this as "hey a backdoor to read the secrets I wasn't
> supposed to be able to see!"
> 

Note that both EFI and kernel (and userspace, for that matter) are inside
the trusted zone in terms of AMD SEV (host/hypervisor => untrusted,
guest VM => trusted).  So it's OK for the guest kernel to see these secrets.


-Dov

> thanks,
> 
> greg k-h
> 

Re: [PATCH v4 1/3] efi/libstub: Copy confidential computing secret area

[Greg KH]

On Wed, Oct 20, 2021 at 03:52:49PM +0300, Dov Murik wrote:
> 
> 
> On 20/10/2021 15:11, Greg KH wrote:
> > On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
> >> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> >>> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> >> [...]
> >>>> +	help
> >>>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
> >>>> +	  injected secrets, if EFI exposes such a table entry.
> >>>
> >>> Why would you want to "copy" secret memory?
> >>>
> >>> This sounds really odd here, it sounds like you are opening up a
> >>> security hole.  Are you sure this is the correct text that everyone
> >>> on the "COCO" group agrees with?
> >>
> >> The way this works is that EFI covers the secret area with a boot time
> >> handoff block, which means it gets destroyed as soon as
> >> ExitBootServices is called as a security measure ... if you do nothing
> >> the secret is shredded.  This means you need to make a copy of it
> >> before that happens if there are secrets that need to live beyond the
> >> EFI boot stub.
> > 
> > Ok, but "copy secrets" does sound really odd, so you all need a much
> > better description here, and hopefully somewhere else in Documentation/
> > to describe exactly what this new API is and is to be used for.
> > 
> 
> 
> So something like:
> 
> 
> config EFI_COCO_SECRET
> 	bool "Keep the EFI Confidential Computing secret area"
> 	depends on EFI
> 	help
> 	  Confidential Computing platforms (such as AMD SEV) allow for
> 	  secrets injection during guest VM launch.  The secrets are
> 	  placed in a designated EFI memory area.  EFI destorys
> 	  the confidential computing secret area when ExitBootServices
> 	  is called.

That last sentence does not make much sense to me, sorry.

> 	  In order to use the secrets in the kernel, the secret area
> 	  must be copied to kernel-reserved memory (before it is erased).
> 
> 	  If you say Y here, the EFI stub will copy the EFI secret area (if
> 	  available) and reserve it for use inside the kernel.  This will
> 	  allow the virt/coco/efi_secret module to access the secrets.

Really this is about getting that data out to userspace, right?  Should
you mention that here?

> and some new file like Documentation/security/coco/efi_secret.rst which
> describes this whole protocol (from secret injection at VM launch
> into an EFI page, through efistub and efi in linux, to the efi_secret
> module which exposes the secrets).

Yes, that would be good to have documented.

thanks,

greg k-h