[PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[PATCH v7 0/5] 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 keeps the address of the EFI-provided memory for
injected secrets, and exposes the secrets to userspace via securityfs
using a new efi_secret kernel module.  The module is autoloaded (by the
EFI driver) if the secret area is populated.

The first patch in EFI keeps the address of the secret area as passed in
the EFI configuration table.  The second patch is a quirk fix for older
firmwares didn't mark the secrets page as EFI_RESERVED_TYPE.  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.  The fourth patch auto-loads the
efi_secret module during startup if the injected secrets area is
populated.  The last patch documents the data flow of confidential
computing secret injection.

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:

# 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


---

v7 changes:
 - Improve description of efi_secret module in Kconfig.
 - Fix sparse warnings on pointer address space mismatch
   (Reported-by: kernel test robot <lkp@intel.com>)

v6: https://lore.kernel.org/linux-coco/20211129114251.3741721-1-dovmurik@linux.ibm.com/
v6 changes:
 - Autoload the efi_secret module if the secret area is populated
   (thanks Greg KH).
 - efi_secret: Depend on X86_64 because we use ioremap_encrypted() which
   is only defined for this arch.
 - efi_secret.c: Remove unneeded tableheader_guid local variable.
 - Documentation fixes.

v5: https://lore.kernel.org/linux-coco/20211118113359.642571-1-dovmurik@linux.ibm.com/
v5 changes:
 - Simplify EFI code: instead of copying the secret area, the firmware
   marks the secret area as EFI_RESERVED_TYPE, and then the uefi_init()
   code just keeps the pointer as it appears in the EFI configuration
   table.  The use of reserved pages is similar to the AMD SEV-SNP
   patches for handling SNP-Secrets and SNP-CPUID pages.
 - In order to handle OVMF releases out there which mark the
   confidential computing secrets page as EFI_BOOT_SERVICES_DATA, add
   efi/libstub code that detects this and fixes the E820 map to reserve
   this page.
 - In the efi_secret module code, map the secrets page using
   ioremap_encrypted (again, similar to the AMD SEV-SNP guest patches
   for accessing SNP-Secrets and SNP-CPUID pages).
 - Add documentation in Documentation/security/coco/efi_secret.

v4: https://lore.kernel.org/linux-coco/20211020061408.3447533-1-dovmurik@linux.ibm.com/
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 (5):
  efi: Save location of EFI confidential computing area
  efi/libstub: Reserve confidential computing secret area
  virt: Add efi_secret module to expose confidential computing secrets
  efi: Load efi_secret module if EFI secret area is populated
  docs: security: Add coco/efi_secret documentation

 .../ABI/testing/securityfs-coco-efi_secret    |  51 +++
 Documentation/security/coco/efi_secret.rst    | 102 ++++++
 Documentation/security/coco/index.rst         |   9 +
 Documentation/security/index.rst              |   1 +
 arch/x86/platform/efi/efi.c                   |   3 +
 drivers/firmware/efi/Kconfig                  |  16 +
 drivers/firmware/efi/Makefile                 |   1 +
 drivers/firmware/efi/coco.c                   |  58 +++
 drivers/firmware/efi/efi.c                    |   6 +
 drivers/firmware/efi/libstub/x86-stub.c       |  28 ++
 drivers/virt/Kconfig                          |   3 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/coco/efi_secret/Kconfig          |  19 +
 drivers/virt/coco/efi_secret/Makefile         |   2 +
 drivers/virt/coco/efi_secret/efi_secret.c     | 337 ++++++++++++++++++
 include/linux/efi.h                           |  10 +
 16 files changed, 647 insertions(+)
 create mode 100644 Documentation/ABI/testing/securityfs-coco-efi_secret
 create mode 100644 Documentation/security/coco/efi_secret.rst
 create mode 100644 Documentation/security/coco/index.rst
 create mode 100644 drivers/firmware/efi/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: 26291c54e111ff6ba87a164d85d4a4e134b7315c
-- 
2.25.1

[PATCH v7 1/5] efi: Save location of EFI confidential computing 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.

If EFI exposes such a table entry, uefi_init() will keep a pointer to
the EFI config table entry in efi.coco_secret, so it can be used later
by the kernel (specifically drivers/virt/coco/efi_secret).  It will also
appear in the kernel log as "CocoSecret=ADDRESS"; for example:

    [    0.000000] efi: EFI v2.70 by EDK II
    [    0.000000] efi: CocoSecret=0x7f22e680 SMBIOS=0x7f541000 ACPI=0x7f77e000 ACPI 2.0=0x7f77e014 MEMATTR=0x7ea0c018

The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 arch/x86/platform/efi/efi.c  |  3 +++
 drivers/firmware/efi/Kconfig | 16 ++++++++++++++++
 drivers/firmware/efi/efi.c   |  6 ++++++
 include/linux/efi.h          | 10 ++++++++++
 4 files changed, 35 insertions(+)

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/Kconfig b/drivers/firmware/efi/Kconfig
index 2c3dac5ecb36..6fa251b3709f 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -284,3 +284,19 @@ config EFI_CUSTOM_SSDT_OVERLAYS
 
 	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
 	  information.
+
+config EFI_COCO_SECRET
+	bool "EFI Confidential Computing Secret Area Support"
+	depends on EFI
+	help
+	  Confidential Computing platforms (such as AMD SEV) allow the
+	  Guest Owner to securely inject secrets during guest VM launch.
+	  The secrets are placed in a designated EFI reserved memory area.
+
+	  In order to use the secrets in the kernel, the location of the secret
+	  area (as published in the EFI config table) must be kept.
+
+	  If you say Y here, the address of the EFI secret area will be kept
+	  for usage inside the kernel.  This will allow the
+	  virt/coco/efi_secret module to access the secrets, which in turn
+	  allows userspace programs to access the injected secrets.
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 7de3f5b6e8d0..378d044b2463 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);
 
@@ -528,6 +531,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
 	{},
 };
diff --git a/include/linux/efi.h b/include/linux/efi.h
index ccd4d3f91c98..771d4cd06b56 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -405,6 +405,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(0xadf956ad, 0xe98c, 0x484c,  0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
 
 /* OEM GUIDs */
 #define DELLEMC_EFI_RCI2_TABLE_GUID		EFI_GUID(0x2d9f28a2, 0xa886, 0x456a,  0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
@@ -596,6 +597,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;
@@ -1335,4 +1337,12 @@ extern void efifb_setup_from_dmi(struct screen_info *si, const char *opt);
 static inline void efifb_setup_from_dmi(struct screen_info *si, const char *opt) { }
 #endif
 
+struct linux_efi_coco_secret_area {
+	u64	base_pa;
+	u64	size;
+};
+
+/* Header of a populated EFI secret area */
+#define EFI_SECRET_TABLE_HEADER_GUID	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
+
 #endif /* _LINUX_EFI_H */
-- 
2.25.1

[PATCH v7 2/5] efi/libstub: Reserve confidential computing secret area

[Dov Murik]

Some older firmware declare the confidential computing secret area as
EFI_BOOT_SERVICES_DATA region.  Fix this up by treating this memory
region as EFI_RESERVED_TYPE, as it should be.

If that memory region is already EFI_RESERVED_TYPE then this has no
effect on the E820 map.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/libstub/x86-stub.c | 28 +++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 01ddd4502e28..4f1218cb87ca 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -24,6 +24,7 @@
 const efi_system_table_t *efi_system_table;
 extern u32 image_offset;
 static efi_loaded_image_t *image = NULL;
+static u64 efi_coco_secret_phys_addr = U64_MAX;
 
 static efi_status_t
 preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
@@ -443,6 +444,21 @@ static void add_e820ext(struct boot_params *params,
 		params->hdr.setup_data = (unsigned long)e820ext;
 }
 
+#ifdef CONFIG_EFI_COCO_SECRET
+static void efi_set_coco_secret_phys_addr(void)
+{
+	struct linux_efi_coco_secret_area *secret_area =
+		get_efi_config_table(LINUX_EFI_COCO_SECRET_AREA_GUID);
+
+	if (!secret_area || secret_area->size == 0 || secret_area->size >= SZ_4G)
+		return;
+
+	efi_coco_secret_phys_addr = secret_area->base_pa;
+}
+#else
+static void efi_set_coco_secret_phys_addr(void) {}
+#endif
+
 static efi_status_t
 setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_size)
 {
@@ -494,6 +510,16 @@ setup_e820(struct boot_params *params, struct setup_data *e820ext, u32 e820ext_s
 				e820_type = E820_TYPE_SOFT_RESERVED;
 			else
 				e820_type = E820_TYPE_RAM;
+#ifdef CONFIG_EFI_COCO_SECRET
+			if (d->phys_addr == efi_coco_secret_phys_addr)
+				/*
+				 * Fix a quirk in firmwares which don't mark
+				 * the EFI confidential computing area as
+				 * EFI_RESERVED_TYPE, but instead as
+				 * EFI_BOOT_SERVICES_DATA.
+				 */
+				e820_type = E820_TYPE_RESERVED;
+#endif
 			break;
 
 		case EFI_ACPI_MEMORY_NVS:
@@ -787,6 +813,8 @@ unsigned long efi_main(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_set_coco_secret_phys_addr();
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
-- 
2.25.1

[PATCH v7 3/5] 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    |  51 +++
 drivers/virt/Kconfig                          |   3 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/coco/efi_secret/Kconfig          |  16 +
 drivers/virt/coco/efi_secret/Makefile         |   2 +
 drivers/virt/coco/efi_secret/efi_secret.c     | 337 ++++++++++++++++++
 6 files changed, 410 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..770abac2c4bb
--- /dev/null
+++ b/Documentation/ABI/testing/securityfs-coco-efi_secret
@@ -0,0 +1,51 @@
+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.  This module is loaded automatically by the EFI driver
+		if the EFI secret area is populated.
+
+		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..ed181797ed86
--- /dev/null
+++ b/drivers/virt/coco/efi_secret/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config EFI_SECRET
+	tristate "EFI secret area securityfs support"
+	depends on EFI && X86_64
+	select EFI_COCO_SECRET
+	select SECURITYFS
+	help
+	  This is a driver for accessing the EFI secret area via securityfs.
+	  The EFI secret area is a memory area designated by the firmware for
+	  confidential computing secret injection (for example for AMD SEV
+	  guests).  The driver exposes the secrets as files in
+	  <securityfs>/coco/efi_secret.  Files can be read and deleted
+	  (deleting a file wipes the secret from memory).
+
+	  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..78d162e5a87a
--- /dev/null
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -0,0 +1,337 @@
+// 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 <linux/cacheflush.h>
+
+#define EFI_SECRET_NUM_FILES 64
+
+struct efi_secret {
+	struct dentry *coco_dir;
+	struct dentry *fs_dir;
+	struct dentry *fs_files[EFI_SECRET_NUM_FILES];
+	void __iomem *secret_data;
+	u64 secret_data_len;
+};
+
+/*
+ * 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)
+{
+	int ret;
+	struct efi_secret *s = efi_secret_get();
+	struct linux_efi_coco_secret_area *secret_area;
+
+	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 EFI config entry\n");
+		return -ENOMEM;
+	}
+	if (!secret_area->base_pa || secret_area->size < sizeof(struct secret_header)) {
+		pr_err("Invalid secret area memory location (base_pa=0x%llx size=0x%llx)\n",
+		       secret_area->base_pa, secret_area->size);
+		ret = -EINVAL;
+		goto unmap;
+	}
+
+	s->secret_data = ioremap_encrypted(secret_area->base_pa, secret_area->size);
+	if (s->secret_data == NULL) {
+		pr_err("Could not map secret area\n");
+		ret = -ENOMEM;
+		goto unmap;
+	}
+
+	s->secret_data_len = secret_area->size;
+	ret = 0;
+
+unmap:
+	memunmap(secret_area);
+	return ret;
+}
+
+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)
+{
+	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 = (void __force *)s->secret_data;
+	h = (struct secret_header *)ptr;
+	if (efi_guidcmp(h->guid, EFI_SECRET_TABLE_HEADER_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;
+	}
+	if (h->len > s->secret_data_len) {
+		pr_err("EFI secret area reported length is too big\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_data) {
+		iounmap(s->secret_data);
+		s->secret_data = NULL;
+		s->secret_data_len = 0;
+	}
+}
+
+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

[PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Dov Murik]

If the efi_secret module is built, register a late_initcall in the EFI
driver which checks whether the EFI secret area is available and
populated, and then requests to load the efi_secret module.

This will cause the <securityfs>/coco/efi_secret directory to appear in
guests into which secrets were injected; in other cases, the module is
not loaded.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/Makefile        |  1 +
 drivers/firmware/efi/coco.c          | 58 ++++++++++++++++++++++++++++
 drivers/virt/coco/efi_secret/Kconfig |  3 ++
 3 files changed, 62 insertions(+)
 create mode 100644 drivers/firmware/efi/coco.c

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..f8efd240ab05
--- /dev/null
+++ b/drivers/firmware/efi/coco.c
@@ -0,0 +1,58 @@
+// 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/io.h>
+#include <linux/kmod.h>
+
+#ifdef CONFIG_EFI_SECRET_MODULE
+
+/*
+ * Load the efi_secret module if the EFI secret area is populated
+ */
+static int __init load_efi_secret_module(void)
+{
+	struct linux_efi_coco_secret_area *area;
+	efi_guid_t *header_guid;
+	int ret = 0;
+
+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB);
+	if (!area) {
+		pr_err("Failed to map confidential computing secret area descriptor\n");
+		return -ENOMEM;
+	}
+	if (!area->base_pa || area->size < sizeof(*header_guid))
+		goto unmap_desc;
+
+	header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid));
+	if (!header_guid) {
+		pr_err("Failed to map secret area\n");
+		ret = -ENOMEM;
+		goto unmap_desc;
+	}
+	if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID))
+		goto unmap_encrypted;
+
+	ret = request_module("efi_secret");
+
+unmap_encrypted:
+	iounmap((void __iomem *)header_guid);
+
+unmap_desc:
+	memunmap(area);
+	return ret;
+}
+late_initcall(load_efi_secret_module);
+
+#endif
diff --git a/drivers/virt/coco/efi_secret/Kconfig b/drivers/virt/coco/efi_secret/Kconfig
index ed181797ed86..068fdd1fccd7 100644
--- a/drivers/virt/coco/efi_secret/Kconfig
+++ b/drivers/virt/coco/efi_secret/Kconfig
@@ -14,3 +14,6 @@ config EFI_SECRET
 
 	  To compile this driver as a module, choose M here.
 	  The module will be called efi_secret.
+
+	  The module is loaded automatically by the EFI driver if the EFI
+	  secret area is populated.
-- 
2.25.1

[PATCH v7 5/5] docs: security: Add coco/efi_secret documentation

[Dov Murik]

Add documentation for the efi_secret module which allows access
to Confidential Computing injected secrets.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 Documentation/security/coco/efi_secret.rst | 102 +++++++++++++++++++++
 Documentation/security/coco/index.rst      |   9 ++
 Documentation/security/index.rst           |   1 +
 3 files changed, 112 insertions(+)
 create mode 100644 Documentation/security/coco/efi_secret.rst
 create mode 100644 Documentation/security/coco/index.rst

diff --git a/Documentation/security/coco/efi_secret.rst b/Documentation/security/coco/efi_secret.rst
new file mode 100644
index 000000000000..fa74899b68a3
--- /dev/null
+++ b/Documentation/security/coco/efi_secret.rst
@@ -0,0 +1,102 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+==========
+EFI secret
+==========
+
+This document describes how Confidential Computing secret injection is handled
+from the firmware to the operating system, in the EFI driver and the efi_secret
+kernel module.
+
+
+Introduction
+============
+
+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.
+
+The efi_secret kernel module allows userspace applications to access these
+secrets via securityfs.
+
+
+Secret data flow
+================
+
+The guest firmware may reserve a designated memory area for secret injection,
+and publish its location (base GPA and length) in the EFI configuration table
+under a ``LINUX_EFI_COCO_SECRET_AREA_GUID`` entry
+(``adf956ad-e98c-484c-ae11-b51c7d336447``).  This memory area should be marked
+by the firmware as ``EFI_RESERVED_TYPE``, and therefore the kernel should not
+be use it for its own purposes.
+
+During the VM's launch, the virtual machine manager may inject a secret to that
+area.  In AMD SEV and SEV-ES this is performed using the
+``KVM_SEV_LAUNCH_SECRET`` command (see [sev]_).  The strucutre of the injected
+Guest Owner secret data should be a GUIDed table of secret values; the binary
+format is described in ``drivers/virt/coco/efi_secret/efi_secret.c`` under
+"Structure of the EFI secret area".
+
+On kernel start, the kernel's EFI driver saves the location of the secret area
+(taken from the EFI configuration table) in the ``efi.coco_secret`` field.
+Later it checks if the secret area is populated: it maps the area and checks
+whether its content begins with ``EFI_SECRET_TABLE_HEADER_GUID``
+(``1e74f542-71dd-4d66-963e-ef4287ff173b``).  If the secret area is populated,
+the EFI driver will autoload the efi_secret kernel module, which exposes the
+secretes to userspace applications via securityfs.  The details of the
+efi_secret filesystem interface are in [efi-secret-abi]_.
+
+
+Application 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.
+
+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::
+
+	# 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
+
+
+References
+==========
+
+See [sev-api-spec]_ for more info regarding SEV ``LAUNCH_SECRET`` operation.
+
+.. [sev] Documentation/virt/kvm/amd-memory-encryption.rst
+.. [efi-secret-abi] Documentation/ABI/testing/securityfs-coco-efi_secret
+.. [sev-api-spec] https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
diff --git a/Documentation/security/coco/index.rst b/Documentation/security/coco/index.rst
new file mode 100644
index 000000000000..56b803d4b33e
--- /dev/null
+++ b/Documentation/security/coco/index.rst
@@ -0,0 +1,9 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+====================================
+Confidential Computing documentation
+====================================
+
+.. toctree::
+
+   efi_secret
diff --git a/Documentation/security/index.rst b/Documentation/security/index.rst
index 16335de04e8c..3662e93a3eba 100644
--- a/Documentation/security/index.rst
+++ b/Documentation/security/index.rst
@@ -17,3 +17,4 @@ Security Documentation
    tpm/index
    digsig
    landlock
+   coco/index
-- 
2.25.1

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote:
> 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 keeps the address of the EFI-provided memory for
> injected secrets, and exposes the secrets to userspace via securityfs
> using a new efi_secret kernel module.  The module is autoloaded (by the
> EFI driver) if the secret area is populated.
> 
> The first patch in EFI keeps the address of the secret area as passed in
> the EFI configuration table.  The second patch is a quirk fix for older
> firmwares didn't mark the secrets page as EFI_RESERVED_TYPE.  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.  The fourth patch auto-loads the
> efi_secret module during startup if the injected secrets area is
> populated.  The last patch documents the data flow of confidential
> computing secret injection.
> 
> 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:
> 
> # 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

Please see my comments on the powerpc version of this type of thing:
	https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com


You all need to work together to come up with a unified place for this
and stop making it platform-specific.

Until then, we can't take this.

sorry,

greg k-h

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[James Bottomley]

[cc's added]
On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
> On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote:
[...]
> > # 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
> 
> Please see my comments on the powerpc version of this type of thing:
> 	
> https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com

If you want a debate, actually cc'ing the people on the other thread
would have been a good start ...

For those added, this patch series is at:

https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/

> You all need to work together to come up with a unified place for
> this and stop making it platform-specific.

I'm not entirely sure of that.  If you look at the differences between
EFI variables and the COCO proposal: the former has an update API
which, in the case of signed variables, is rather complex and a UC16
content requirement.  The latter is binary data with read only/delete. 
Plus each variable in EFI is described by a GUID, so having a directory
of random guids, some of which behave like COCO secrets and some of
which are EFI variables is going to be incredibly confusing (and also
break all our current listing tools which seems somewhat undesirable).

So we could end up with 

<common path prefix>/efivar
<common path prefix>/coco

To achieve the separation, but I really don't see what this buys us. 
Both filesystems would likely end up with different backends because of
the semantic differences and we can easily start now in different
places (effectively we've already done this for efi variables) and
unify later if that is the chosen direction, so it doesn't look like a
blocker.

> Until then, we can't take this.

I don't believe anyone was asking you to take it.

James

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote:
> [cc's added]
> On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
> > On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote:
> [...]
> > > # 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
> > 
> > Please see my comments on the powerpc version of this type of thing:
> > 	
> > https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com
> 
> If you want a debate, actually cc'ing the people on the other thread
> would have been a good start ...
> 
> For those added, this patch series is at:
> 
> https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/

Thanks for adding everyone.

> > You all need to work together to come up with a unified place for
> > this and stop making it platform-specific.
> 
> I'm not entirely sure of that.  If you look at the differences between
> EFI variables and the COCO proposal: the former has an update API
> which, in the case of signed variables, is rather complex and a UC16
> content requirement.  The latter is binary data with read only/delete. 
> Plus each variable in EFI is described by a GUID, so having a directory
> of random guids, some of which behave like COCO secrets and some of
> which are EFI variables is going to be incredibly confusing (and also
> break all our current listing tools which seems somewhat undesirable).
> 
> So we could end up with 
> 
> <common path prefix>/efivar
> <common path prefix>/coco

The powerpc stuff is not efi.  But yes, that is messy here.  But why
doesn't the powerpc follow the coco standard?

> To achieve the separation, but I really don't see what this buys us. 
> Both filesystems would likely end up with different backends because of
> the semantic differences and we can easily start now in different
> places (effectively we've already done this for efi variables) and
> unify later if that is the chosen direction, so it doesn't look like a
> blocker.
> 
> > Until then, we can't take this.
> 
> I don't believe anyone was asking you to take it.

I was on the review list...

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[James Bottomley]

On Tue, 2022-02-01 at 15:41 +0100, Greg KH wrote:
> On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote:
> > [cc's added]
> > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
[...]
> > > You all need to work together to come up with a unified place for
> > > this and stop making it platform-specific.
> > 
> > I'm not entirely sure of that.  If you look at the differences
> > between EFI variables and the COCO proposal: the former has an
> > update API which, in the case of signed variables, is rather
> > complex and a UC16 content requirement.  The latter is binary data
> > with read only/delete.  Plus each variable in EFI is described by a
> > GUID, so having a directory of random guids, some of which behave
> > like COCO secrets and some of which are EFI variables is going to
> > be incredibly confusing (and also break all our current listing
> > tools which seems somewhat undesirable).
> > 
> > So we could end up with 
> > 
> > <common path prefix>/efivar
> > <common path prefix>/coco
> 
> The powerpc stuff is not efi.  But yes, that is messy here.  But why
> doesn't the powerpc follow the coco standard?

There is no coco standard for EFI variables.  There's only a UEFI
variable standard which, I believe, power tries to follow in some
measure since the variables are mostly used for its version of secure
boot.  Certainly you're either a power or UEFI platform but not both,
so they could live at the same location ... that's not true with the
coco ones.  I added the cc's to see if there are other ideas, but I
really think the use cases are too disjoint.

As Daniel has previously proposed, it might be possible to unify the
power and UEFI implementations ... useful if we want them to respond to
the same tooling, but we'll do that by giving them the same EFI
semantics.  The semantics and source of the coco secrets will still be
immutable and completely alien to whatever backend does the non
volatile power/efi authenticated variables, so we'll still need two
different backends and then it's just a question of arguing about path,
which doesn't make sense as a blocker.

> > To achieve the separation, but I really don't see what this buys
> > us.  Both filesystems would likely end up with different backends
> > because of the semantic differences and we can easily start now in
> > different places (effectively we've already done this for efi
> > variables) and unify later if that is the chosen direction, so it
> > doesn't look like a blocker.
> > 
> > > Until then, we can't take this.
> > 
> > I don't believe anyone was asking you to take it.
> 
> I was on the review list...

You raised a doc/API concenrn.  I think you were on the review list to
ensure it got addressed.

James

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) wrote:
> [cc's added]
> On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
> > On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote:
> [...]
> > > # 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
> > 
> > Please see my comments on the powerpc version of this type of thing:
> > 	
> > https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com
> 
> If you want a debate, actually cc'ing the people on the other thread
> would have been a good start ...
> 
> For those added, this patch series is at:
> 
> https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/
> 
> > You all need to work together to come up with a unified place for
> > this and stop making it platform-specific.
> 
> I'm not entirely sure of that.  If you look at the differences between
> EFI variables and the COCO proposal: the former has an update API
> which, in the case of signed variables, is rather complex and a UC16
> content requirement.  The latter is binary data with read only/delete. 
> Plus each variable in EFI is described by a GUID, so having a directory
> of random guids, some of which behave like COCO secrets and some of
> which are EFI variables is going to be incredibly confusing (and also
> break all our current listing tools which seems somewhat undesirable).
> 
> So we could end up with 
> 
> <common path prefix>/efivar
> <common path prefix>/coco
> 
> To achieve the separation, but I really don't see what this buys us. 
> Both filesystems would likely end up with different backends because of
> the semantic differences and we can easily start now in different
> places (effectively we've already done this for efi variables) and
> unify later if that is the chosen direction, so it doesn't look like a
> blocker.
> 
> > Until then, we can't take this.
> 
> I don't believe anyone was asking you to take it.

I have some sympathy in wanting some unification; (I'm not sure that
list of comparison even includes the TDX world).
But I'm not sure if they're the same thing - these are strictly
constants, they're not changable.

But it is a messy list of differences - especially things like the
UTF-16 stuff
I guess the PowerVM key naming contains nul and / can be ignored
- if anyone is silly enough to create keys with those names then they
can not access them; so at least that would solve that problem.

I don't really understand the talk of 32bit attributes in either the
uEFI or PowerVM key store case.

Is that GOOGLE_SMI stuff already there? If so I guess there's not much
we can do  - but it's a shame that there's the directory per variable.

Dave



> James
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote:
> On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
> > You all need to work together to come up with a unified place for
> > this and stop making it platform-specific.

We're talking about things that have massively different semantics. How 
do we expose that without an unwieldy API that has to try to be a 
superset of everything implemented, which then has to be extended when 
yet another implementation shows up with another behavioural quirk? EFI 
variables already need extremely careful handling to avoid rm -rf /sys 
bricking the system - should we impose that on everything, or should we 
allow the underlying implementation to leak through in some ways?

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote:
> On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote:
> > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote:
> > > You all need to work together to come up with a unified place for
> > > this and stop making it platform-specific.
> 
> We're talking about things that have massively different semantics.

I see lots of different platforms trying to provide access to their
"secure" firmware data to userspace in different ways.  That feels to me
like they are the same thing that userspace would care about in a
unified way.

Unless we expeect userspace tools to have to be platform-specific for
all of this?  That does not seem wise.

what am I missing here?

thanks,

greg k-h

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

On Wed, Feb 02, 2022 at 07:10:02AM +0100, Greg KH wrote:
> On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote:
> > We're talking about things that have massively different semantics.
> 
> I see lots of different platforms trying to provide access to their
> "secure" firmware data to userspace in different ways.  That feels to me
> like they are the same thing that userspace would care about in a
> unified way.

EFI variables are largely for the OS to provide information to the 
firmware, while this patchset is to provide information from the 
firmware to the OS. I don't see why we'd expect to use the same userland 
tooling for both.

In the broader case - I don't think we *can* use the same userland
tooling for everything. For example, the patches to add support for 
manipulating the Power secure boot keys originally attempted to make it 
look like efivars, but the underlying firmware semantics are 
sufficiently different that even exposing the same kernel interface 
wouldn't be a sufficient abstraction and userland would still need to 
behave differently. Exposing an interface that looks consistent but 
isn't is arguably worse for userland than exposing explicitly distinct 
interfaces.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

On Wed, Feb 02, 2022 at 06:54:43AM +0000, Matthew Garrett wrote:
> On Wed, Feb 02, 2022 at 07:10:02AM +0100, Greg KH wrote:
> > On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote:
> > > We're talking about things that have massively different semantics.
> > 
> > I see lots of different platforms trying to provide access to their
> > "secure" firmware data to userspace in different ways.  That feels to me
> > like they are the same thing that userspace would care about in a
> > unified way.
> 
> EFI variables are largely for the OS to provide information to the 
> firmware, while this patchset is to provide information from the 
> firmware to the OS. I don't see why we'd expect to use the same userland 
> tooling for both.

I totally agree, I'm not worried about EFI variables here, I don't know
why that came up.

> In the broader case - I don't think we *can* use the same userland
> tooling for everything. For example, the patches to add support for 
> manipulating the Power secure boot keys originally attempted to make it 
> look like efivars, but the underlying firmware semantics are 
> sufficiently different that even exposing the same kernel interface 
> wouldn't be a sufficient abstraction and userland would still need to 
> behave differently. Exposing an interface that looks consistent but 
> isn't is arguably worse for userland than exposing explicitly distinct 
> interfaces.

So what does userspace really need here?  Just the ability to find if
the platform has blobs that it cares about, and how to read/write them.

I see different platform patches trying to stick these blobs in
different locations and ways to access (securityfs, sysfs, char device
node), which seems crazy to me.  Why can't we at least pick one way to
access these to start with, and then have the filesystem layout be
platform-specific as needed, which will give the correct hints to
userspace as to what it needs to do here?

thanks,

greg k-h

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

On Wed, Feb 02, 2022 at 08:05:23AM +0100, Greg KH wrote:

> I see different platform patches trying to stick these blobs in
> different locations and ways to access (securityfs, sysfs, char device
> node), which seems crazy to me.  Why can't we at least pick one way to
> access these to start with, and then have the filesystem layout be
> platform-specific as needed, which will give the correct hints to
> userspace as to what it needs to do here?

Which other examples are you thinking of? I think this conversation may 
have accidentally become conflated with a different prior one and now 
we're talking at cross purposes.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Ard Biesheuvel]

On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>
> On Wed, Feb 02, 2022 at 08:05:23AM +0100, Greg KH wrote:
>
> > I see different platform patches trying to stick these blobs in
> > different locations and ways to access (securityfs, sysfs, char device
> > node), which seems crazy to me.  Why can't we at least pick one way to
> > access these to start with, and then have the filesystem layout be
> > platform-specific as needed, which will give the correct hints to
> > userspace as to what it needs to do here?
>
> Which other examples are you thinking of? I think this conversation may
> have accidentally become conflated with a different prior one and now
> we're talking at cross purposes.

This came up a while ago during review of one of the earlier revisions
of this patch set.

https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/

which describes another two variations on the theme, for pKVM guests
as well as Android bare metal.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote:
> On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > Which other examples are you thinking of? I think this conversation may
> > have accidentally become conflated with a different prior one and now
> > we're talking at cross purposes.
> 
> This came up a while ago during review of one of the earlier revisions
> of this patch set.
> 
> https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/
> 
> which describes another two variations on the theme, for pKVM guests
> as well as Android bare metal.

Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, 
so thought this was related to the efivars/Power secure boot. My 
apologies, sorry for the noise. In that case, given the apparent 
agreement between the patch owners that a consistent interface would 
work for them, I think I agree with Greg that we should strive for that. 
Given the described behaviour of the Google implementation, it feels 
like the semantics in this implementation would be sufficient for them 
as well, but having confirmation of that would be helpful.

On the other hand, I also agree that a new filesystem for this is 
overkill. I did that for efivarfs and I think the primary lesson from 
that is that people who aren't familiar with the vfs shouldn't be 
writing filesystems. Securityfs seems entirely reasonable, and it's 
consistent with other cases where we expose firmware-provided data 
that's security relevant.

The only thing I personally struggle with here is whether "coco" is the 
best name for it, and whether there are reasonable use cases that 
wouldn't be directly related to confidential computing (eg, if the 
firmware on a bare-metal platform had a mechanism for exposing secrets 
to the OS based on some specific platform security state, it would seem 
reasonable to expose it via this mechanism but it may not be what we'd 
normally think of as Confidential Computing).

But I'd also say that while we only have one implementation currently 
sending patches, it's fine for the code to live in that implementation 
and then be abstracted out once we have another.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

On Wed, Feb 02, 2022 at 08:04:01AM +0000, Matthew Garrett wrote:
> On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote:
> > On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> > > Which other examples are you thinking of? I think this conversation may
> > > have accidentally become conflated with a different prior one and now
> > > we're talking at cross purposes.
> > 
> > This came up a while ago during review of one of the earlier revisions
> > of this patch set.
> > 
> > https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/
> > 
> > which describes another two variations on the theme, for pKVM guests
> > as well as Android bare metal.
> 
> Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, 
> so thought this was related to the efivars/Power secure boot. My 
> apologies, sorry for the noise. In that case, given the apparent 
> agreement between the patch owners that a consistent interface would 
> work for them, I think I agree with Greg that we should strive for that. 
> Given the described behaviour of the Google implementation, it feels 
> like the semantics in this implementation would be sufficient for them 
> as well, but having confirmation of that would be helpful.
> 
> On the other hand, I also agree that a new filesystem for this is 
> overkill. I did that for efivarfs and I think the primary lesson from 
> that is that people who aren't familiar with the vfs shouldn't be 
> writing filesystems. Securityfs seems entirely reasonable, and it's 
> consistent with other cases where we expose firmware-provided data 
> that's security relevant.
> 
> The only thing I personally struggle with here is whether "coco" is the 
> best name for it, and whether there are reasonable use cases that 
> wouldn't be directly related to confidential computing (eg, if the 
> firmware on a bare-metal platform had a mechanism for exposing secrets 
> to the OS based on some specific platform security state, it would seem 
> reasonable to expose it via this mechanism but it may not be what we'd 
> normally think of as Confidential Computing).
> 
> But I'd also say that while we only have one implementation currently 
> sending patches, it's fine for the code to live in that implementation 
> and then be abstracted out once we have another.

Well right now the Android code looks the cleanest and should be about
ready to be merged into my tree.

But I can almost guarantee that that interface is not what anyone else
wants to use, so if you think somehow that everyone else is going to
want to deal with a char device node and a simple mmap, with a DT
description of the thing, hey, I'm all for it :)

Seriously, people need to come up with something sane or this is going
to be a total mess.

thanks,

greg k-h

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Gerd Hoffmann]

  Hi,
 
> The only thing I personally struggle with here is whether "coco" is the 
> best name for it, and whether there are reasonable use cases that 
> wouldn't be directly related to confidential computing (eg, if the 
> firmware on a bare-metal platform had a mechanism for exposing secrets 
> to the OS based on some specific platform security state, it would seem 
> reasonable to expose it via this mechanism but it may not be what we'd 
> normally think of as Confidential Computing).
> 
> But I'd also say that while we only have one implementation currently 
> sending patches, it's fine for the code to live in that implementation 
> and then be abstracted out once we have another.

The implementation can be sorted later when a second implementation
shows up, but it'll be better if we don't have to change the naming
convention.

"coco/efi_secrets" doesn't look like a good choice to me, given that it
is rather likely we see more users for this showing up.

Having a "secrets/" directory looks good to me.  Then the individual
implementations can either add files to the directory, i.e. efi_secrets
would create "secrets/<guid>" files.  Or each implementation creates a
subdirectory with the secrets, i.e. "secrets/coco/" and
"secrets/coco/<guid>".

Longer-term (i.e once we have more than one implementation) we probably
need a separate module which owns and manages the "secrets/" directory,
and possibly provides some common helper functions too.

take care,
  Gerd

Re: [PATCH v7 1/5] efi: Save location of EFI confidential computing area

[Gerd Hoffmann]

On Tue, Feb 01, 2022 at 12:44:09PM +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.
> 
> If EFI exposes such a table entry, uefi_init() will keep a pointer to
> the EFI config table entry in efi.coco_secret, so it can be used later
> by the kernel (specifically drivers/virt/coco/efi_secret).  It will also
> appear in the kernel log as "CocoSecret=ADDRESS"; for example:
> 
>     [    0.000000] efi: EFI v2.70 by EDK II
>     [    0.000000] efi: CocoSecret=0x7f22e680 SMBIOS=0x7f541000 ACPI=0x7f77e000 ACPI 2.0=0x7f77e014 MEMATTR=0x7ea0c018
> 
> The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v7 2/5] efi/libstub: Reserve confidential computing secret area

[Gerd Hoffmann]

On Tue, Feb 01, 2022 at 12:44:10PM +0000, Dov Murik wrote:
> Some older firmware declare the confidential computing secret area as
> EFI_BOOT_SERVICES_DATA region.  Fix this up by treating this memory
> region as EFI_RESERVED_TYPE, as it should be.
> 
> If that memory region is already EFI_RESERVED_TYPE then this has no
> effect on the E820 map.

Hmm, sure we actually want merge this?  I suspect by the time this
landed in an upstream kernel "older firmware" isn't much of a problem
any more.

take care,
  Gerd

Re: [PATCH v7 3/5] virt: Add efi_secret module to expose confidential computing secrets

[Gerd Hoffmann]

  Hi,

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

Why have two levels of subdirectories here?  Do we expect more users for
the coco/ directory?

See also the naming discussion in the cover letter sub-thread.

take care,
  Gerd

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

On Wed, Feb 02, 2022 at 09:36:53AM +0100, Gerd Hoffmann wrote:

> Having a "secrets/" directory looks good to me.  Then the individual
> implementations can either add files to the directory, i.e. efi_secrets
> would create "secrets/<guid>" files.  Or each implementation creates a
> subdirectory with the secrets, i.e. "secrets/coco/" and
> "secrets/coco/<guid>".

I prefer a subdirectory, on the basis that we could conceivably end up 
with more than one implementation on a single device at some point, and 
also because it makes it trivial for userland to determine what the 
source is which may make a semantic difference under certain 
circumstances.
 
> Longer-term (i.e once we have more than one implementation) we probably
> need a separate module which owns and manages the "secrets/" directory,
> and possibly provides some common helper functions too.

Agree.

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Gerd Hoffmann]

On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik wrote:
> If the efi_secret module is built, register a late_initcall in the EFI
> driver which checks whether the EFI secret area is available and
> populated, and then requests to load the efi_secret module.

> +	area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB);
> +	if (!area) {
> +		pr_err("Failed to map confidential computing secret area descriptor\n");
> +		return -ENOMEM;
> +	}
> +	if (!area->base_pa || area->size < sizeof(*header_guid))
> +		goto unmap_desc;
> +
> +	header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid));
> +	if (!header_guid) {
> +		pr_err("Failed to map secret area\n");
> +		ret = -ENOMEM;
> +		goto unmap_desc;
> +	}
> +	if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID))
> +		goto unmap_encrypted;

Why these sanity checks are here and not in the efi_secret module?

take care,
  Gerd

Re: [PATCH v7 5/5] docs: security: Add coco/efi_secret documentation

[Gerd Hoffmann]

On Tue, Feb 01, 2022 at 12:44:13PM +0000, Dov Murik wrote:
> Add documentation for the efi_secret module which allows access
> to Confidential Computing injected secrets.

Looks good, but might need updates when the paths change.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v7 3/5] virt: Add efi_secret module to expose confidential computing secrets

[Dov Murik]

On 02/02/2022 10:45, Gerd Hoffmann wrote:
>   Hi,
> 
>> +	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;
> 
> Why have two levels of subdirectories here?  Do we expect more users for
> the coco/ directory?
> 

In the RFCv2 of this series Borislav Petkov suggested [1] adding a
"coco/" directory (originally it was "<securityfs>/sev_secret"); he
envisioned that other coco platforms (SNP, TDX) might want to expose
something to userspace via filesystem.

[1] https://lore.kernel.org/linux-coco/YNoiydeow+ftvfYX@zn.tnic/



> See also the naming discussion in the cover letter sub-thread.

Yep, following.



Thanks,
Dov

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Dov Murik]

On 02/02/2022 10:47, Gerd Hoffmann wrote:
> On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik wrote:
>> If the efi_secret module is built, register a late_initcall in the EFI
>> driver which checks whether the EFI secret area is available and
>> populated, and then requests to load the efi_secret module.
> 
>> +	area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB);
>> +	if (!area) {
>> +		pr_err("Failed to map confidential computing secret area descriptor\n");
>> +		return -ENOMEM;
>> +	}
>> +	if (!area->base_pa || area->size < sizeof(*header_guid))
>> +		goto unmap_desc;
>> +
>> +	header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid));
>> +	if (!header_guid) {
>> +		pr_err("Failed to map secret area\n");
>> +		ret = -ENOMEM;
>> +		goto unmap_desc;
>> +	}
>> +	if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID))
>> +		goto unmap_encrypted;
> 
> Why these sanity checks are here and not in the efi_secret module?
> 

The same checks indeed appear in the efi_secret module (see in patch 3:
efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()).

However, in the efi_secret module, the checks are noisy, because they
expect the secret area to be populated.  For example:

+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) {
+		pr_err("Secret area address is not available\n");
+		return -EINVAL;
+	}
...
+	if (efi_guidcmp(h->guid, EFI_SECRET_TABLE_HEADER_GUID)) {
+		pr_err("EFI secret area does not start with correct GUID\n");
+		ret = -EINVAL;
+		goto err_cleanup;
+	}
...


Whereas here (patch 4, in efi/coco.c) the EFI driver checks if there a
populated secret area, and if there is one, triggers the efi_secret
module load.


Another approach could be to just try to load the module anyway, and the
module will fail (silently? noisily?) if there's no designated secret area
or it's not populated.  I feel that will be harder to understand what's going on.


I'm open to suggestions in case this duplication is too bad (maybe efi_secret
can expose some kind of probe() function that can be called before actually
initializing it?)


Thanks,
-Dov

Re: [PATCH v7 2/5] efi/libstub: Reserve confidential computing secret area

[Dov Murik]

On 02/02/2022 10:41, Gerd Hoffmann wrote:
> On Tue, Feb 01, 2022 at 12:44:10PM +0000, Dov Murik wrote:
>> Some older firmware declare the confidential computing secret area as
>> EFI_BOOT_SERVICES_DATA region.  Fix this up by treating this memory
>> region as EFI_RESERVED_TYPE, as it should be.
>>
>> If that memory region is already EFI_RESERVED_TYPE then this has no
>> effect on the E820 map.
> 
> Hmm, sure we actually want merge this?  I suspect by the time this
> landed in an upstream kernel "older firmware" isn't much of a problem
> any more.
> 

When we originally wrote this patch the OVMF fix was not yet upstream
(and currently it is still not part of an official edk2 stable tag/release).

But I agree that as time goes by, the need for this fix is diminishing.

I'll consider dropping this patch entirely in the next round.

Thanks,
-Dov

Re: [PATCH v7 5/5] docs: security: Add coco/efi_secret documentation

[Dov Murik]

On 02/02/2022 10:49, Gerd Hoffmann wrote:
> On Tue, Feb 01, 2022 at 12:44:13PM +0000, Dov Murik wrote:
>> Add documentation for the efi_secret module which allows access
>> to Confidential Computing injected secrets.
> 
> Looks good, but might need updates when the paths change.
> 
> Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
> 

Thanks for reviewing the series.

-Dov

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Gerd Hoffmann]

On Wed, Feb 02, 2022 at 01:08:43PM +0200, Dov Murik wrote:
> 
> 
> On 02/02/2022 10:47, Gerd Hoffmann wrote:
> > On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik wrote:
> >> If the efi_secret module is built, register a late_initcall in the EFI
> >> driver which checks whether the EFI secret area is available and
> >> populated, and then requests to load the efi_secret module.
> > 
> >> +	area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB);
> >> +	if (!area) {
> >> +		pr_err("Failed to map confidential computing secret area descriptor\n");
> >> +		return -ENOMEM;
> >> +	}
> >> +	if (!area->base_pa || area->size < sizeof(*header_guid))
> >> +		goto unmap_desc;
> >> +
> >> +	header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid));
> >> +	if (!header_guid) {
> >> +		pr_err("Failed to map secret area\n");
> >> +		ret = -ENOMEM;
> >> +		goto unmap_desc;
> >> +	}
> >> +	if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID))
> >> +		goto unmap_encrypted;
> > 
> > Why these sanity checks are here and not in the efi_secret module?
> 
> The same checks indeed appear in the efi_secret module (see in patch 3:
> efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()).
> 
> However, in the efi_secret module, the checks are noisy, because they
> expect the secret area to be populated.  For example:
> 
> +	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) {
> +		pr_err("Secret area address is not available\n");
> +		return -EINVAL;
> +	}

Note I explicitly excluded that check ;)

Checking whenever efi.coco_secret looks valid and only try load
efi_secret if that is the case (and otherwise stay silent) makes
perfect sense.  The other checks should be dropped IMHO.

> Another approach could be to just try to load the module anyway, and
> the module will fail (silently? noisily?) if there's no designated
> secret area or it's not populated.  I feel that will be harder to
> understand what's going on.

I think the module should fail noisily.  See above for autoload.  In
case the module is loaded (either manually by the admin, or because
efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load
the secrets we want know why ...

take care,
  Gerd

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Dov Murik]

On 02/02/2022 16:31, Gerd Hoffmann wrote:
> On Wed, Feb 02, 2022 at 01:08:43PM +0200, Dov Murik wrote:
>>
>>
>> On 02/02/2022 10:47, Gerd Hoffmann wrote:
>>> On Tue, Feb 01, 2022 at 12:44:12PM +0000, Dov Murik wrote:
>>>> If the efi_secret module is built, register a late_initcall in the EFI
>>>> driver which checks whether the EFI secret area is available and
>>>> populated, and then requests to load the efi_secret module.
>>>
>>>> +	area = memremap(efi.coco_secret, sizeof(*area), MEMREMAP_WB);
>>>> +	if (!area) {
>>>> +		pr_err("Failed to map confidential computing secret area descriptor\n");
>>>> +		return -ENOMEM;
>>>> +	}
>>>> +	if (!area->base_pa || area->size < sizeof(*header_guid))
>>>> +		goto unmap_desc;
>>>> +
>>>> +	header_guid = (void __force *)ioremap_encrypted(area->base_pa, sizeof(*header_guid));
>>>> +	if (!header_guid) {
>>>> +		pr_err("Failed to map secret area\n");
>>>> +		ret = -ENOMEM;
>>>> +		goto unmap_desc;
>>>> +	}
>>>> +	if (efi_guidcmp(*header_guid, EFI_SECRET_TABLE_HEADER_GUID))
>>>> +		goto unmap_encrypted;
>>>
>>> Why these sanity checks are here and not in the efi_secret module?
>>
>> The same checks indeed appear in the efi_secret module (see in patch 3:
>> efi_secret_map_area() and the beginning of efi_secret_securityfs_setup()).
>>
>> However, in the efi_secret module, the checks are noisy, because they
>> expect the secret area to be populated.  For example:
>>
>> +	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) {
>> +		pr_err("Secret area address is not available\n");
>> +		return -EINVAL;
>> +	}
> 
> Note I explicitly excluded that check ;)
> 
> Checking whenever efi.coco_secret looks valid and only try load
> efi_secret if that is the case (and otherwise stay silent) makes
> perfect sense.  The other checks should be dropped IMHO.
> 
>> Another approach could be to just try to load the module anyway, and
>> the module will fail (silently? noisily?) if there's no designated
>> secret area or it's not populated.  I feel that will be harder to
>> understand what's going on.
> 
> I think the module should fail noisily.  See above for autoload.  In
> case the module is loaded (either manually by the admin, or because
> efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load
> the secrets we want know why ...
> 

Note that the AmdSev build of OVMF always publishes
LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table.  Even when
LAUNCH_SECRET was not executed.  In such cases the secret area will be
empty.

If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check,
we'll get errors from efi_secret for every VM launch that doesn't
undergo LAUNCH_SECRET.  I don't think that's good.

If we *do* want to check that the area starts with
EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the
checks before that, like checking that the area is big enough, and that
all the memremap()s succeed -- before actually comparing the
header_guid.  The checks are basically prerequisites for calling
efi_guidcmp() safely.

-Dov

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Gerd Hoffmann]

  Hi,

> > I think the module should fail noisily.  See above for autoload.  In
> > case the module is loaded (either manually by the admin, or because
> > efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load
> > the secrets we want know why ...
> 
> Note that the AmdSev build of OVMF always publishes
> LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table.  Even when
> LAUNCH_SECRET was not executed.  In such cases the secret area will be
> empty.

Hmm, ok.  Why?  I assume the secret area is filled by the host and ovmf
doesn't even look at it?

> If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check,
> we'll get errors from efi_secret for every VM launch that doesn't
> undergo LAUNCH_SECRET.  I don't think that's good.

Well, if that is a common case the module could either print nothing or
log KERN_INFO level instead of KERN_ERROR.

> If we *do* want to check that the area starts with
> EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the
> checks before that, like checking that the area is big enough, and that
> all the memremap()s succeed -- before actually comparing the
> header_guid.  The checks are basically prerequisites for calling
> efi_guidcmp() safely.

It is still not fully clear to me why you want do that check twice.

take care,
  Gerd

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Dov Murik]

On 03/02/2022 8:16, Gerd Hoffmann wrote:
>   Hi,
> 
>>> I think the module should fail noisily.  See above for autoload.  In
>>> case the module is loaded (either manually by the admin, or because
>>> efi.coco_secret != EFI_INVALID_TABLE_ADDR) and it can't actually load
>>> the secrets we want know why ...
>>
>> Note that the AmdSev build of OVMF always publishes
>> LINUX_EFI_COCO_SECRET_TABLE_GUID in the EFI table.  Even when
>> LAUNCH_SECRET was not executed.  In such cases the secret area will be
>> empty.
> 
> Hmm, ok.  Why?  I assume the secret area is filled by the host and ovmf
> doesn't even look at it?
> 

Exactly.  OVMF just reserves this area, and puts its address+size in the
EFI config table.  It doesn't care about its format and usage.

There are currently two "users" for the actual data in this memory area:

1. grub's efisecret module (which reads the disk passphrase from an
entry in the secret area)
2. linux's efi_secret module (which we're discussing here)



>> If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check,
>> we'll get errors from efi_secret for every VM launch that doesn't
>> undergo LAUNCH_SECRET.  I don't think that's good.
> 
> Well, if that is a common case the module could either print nothing or
> log KERN_INFO level instead of KERN_ERROR.
> 

What if the user doesn't inject a secret and doesn't include the
efi_secret module at all in the initrd?  request_module("efi_secret")
will fail.

I can ignore the error code of request_module("efi_secret") but that
feels bad.



>> If we *do* want to check that the area starts with
>> EFI_SECRET_TABLE_HEADER_GUID (like I think we should), we need all the
>> checks before that, like checking that the area is big enough, and that
>> all the memremap()s succeed -- before actually comparing the
>> header_guid.  The checks are basically prerequisites for calling
>> efi_guidcmp() safely.
> 
> It is still not fully clear to me why you want do that check twice.
> 

I want to load the module only if secrets were injected by the Guest
Owner.

Again, I'm open to ideas on how to de-duplicate these early checks, if
that's important.


-Dov

Re: [PATCH v7 4/5] efi: Load efi_secret module if EFI secret area is populated

[Gerd Hoffmann]

  Hi,

> >> If we keep only the 'efi.coco_secret != EFI_INVALID_TABLE_ADDR' check,
> >> we'll get errors from efi_secret for every VM launch that doesn't
> >> undergo LAUNCH_SECRET.  I don't think that's good.
> > 
> > Well, if that is a common case the module could either print nothing or
> > log KERN_INFO level instead of KERN_ERROR.
> 
> What if the user doesn't inject a secret and doesn't include the
> efi_secret module at all in the initrd?  request_module("efi_secret")
> will fail.
> 
> I can ignore the error code of request_module("efi_secret") but that
> feels bad.

Looking at the error code returned by request_module should help to
figure what happened (module load failed / no secret present / something
else).

But, yes, module load errors are harmless in case there is no secret
present in the first place.  Hmm, tricky.  I don't see a way to solve
that without duplicating the checks.

I withdraw my objections.

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

take care,
  Gerd

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Dov Murik]

On 02/02/2022 10:45, Matthew Garrett wrote:
> On Wed, Feb 02, 2022 at 09:36:53AM +0100, Gerd Hoffmann wrote:
> 
>> Having a "secrets/" directory looks good to me.  Then the individual
>> implementations can either add files to the directory, i.e. efi_secrets
>> would create "secrets/<guid>" files.  Or each implementation creates a
>> subdirectory with the secrets, i.e. "secrets/coco/" and
>> "secrets/coco/<guid>".
> 
> I prefer a subdirectory, on the basis that we could conceivably end up 
> with more than one implementation on a single device at some point, and 
> also because it makes it trivial for userland to determine what the 
> source is which may make a semantic difference under certain 
> circumstances.
>  

OK, sounds good.  In the next round of the series the module will create
the files in <securityfs>/secrets/coco/ .


>> Longer-term (i.e once we have more than one implementation) we probably
>> need a separate module which owns and manages the "secrets/" directory,
>> and possibly provides some common helper functions too.
> 
> Agree.

Yes; one candidate for such helper function is a filesystem that
implements the "wipe file content from memory on unlink".


-Dov

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Nayna]

On 2/2/22 03:25, Greg KH wrote:
> On Wed, Feb 02, 2022 at 08:04:01AM +0000, Matthew Garrett wrote:
>> On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote:
>>> On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>>>> Which other examples are you thinking of? I think this conversation may
>>>> have accidentally become conflated with a different prior one and now
>>>> we're talking at cross purposes.
>>> This came up a while ago during review of one of the earlier revisions
>>> of this patch set.
>>>
>>> https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/
>>>
>>> which describes another two variations on the theme, for pKVM guests
>>> as well as Android bare metal.
>> Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that,
>> so thought this was related to the efivars/Power secure boot. My
>> apologies, sorry for the noise. In that case, given the apparent
>> agreement between the patch owners that a consistent interface would
>> work for them, I think I agree with Greg that we should strive for that.
>> Given the described behaviour of the Google implementation, it feels
>> like the semantics in this implementation would be sufficient for them
>> as well, but having confirmation of that would be helpful.
>>
>> On the other hand, I also agree that a new filesystem for this is
>> overkill. I did that for efivarfs and I think the primary lesson from
>> that is that people who aren't familiar with the vfs shouldn't be
>> writing filesystems. Securityfs seems entirely reasonable, and it's
>> consistent with other cases where we expose firmware-provided data
>> that's security relevant.
>>
>> The only thing I personally struggle with here is whether "coco" is the
>> best name for it, and whether there are reasonable use cases that
>> wouldn't be directly related to confidential computing (eg, if the
>> firmware on a bare-metal platform had a mechanism for exposing secrets
>> to the OS based on some specific platform security state, it would seem
>> reasonable to expose it via this mechanism but it may not be what we'd
>> normally think of as Confidential Computing).
>>
>> But I'd also say that while we only have one implementation currently
>> sending patches, it's fine for the code to live in that implementation
>> and then be abstracted out once we have another.
> Well right now the Android code looks the cleanest and should be about
> ready to be merged into my tree.
>
> But I can almost guarantee that that interface is not what anyone else
> wants to use, so if you think somehow that everyone else is going to
> want to deal with a char device node and a simple mmap, with a DT
> description of the thing, hey, I'm all for it :)
>
> Seriously, people need to come up with something sane or this is going
> to be a total mess.
>

Thanks for adding us to this discussion. I think somehow my last post 
got html content and didn't make to mailing list, so am posting it 
again. Sorry to those who are receiving it twice.

If I have understood the discussion right, the key idea discussed here 
is to unify multiple different interfaces(this one, and [1]) exposing 
secrets for confidential computing usecase via securityfs.

And the suggestion is to see if the proposed pseries interface [2] can 
unify with the coco interface.

At high level, pseries interface is reading/writing/adding/deleting 
variables using the sysfs interface, but the underlying semantics and 
actual usecases are quite different.

The variables exposed via pseries proposed interface are:

* Variables owned by firmware as read-only.
* Variables owned by bootloader as read-only.
* Variables owned by OS and get updated as signed updates. These support 
both read/write.
* Variables owned by OS and get directly updated(unsigned) eg config 
information or some boot variables. These support both read/write.

It can be extended to support variables which contain secrets like 
symmetric keys, are owned by OS and stored in platform keystore.

Naming convention wise also, there are differences like pseries 
variables do not use GUIDs.

The initial patchset discusses secure boot usecase, but it would be 
extended for other usecases as well.

First, I feel the purpose itself is different here. If we still 
continue, I fear if we will get into similar situation as Matthew 
mentioned in context of efivars -

"the patches to add support for
manipulating the Power secure boot keys originally attempted to make it
look like efivars, but the underlying firmware semantics are
sufficiently different that even exposing the same kernel interface
wouldn't be a sufficient abstraction and userland would still need to
behave differently. Exposing an interface that looks consistent but
isn't is arguably worse for userland than exposing explicitly distinct
interfaces."

With that, I believe the scope of pseries interface is different and 
much broader than being discussed here. So, I wonder if it would be 
better to still keep pseries interface separate from this and have its 
own platform specific interface.

I would be happy to hear the ideas.

[1] https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/

[2] https://lore.kernel.org/all/20220122005637.28199-1-nayna@linux.ibm.com/

Thanks & Regards,

      - Nayna