linux-efi.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
@ 2021-08-09 19:01 Dov Murik
  2021-08-09 19:01 ` [PATCH 1/3] efi/libstub: Copy confidential computing secret area Dov Murik
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Dov Murik @ 2021-08-09 19:01 UTC (permalink / raw)
  To: linux-efi
  Cc: Dov Murik, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

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

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

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

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

As a usage example, consider a guest performing computations on
encrypted files.  The Guest Owner provides the decryption key (= secret)
using the secret injection mechanism.  The guest application reads the
secret from the sev_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 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.

Here is a simple example for usage of the sev_secret module in a guest
to which a secret are with 4 secrets was injected during launch:

# modprobe sev_secret
# ls -la /sys/kernel/security/coco/sev_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/sev_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/sev_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910

# ls -la /sys/kernel/security/coco/sev_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


Previously sent as an RFC series [2].

[1] https://github.com/tianocore/edk2/commit/01726b6d23d4
[2] https://lore.kernel.org/linux-coco/20210628183431.953934-1-dovmurik@linux.ibm.com/


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

 arch/x86/platform/efi/efi.c               |   1 +
 drivers/firmware/efi/Makefile             |   2 +-
 drivers/firmware/efi/coco.c               |  41 +++
 drivers/firmware/efi/efi.c                |   3 +
 drivers/firmware/efi/libstub/Makefile     |   2 +-
 drivers/firmware/efi/libstub/coco.c       |  68 +++++
 drivers/firmware/efi/libstub/efi-stub.c   |   2 +
 drivers/firmware/efi/libstub/efistub.h    |   2 +
 drivers/firmware/efi/libstub/x86-stub.c   |   2 +
 drivers/virt/Kconfig                      |   3 +
 drivers/virt/Makefile                     |   1 +
 drivers/virt/coco/sev_secret/Kconfig      |  11 +
 drivers/virt/coco/sev_secret/Makefile     |   2 +
 drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++
 include/linux/efi.h                       |   9 +
 15 files changed, 460 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/coco.c
 create mode 100644 drivers/firmware/efi/libstub/coco.c
 create mode 100644 drivers/virt/coco/sev_secret/Kconfig
 create mode 100644 drivers/virt/coco/sev_secret/Makefile
 create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c


base-commit: 36a21d51725af2ce0700c6ebcb6b9594aac658a6
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 1/3] efi/libstub: Copy confidential computing secret area
  2021-08-09 19:01 [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Dov Murik
@ 2021-08-09 19:01 ` Dov Murik
  2021-08-09 19:01 ` [PATCH 2/3] efi: Reserve " Dov Murik
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 18+ messages in thread
From: Dov Murik @ 2021-08-09 19:01 UTC (permalink / raw)
  To: linux-efi
  Cc: Dov Murik, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

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

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

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

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

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

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d0537573501e..d77690b7dfb9 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -55,7 +55,7 @@ KCOV_INSTRUMENT			:= n
 lib-y				:= efi-stub-helper.o gop.o secureboot.o tpm.o \
 				   file.o mem.o random.o randomalloc.o pci.o \
 				   skip_spaces.o lib-cmdline.o lib-ctype.o \
-				   alignedmem.o relocate.o vsprintf.o
+				   alignedmem.o relocate.o vsprintf.o coco.o
 
 # include the stub's generic dependencies from lib/ when building for ARM/arm64
 efi-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
new file mode 100644
index 000000000000..bf546b6a3f72
--- /dev/null
+++ b/drivers/firmware/efi/libstub/coco.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing (coco) secret area handling
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#include <linux/efi.h>
+#include <linux/sizes.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
+	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
+
+/**
+ * struct efi_coco_secret_table - EFI config table that points to the
+ * confidential computing secret area. The guid
+ * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
+ * @base:	Physical address of the EFI secret area
+ * @size:	Size (in bytes) of the EFI secret area
+ */
+struct efi_coco_secret_table {
+	u64 base;
+	u64 size;
+} __attribute((packed));
+
+/*
+ * Create a copy of EFI's confidential computing secret area (if available) so
+ * that the secrets are accessible in the kernel after ExitBootServices.
+ */
+void efi_copy_coco_secret_area(void)
+{
+	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
+	efi_status_t status;
+	struct efi_coco_secret_table *secret_table;
+	struct linux_efi_coco_secret_area *secret_area;
+
+	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
+	if (!secret_table)
+		return;
+
+	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
+		return;
+
+	/* Allocate space for the secret area and copy it */
+	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
+
+	if (status != EFI_SUCCESS) {
+		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
+		return;
+	}
+
+	secret_area->size = secret_table->size;
+	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
+
+	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
+	if (status != EFI_SUCCESS)
+		goto err_free;
+
+	return;
+
+err_free:
+	efi_bs_call(free_pool, secret_area);
+}
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..18b3acd15c85 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -205,6 +205,8 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	/* Ask the firmware to clear memory on unclean shutdown */
 	efi_enable_reset_attack_mitigation();
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index cde0a2ef507d..d604c6744cef 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -858,4 +858,6 @@ efi_enable_reset_attack_mitigation(void) { }
 
 void efi_retrieve_tpm2_eventlog(void);
 
+void efi_copy_coco_secret_area(void);
+
 #endif
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..4ad85e1b6191 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -793,6 +793,8 @@ unsigned long efi_main(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6b5d36babfcc..9021dd521302 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -359,6 +359,7 @@ void efi_native_runtime_setup(void);
 #define LINUX_EFI_MEMRESERVE_TABLE_GUID		EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5,  0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
 #define LINUX_EFI_INITRD_MEDIA_GUID		EFI_GUID(0x5568e427, 0x68fc, 0x4f3d,  0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
 #define LINUX_EFI_MOK_VARIABLE_TABLE_GUID	EFI_GUID(0xc451ed2b, 0x9694, 0x45d3,  0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89)
+#define LINUX_EFI_COCO_SECRET_AREA_GUID		EFI_GUID(0x940ed1e9, 0xd3da, 0x408b,  0xb3, 0x07, 0xe3, 0x2d, 0x25, 0x4a, 0x65, 0x16)
 
 /* OEM GUIDs */
 #define DELLEMC_EFI_RCI2_TABLE_GUID		EFI_GUID(0x2d9f28a2, 0xa886, 0x456a,  0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
@@ -1282,4 +1283,9 @@ static inline struct efi_mokvar_table_entry *efi_mokvar_entry_find(
 }
 #endif
 
+struct linux_efi_coco_secret_area {
+	u32	size;
+	u8	area[];
+};
+
 #endif /* _LINUX_EFI_H */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 2/3] efi: Reserve confidential computing secret area
  2021-08-09 19:01 [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Dov Murik
  2021-08-09 19:01 ` [PATCH 1/3] efi/libstub: Copy confidential computing secret area Dov Murik
@ 2021-08-09 19:01 ` Dov Murik
  2021-08-09 19:01 ` [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
  2021-09-02 12:57 ` [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Greg KH
  3 siblings, 0 replies; 18+ messages in thread
From: Dov Murik @ 2021-08-09 19:01 UTC (permalink / raw)
  To: linux-efi
  Cc: Dov Murik, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

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

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

diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
index 147c30a81f15..35e082e5f603 100644
--- a/arch/x86/platform/efi/efi.c
+++ b/arch/x86/platform/efi/efi.c
@@ -93,6 +93,7 @@ static const unsigned long * const efi_tables[] = {
 #ifdef CONFIG_LOAD_UEFI_KEYS
 	&efi.mokvar_table,
 #endif
+	&efi.coco_secret,
 };
 
 u64 efi_setup;		/* efi setup_data physical address */
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 467e94259679..8703ffcca351 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -12,7 +12,7 @@ KASAN_SANITIZE_runtime-wrappers.o	:= n
 
 obj-$(CONFIG_ACPI_BGRT) 		+= efi-bgrt.o
 obj-$(CONFIG_EFI)			+= efi.o vars.o reboot.o memattr.o tpm.o
-obj-$(CONFIG_EFI)			+= memmap.o
+obj-$(CONFIG_EFI)			+= memmap.o coco.o
 ifneq ($(CONFIG_EFI_CAPSULE_LOADER),)
 obj-$(CONFIG_EFI)			+= capsule.o
 endif
diff --git a/drivers/firmware/efi/coco.c b/drivers/firmware/efi/coco.c
new file mode 100644
index 000000000000..42f477d6188c
--- /dev/null
+++ b/drivers/firmware/efi/coco.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing (coco) secret area handling
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include <linux/efi.h>
+#include <linux/init.h>
+#include <linux/memblock.h>
+#include <asm/early_ioremap.h>
+
+/*
+ * Reserve the confidential computing secret area memory
+ */
+int __init efi_coco_secret_area_reserve(void)
+{
+	struct linux_efi_coco_secret_area *secret_area;
+	unsigned long secret_area_size;
+
+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	secret_area = early_memremap(efi.coco_secret, sizeof(*secret_area));
+	if (!secret_area) {
+		pr_err("Failed to map confidential computing secret area\n");
+		efi.coco_secret = EFI_INVALID_TABLE_ADDR;
+		return -ENOMEM;
+	}
+
+	secret_area_size = sizeof(*secret_area) + secret_area->size;
+	memblock_reserve(efi.coco_secret, secret_area_size);
+
+	pr_info("Reserved memory of EFI-provided confidential computing secret area");
+
+	early_memunmap(secret_area, sizeof(*secret_area));
+	return 0;
+}
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 847f33ffc4ae..07e17ad225a6 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -526,6 +526,7 @@ static const efi_config_table_type_t common_tables[] __initconst = {
 #ifdef CONFIG_LOAD_UEFI_KEYS
 	{LINUX_EFI_MOK_VARIABLE_TABLE_GUID,	&efi.mokvar_table,	"MOKvar"	},
 #endif
+	{LINUX_EFI_COCO_SECRET_AREA_GUID,	&efi.coco_secret,	"CocoSecret"	},
 	{},
 };
 
@@ -613,6 +614,8 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 	efi_tpm_eventlog_init();
 
+	efi_coco_secret_area_reserve();
+
 	if (mem_reserve != EFI_INVALID_TABLE_ADDR) {
 		unsigned long prsv = mem_reserve;
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 9021dd521302..e86600af5dfd 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -550,6 +550,7 @@ extern struct efi {
 	unsigned long			tpm_log;		/* TPM2 Event Log table */
 	unsigned long			tpm_final_log;		/* TPM2 Final Events Log table */
 	unsigned long			mokvar_table;		/* MOK variable config table */
+	unsigned long			coco_secret;		/* Confidential computing secret table */
 
 	efi_get_time_t			*get_time;
 	efi_set_time_t			*set_time;
@@ -1189,6 +1190,8 @@ extern int efi_tpm_final_log_size;
 
 extern unsigned long rci2_table_phys;
 
+extern int efi_coco_secret_area_reserve(void);
+
 /*
  * efi_runtime_service() function identifiers.
  * "NONE" is used by efi_recover_from_page_fault() to check if the page
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-09 19:01 [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Dov Murik
  2021-08-09 19:01 ` [PATCH 1/3] efi/libstub: Copy confidential computing secret area Dov Murik
  2021-08-09 19:01 ` [PATCH 2/3] efi: Reserve " Dov Murik
@ 2021-08-09 19:01 ` Dov Murik
  2021-08-13 13:05   ` Andrew Scull
  2021-09-02 12:59   ` Greg KH
  2021-09-02 12:57 ` [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Greg KH
  3 siblings, 2 replies; 18+ messages in thread
From: Dov Murik @ 2021-08-09 19:01 UTC (permalink / raw)
  To: linux-efi
  Cc: Dov Murik, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

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

When the module is loaded (and securityfs is mounted, typically under
/sys/kernel/security), a "coco/sev_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/sev_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>
---
 drivers/virt/Kconfig                      |   3 +
 drivers/virt/Makefile                     |   1 +
 drivers/virt/coco/sev_secret/Kconfig      |  11 +
 drivers/virt/coco/sev_secret/Makefile     |   2 +
 drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++
 5 files changed, 330 insertions(+)
 create mode 100644 drivers/virt/coco/sev_secret/Kconfig
 create mode 100644 drivers/virt/coco/sev_secret/Makefile
 create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c

diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 8061e8ef449f..6f73672f593f 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/sev_secret/Kconfig"
+
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 3e272ea60cd9..2a7d472478bd 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_AMD_SEV_SECRET)	+= coco/sev_secret/
diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig
new file mode 100644
index 000000000000..76cfb4f405e0
--- /dev/null
+++ b/drivers/virt/coco/sev_secret/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config AMD_SEV_SECRET
+	tristate "AMD SEV secret area securityfs support"
+	depends on AMD_MEM_ENCRYPT && EFI
+	select SECURITYFS
+	help
+	  This is a driver for accessing the AMD SEV secret area via
+	  securityfs.
+
+	  To compile this driver as a module, choose M here.
+	  The module will be called sev_secret.
diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile
new file mode 100644
index 000000000000..dca0ed3f8f94
--- /dev/null
+++ b/drivers/virt/coco/sev_secret/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o
diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c
new file mode 100644
index 000000000000..d9a60166b142
--- /dev/null
+++ b/drivers/virt/coco/sev_secret/sev_secret.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sev_secret module
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+/**
+ * DOC: sev_secret: Allow reading confidential computing (coco) secret area via
+ * securityfs interface.
+ *
+ * When the module is loaded (and securityfs is mounted, typically under
+ * /sys/kernel/security), a "coco/sev_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>
+
+#define SEV_SECRET_NUM_FILES 64
+
+#define EFI_SEVSECRET_TABLE_HEADER_GUID \
+	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
+
+struct sev_secret {
+	struct dentry *coco_dir;
+	struct dentry *fs_dir;
+	struct dentry *fs_files[SEV_SECRET_NUM_FILES];
+	struct linux_efi_coco_secret_area *secret_area;
+};
+
+/*
+ * Structure of the SEV 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_SEVSECRET_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 sev_secret the_sev_secret;
+
+static inline struct sev_secret *sev_secret_get(void)
+{
+	return &the_sev_secret;
+}
+
+static int sev_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(sev_secret_bin_file);
+
+static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct sev_secret *s = sev_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 */
+		memzero_explicit(e->data, secret_entry_data_len(e));
+		e->guid = NULL_GUID;
+	}
+
+	inode->i_private = NULL;
+
+	for (i = 0; i < SEV_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 sev_secret_dir_inode_operations = {
+	.lookup         = simple_lookup,
+	.unlink         = sev_secret_unlink,
+};
+
+static int sev_secret_map_area(void)
+{
+	struct sev_secret *s = sev_secret_get();
+	struct linux_efi_coco_secret_area *secret_area;
+	u32 secret_area_size;
+
+	if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) {
+		pr_err("Secret area address is not available\n");
+		return -EINVAL;
+	}
+
+	secret_area = memremap(efi.coco_secret, sizeof(*secret_area), MEMREMAP_WB);
+	if (secret_area == NULL) {
+		pr_err("Could not map secret area header\n");
+		return -ENOMEM;
+	}
+
+	secret_area_size = sizeof(*secret_area) + secret_area->size;
+	memunmap(secret_area);
+
+	secret_area = memremap(efi.coco_secret, secret_area_size, MEMREMAP_WB);
+	if (secret_area == NULL) {
+		pr_err("Could not map secret area\n");
+		return -ENOMEM;
+	}
+
+	s->secret_area = secret_area;
+	return 0;
+}
+
+static void sev_secret_securityfs_teardown(void)
+{
+	struct sev_secret *s = sev_secret_get();
+	int i;
+
+	for (i = (SEV_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 sev_secret securityfs entries\n");
+}
+
+static int sev_secret_securityfs_setup(void)
+{
+	efi_guid_t tableheader_guid = EFI_SEVSECRET_TABLE_HEADER_GUID;
+	struct sev_secret *s = sev_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("sev_secret", s->coco_dir);
+	if (IS_ERR(dent)) {
+		pr_err("Error creating SEV secret securityfs directory entry err=%ld\n",
+		       PTR_ERR(dent));
+		return PTR_ERR(dent);
+	}
+	d_inode(dent)->i_op = &sev_secret_dir_inode_operations;
+	s->fs_dir = dent;
+
+	ptr = s->secret_area->area;
+	h = (struct secret_header *)ptr;
+	if (memcmp(&h->guid, &tableheader_guid, sizeof(h->guid))) {
+		pr_err("SEV secret area does not start with correct GUID\n");
+		ret = -EINVAL;
+		goto err_cleanup;
+	}
+	if (h->len < sizeof(*h)) {
+		pr_err("SEV secret area reported length is too small\n");
+		ret = -EINVAL;
+		goto err_cleanup;
+	}
+
+	bytes_left = h->len - sizeof(*h);
+	ptr += sizeof(*h);
+	while (bytes_left >= (int)sizeof(*e) && i < SEV_SECRET_NUM_FILES) {
+		e = (struct secret_entry *)ptr;
+		if (e->len < sizeof(*e) || e->len > (unsigned int)bytes_left) {
+			pr_err("SEV 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,
+						      &sev_secret_bin_file_fops);
+			if (IS_ERR(dent)) {
+				pr_err("Error creating SEV 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 sev_secret securityfs\n", i);
+	return 0;
+
+err_cleanup:
+	sev_secret_securityfs_teardown();
+	return ret;
+}
+
+static void sev_secret_unmap_area(void)
+{
+	struct sev_secret *s = sev_secret_get();
+
+	if (s->secret_area) {
+		memunmap(s->secret_area);
+		s->secret_area = NULL;
+	}
+}
+
+static int __init sev_secret_init(void)
+{
+	int ret;
+
+	ret = sev_secret_map_area();
+	if (ret)
+		return ret;
+
+	ret = sev_secret_securityfs_setup();
+	if (ret)
+		goto err_unmap;
+
+	return ret;
+
+err_unmap:
+	sev_secret_unmap_area();
+	return ret;
+}
+
+static void __exit sev_secret_exit(void)
+{
+	sev_secret_securityfs_teardown();
+	sev_secret_unmap_area();
+}
+
+module_init(sev_secret_init);
+module_exit(sev_secret_exit);
+
+MODULE_DESCRIPTION("AMD SEV confidential computing secret area access");
+MODULE_AUTHOR("IBM");
+MODULE_LICENSE("GPL");
-- 
2.25.1


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-09 19:01 ` [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
@ 2021-08-13 13:05   ` Andrew Scull
  2021-08-16  9:56     ` Ard Biesheuvel
  2021-09-02 12:59   ` Greg KH
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Scull @ 2021-08-13 13:05 UTC (permalink / raw)
  To: Dov Murik
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
> The new sev_secret module exposes the confidential computing (coco)
> secret area via securityfs interface.
> 
> When the module is loaded (and securityfs is mounted, typically under
> /sys/kernel/security), a "coco/sev_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/sev_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.

We've also been looking into a similar secret mechanism recently in the
context of Android and protected KVM [1]. Our secrets would come from a
different source, likely described as a reserved-memory node in the DT,
but would need to be exposed to userspace in the same way as the SEV
secrets. Originally I tried using a character device, but this approach
with securityfs feels neater to me.

We're also looking to pass secrets from the bootloader to Linux, outside
of any virtualization or confidential compute context (at least a far as
I have understood the meaning of the term). Again, this feels like it
would be exposed to userspace in the same way.

It would be good to be able to share the parts that would be common. I
expect that would mean the operations for a secret file and for a
directory of secrets at a minimum. But it might also influence the paths
in securityfs; I see, looking back, that the "coco" directory was added
since the RFC but would a generalized "secret" subsystem make sense? Or
would it be preferable for each case to define their own path?

[1] -- https://lwn.net/Articles/836693/

> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
> +{
> +	struct sev_secret *s = sev_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 */
> +		memzero_explicit(e->data, secret_entry_data_len(e));

Would there be a benefit in flushing these zeros?

> +		e->guid = NULL_GUID;
> +	}
> +
> +	inode->i_private = NULL;
> +
> +	for (i = 0; i < SEV_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;
> +}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-13 13:05   ` Andrew Scull
@ 2021-08-16  9:56     ` Ard Biesheuvel
  2021-08-19 13:02       ` Andrew Scull
  0 siblings, 1 reply; 18+ messages in thread
From: Ard Biesheuvel @ 2021-08-16  9:56 UTC (permalink / raw)
  To: Andrew Scull
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, Linux Kernel Mailing List

On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote:
>
> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
> > The new sev_secret module exposes the confidential computing (coco)
> > secret area via securityfs interface.
> >
> > When the module is loaded (and securityfs is mounted, typically under
> > /sys/kernel/security), a "coco/sev_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/sev_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.
>
> We've also been looking into a similar secret mechanism recently in the
> context of Android and protected KVM [1]. Our secrets would come from a
> different source, likely described as a reserved-memory node in the DT,
> but would need to be exposed to userspace in the same way as the SEV
> secrets. Originally I tried using a character device, but this approach
> with securityfs feels neater to me.
>

Agreed. I particularly like how deleting the file wipes the secret from memory.

> We're also looking to pass secrets from the bootloader to Linux, outside
> of any virtualization or confidential compute context (at least a far as
> I have understood the meaning of the term). Again, this feels like it
> would be exposed to userspace in the same way.
>

Indeed.

> It would be good to be able to share the parts that would be common. I
> expect that would mean the operations for a secret file and for a
> directory of secrets at a minimum. But it might also influence the paths
> in securityfs; I see, looking back, that the "coco" directory was added
> since the RFC but would a generalized "secret" subsystem make sense? Or
> would it be preferable for each case to define their own path?
>

I think we should avoid 'secret', to be honest. Even if protected KVM
is not riding the SEV/TDX wave, I think confidential computing is
still an accurate description of its semantics.

> [1] -- https://lwn.net/Articles/836693/
>
> > +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
> > +{
> > +     struct sev_secret *s = sev_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 */
> > +             memzero_explicit(e->data, secret_entry_data_len(e));
>
> Would there be a benefit in flushing these zeros?
>

Do you mean cache clean+invalidate? Better to be precise here.


> > +             e->guid = NULL_GUID;
> > +     }
> > +
> > +     inode->i_private = NULL;
> > +
> > +     for (i = 0; i < SEV_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;
> > +}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-16  9:56     ` Ard Biesheuvel
@ 2021-08-19 13:02       ` Andrew Scull
  2021-08-20 18:36         ` Dov Murik
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Scull @ 2021-08-19 13:02 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, Linux Kernel Mailing List

On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote:
> >
> > On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
> > > The new sev_secret module exposes the confidential computing (coco)
> > > secret area via securityfs interface.
> > >
> > > When the module is loaded (and securityfs is mounted, typically under
> > > /sys/kernel/security), a "coco/sev_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/sev_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.
> >
> > We've also been looking into a similar secret mechanism recently in the
> > context of Android and protected KVM [1]. Our secrets would come from a
> > different source, likely described as a reserved-memory node in the DT,
> > but would need to be exposed to userspace in the same way as the SEV
> > secrets. Originally I tried using a character device, but this approach
> > with securityfs feels neater to me.
> >
>
> Agreed. I particularly like how deleting the file wipes the secret from memory.
>
> > We're also looking to pass secrets from the bootloader to Linux, outside
> > of any virtualization or confidential compute context (at least a far as
> > I have understood the meaning of the term). Again, this feels like it
> > would be exposed to userspace in the same way.
> >
>
> Indeed.
>
> > It would be good to be able to share the parts that would be common. I
> > expect that would mean the operations for a secret file and for a
> > directory of secrets at a minimum. But it might also influence the paths
> > in securityfs; I see, looking back, that the "coco" directory was added
> > since the RFC but would a generalized "secret" subsystem make sense? Or
> > would it be preferable for each case to define their own path?
> >
>
> I think we should avoid 'secret', to be honest. Even if protected KVM
> is not riding the SEV/TDX wave, I think confidential computing is
> still an accurate description of its semantics.

I agree that protected KVM fits with the ideas of confidential
computing. It was the non-virtualization context that I was less
certain about. For example, the Open Profile for DICE [2] starts with
a hardware secret and derives, at each boot stage, a secret that is
passed to the next stage. It's a process that applies both to a VM,
matching confidential compute as I understand it, but also the host
Linux, which is the part that I wasn't so clear on.

[2] -- https://pigweed.googlesource.com/open-dice/+/refs/heads/main/docs/specification.md

> > [1] -- https://lwn.net/Articles/836693/
> >
> > > +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
> > > +{
> > > +     struct sev_secret *s = sev_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 */
> > > +             memzero_explicit(e->data, secret_entry_data_len(e));
> >
> > Would there be a benefit in flushing these zeros?
> >
>
> Do you mean cache clean+invalidate? Better to be precise here.

At least a clean, to have the zeros written back to memory from the
cache, in order to overwrite the secret.

>
> > > +             e->guid = NULL_GUID;
> > > +     }
> > > +
> > > +     inode->i_private = NULL;
> > > +
> > > +     for (i = 0; i < SEV_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;
> > > +}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-19 13:02       ` Andrew Scull
@ 2021-08-20 18:36         ` Dov Murik
  2021-08-23 19:21           ` Andrew Scull
  0 siblings, 1 reply; 18+ messages in thread
From: Dov Murik @ 2021-08-20 18:36 UTC (permalink / raw)
  To: Andrew Scull, Ard Biesheuvel
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, James Morris, Serge E. Hallyn, Andi Kleen,
	Dr. David Alan Gilbert, James Bottomley, Tobin Feldman-Fitzthum,
	Jim Cadden, linux-coco, linux-security-module,
	Linux Kernel Mailing List, Dov Murik



On 19/08/2021 16:02, Andrew Scull wrote:
> On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote:
>>>
>>> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:

[...]

>>>
>>>> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
>>>> +{
>>>> +     struct sev_secret *s = sev_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 */
>>>> +             memzero_explicit(e->data, secret_entry_data_len(e));
>>>
>>> Would there be a benefit in flushing these zeros?
>>>
>>
>> Do you mean cache clean+invalidate? Better to be precise here.
> 
> At least a clean, to have the zeros written back to memory from the
> cache, in order to overwrite the secret.
> 

I agree, but not sure how to implement this:

I see there's an arch_wb_cache_pmem exported function which internally
(in arch/x86/lib/usercopy_64.c) calls clean_cache_range which seems to
do what we want (assume the secret can be longer than the cache line).

But arch_wb_cache_pmem is declared in include/linux/libnvdimm.h and
guarded with #ifdef CONFIG_ARCH_HAS_PMEM_API -- both seem not related to
what I'm trying to do.

I see there's an exported clflush_cache_range for x86 -- but that's a
clean+flush if I understand correctly.

Suggestions on how to approach? I can copy the clean_cache_range
implementation into the sev_secret module but hopefully there's a better
way to reuse.  Maybe export clean_cache_range in x86?

Since this is for SEV the solution can be x86-specific, but if there's a
generic way I guess it's better (I think all of sev_secret module
doesn't have x86-specific stuff).

-Dov


>>
>>>> +             e->guid = NULL_GUID;
>>>> +     }
>>>> +
>>>> +     inode->i_private = NULL;
>>>> +
>>>> +     for (i = 0; i < SEV_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;
>>>> +}

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-20 18:36         ` Dov Murik
@ 2021-08-23 19:21           ` Andrew Scull
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Scull @ 2021-08-23 19:21 UTC (permalink / raw)
  To: Dov Murik
  Cc: Ard Biesheuvel, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, Linux Kernel Mailing List

On Fri, 20 Aug 2021 at 19:36, Dov Murik <dovmurik@linux.ibm.com> wrote:
>
>
>
> On 19/08/2021 16:02, Andrew Scull wrote:
> > On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote:
> >>>
> >>> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
>
> [...]
>
> >>>
> >>>> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
> >>>> +{
> >>>> +     struct sev_secret *s = sev_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 */
> >>>> +             memzero_explicit(e->data, secret_entry_data_len(e));
> >>>
> >>> Would there be a benefit in flushing these zeros?
> >>>
> >>
> >> Do you mean cache clean+invalidate? Better to be precise here.
> >
> > At least a clean, to have the zeros written back to memory from the
> > cache, in order to overwrite the secret.
> >
>
> I agree, but not sure how to implement this:
>
> I see there's an arch_wb_cache_pmem exported function which internally
> (in arch/x86/lib/usercopy_64.c) calls clean_cache_range which seems to
> do what we want (assume the secret can be longer than the cache line).
>
> But arch_wb_cache_pmem is declared in include/linux/libnvdimm.h and
> guarded with #ifdef CONFIG_ARCH_HAS_PMEM_API -- both seem not related to
> what I'm trying to do.
>
> I see there's an exported clflush_cache_range for x86 -- but that's a
> clean+flush if I understand correctly.

This would be perfectly correct, the invalidation is just unnecessary.

> Suggestions on how to approach? I can copy the clean_cache_range
> implementation into the sev_secret module but hopefully there's a better
> way to reuse.  Maybe export clean_cache_range in x86?

Exporting sounds much better than duplicating.

It looks like the clean-only instruction was added to x86 more
recently and with persistent memory as the intended application.

d9dc64f30 "x86/asm: Add support for the CLWB instruction" says:

"This should be used in favor of clflushopt or clflush in cases where
you require the cache line to be written to memory but plan to access
the data cache line to be written to memory but plan to access the
data"

I don't expect the secret table would be accessed with such frequency
that it would actually make a difference, but if it's just a quirk of
history that the clean-only version isn't exported, now seems as good
a time as any to change that!

> Since this is for SEV the solution can be x86-specific, but if there's a
> generic way I guess it's better (I think all of sev_secret module
> doesn't have x86-specific stuff).

arch_wb_cache_pmem is the closest to arch agnostic I've seen, but that
has it own problems :/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-08-09 19:01 [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Dov Murik
                   ` (2 preceding siblings ...)
  2021-08-09 19:01 ` [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
@ 2021-09-02 12:57 ` Greg KH
  2021-09-02 14:35   ` James Bottomley
  3 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2021-09-02 12:57 UTC (permalink / raw)
  To: Dov Murik
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Mon, Aug 09, 2021 at 07:01:54PM +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 copies the secrets from the EFI-provided memory to
> kernel reserved memory, and optionally exposes them to userspace via
> securityfs using a new sev_secret kernel module.
> 
> The first patch in efi/libstub copies the secret area from the EFI
> memory to specially allocated memory; the second patch reserves that
> memory block; and the third patch introduces the new sev_secret module
> that exposes the content of the secret entries as securityfs files, and
> allows clearing out secrets with a file unlink interface.
> 
> As a usage example, consider a guest performing computations on
> encrypted files.  The Guest Owner provides the decryption key (= secret)
> using the secret injection mechanism.  The guest application reads the
> secret from the sev_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 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.
> 
> Here is a simple example for usage of the sev_secret module in a guest
> to which a secret are with 4 secrets was injected during launch:
> 
> # modprobe sev_secret
> # ls -la /sys/kernel/security/coco/sev_secret


Wait, why are you using securityfs for this?

securityfs is for LSMs to use.  If you want your own filesystem to play
around with stuff like this, great, write your own, it's only 200 lines
or less these days.  We used to do it all the time until people realized
they should just use sysfs for driver stuff.

But this isn't a driver, so sure, add your own virtual filesystem, mount
it somewhere and away you go, no messing around with securityfs, right?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-08-09 19:01 ` [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
  2021-08-13 13:05   ` Andrew Scull
@ 2021-09-02 12:59   ` Greg KH
  2021-09-02 18:14     ` Dov Murik
  1 sibling, 1 reply; 18+ messages in thread
From: Greg KH @ 2021-09-02 12:59 UTC (permalink / raw)
  To: Dov Murik
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
> The new sev_secret module exposes the confidential computing (coco)
> secret area via securityfs interface.
> 
> When the module is loaded (and securityfs is mounted, typically under
> /sys/kernel/security), a "coco/sev_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/sev_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>
> ---
>  drivers/virt/Kconfig                      |   3 +
>  drivers/virt/Makefile                     |   1 +
>  drivers/virt/coco/sev_secret/Kconfig      |  11 +
>  drivers/virt/coco/sev_secret/Makefile     |   2 +
>  drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++
>  5 files changed, 330 insertions(+)
>  create mode 100644 drivers/virt/coco/sev_secret/Kconfig
>  create mode 100644 drivers/virt/coco/sev_secret/Makefile
>  create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c
> 
> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
> index 8061e8ef449f..6f73672f593f 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/sev_secret/Kconfig"
> +
>  endif
> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
> index 3e272ea60cd9..2a7d472478bd 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_AMD_SEV_SECRET)	+= coco/sev_secret/
> diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig
> new file mode 100644
> index 000000000000..76cfb4f405e0
> --- /dev/null
> +++ b/drivers/virt/coco/sev_secret/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config AMD_SEV_SECRET
> +	tristate "AMD SEV secret area securityfs support"
> +	depends on AMD_MEM_ENCRYPT && EFI
> +	select SECURITYFS
> +	help
> +	  This is a driver for accessing the AMD SEV secret area via
> +	  securityfs.
> +
> +	  To compile this driver as a module, choose M here.
> +	  The module will be called sev_secret.
> diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile
> new file mode 100644
> index 000000000000..dca0ed3f8f94
> --- /dev/null
> +++ b/drivers/virt/coco/sev_secret/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o
> diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c
> new file mode 100644
> index 000000000000..d9a60166b142
> --- /dev/null
> +++ b/drivers/virt/coco/sev_secret/sev_secret.c
> @@ -0,0 +1,313 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * sev_secret module
> + *
> + * Copyright (C) 2021 IBM Corporation
> + * Author: Dov Murik <dovmurik@linux.ibm.com>
> + */
> +
> +/**
> + * DOC: sev_secret: Allow reading confidential computing (coco) secret area via
> + * securityfs interface.
> + *
> + * When the module is loaded (and securityfs is mounted, typically under
> + * /sys/kernel/security), a "coco/sev_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>
> +
> +#define SEV_SECRET_NUM_FILES 64
> +
> +#define EFI_SEVSECRET_TABLE_HEADER_GUID \
> +	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
> +
> +struct sev_secret {
> +	struct dentry *coco_dir;
> +	struct dentry *fs_dir;
> +	struct dentry *fs_files[SEV_SECRET_NUM_FILES];
> +	struct linux_efi_coco_secret_area *secret_area;
> +};
> +
> +/*
> + * Structure of the SEV 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)

Why isn't all of this documented in Documentation/ABI/ which is needed
for any new user/kernel api that you come up with like this.  We have to
have it documented somewhere, otherwise how will you know how to use
these files?

thanks

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 12:57 ` [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Greg KH
@ 2021-09-02 14:35   ` James Bottomley
  2021-09-02 15:05     ` Greg KH
  0 siblings, 1 reply; 18+ messages in thread
From: James Bottomley @ 2021-09-02 14:35 UTC (permalink / raw)
  To: Greg KH, Dov Murik
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, Tobin Feldman-Fitzthum,
	Jim Cadden, linux-coco, linux-security-module, linux-kernel

On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
[...]
> Wait, why are you using securityfs for this?
> 
> securityfs is for LSMs to use. 

No it isn't ... at least not exclusively; we use it for non LSM
security purposes as well, like for the TPM BIOS log and for IMA.  What
makes you think we should start restricting securityfs to LSMs only? 
That's not been the policy up to now.
 
>  If you want your own filesystem to play around with stuff like this,
> great, write your own, it's only 200 lines or less these days.  We
> used to do it all the time until people realized they should just use
> sysfs for driver stuff.

This is a security purpose (injected key retrieval), so securityfs
seems to be the best choice.  It's certainly possible to create a new
filesystem, but I really think things with a security purpose should
use securityfs so people know where to look for them.

James


> But this isn't a driver, so sure, add your own virtual filesystem,
> mount it somewhere and away you go, no messing around with
> securityfs, right?
> 
> thanks,
> 
> greg k-h



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 14:35   ` James Bottomley
@ 2021-09-02 15:05     ` Greg KH
  2021-09-02 15:19       ` James Bottomley
  0 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2021-09-02 15:05 UTC (permalink / raw)
  To: James Bottomley
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
	Serge E. Hallyn, Andi Kleen, Dr. David Alan Gilbert,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Thu, Sep 02, 2021 at 07:35:10AM -0700, James Bottomley wrote:
> On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
> [...]
> > Wait, why are you using securityfs for this?
> > 
> > securityfs is for LSMs to use. 
> 
> No it isn't ... at least not exclusively; we use it for non LSM
> security purposes as well, like for the TPM BIOS log and for IMA.  What
> makes you think we should start restricting securityfs to LSMs only? 
> That's not been the policy up to now.

Well that was the original intent of the filesystem when it was created,
but I guess it's really up to the LSM maintainers now what they want it
for.

> >  If you want your own filesystem to play around with stuff like this,
> > great, write your own, it's only 200 lines or less these days.  We
> > used to do it all the time until people realized they should just use
> > sysfs for driver stuff.
> 
> This is a security purpose (injected key retrieval), so securityfs
> seems to be the best choice.  It's certainly possible to create a new
> filesystem, but I really think things with a security purpose should
> use securityfs so people know where to look for them.

knowing where to look should not be an issue, as that should be
documented in Documentation/ABI/ anyway, right?

It's just the overlap / overreach of using an existing filesystem for
things that don't seem to be LSM-related that feels odd to me.

Why not just make a cocofs if those people want a filesystem interface?
It's 200 lines or so these days, if not less, and that way you only
mount what you actually need for the system.

Why force this into securityfs if it doesn't have to be?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 15:05     ` Greg KH
@ 2021-09-02 15:19       ` James Bottomley
  2021-09-02 16:09         ` Greg KH
  0 siblings, 1 reply; 18+ messages in thread
From: James Bottomley @ 2021-09-02 15:19 UTC (permalink / raw)
  To: Greg KH
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
	Serge E. Hallyn, Andi Kleen, Dr. David Alan Gilbert,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Thu, 2021-09-02 at 17:05 +0200, Greg KH wrote:
> On Thu, Sep 02, 2021 at 07:35:10AM -0700, James Bottomley wrote:
> > On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
> > [...]
> > > Wait, why are you using securityfs for this?
> > > 
> > > securityfs is for LSMs to use. 
> > 
> > No it isn't ... at least not exclusively; we use it for non LSM
> > security purposes as well, like for the TPM BIOS log and for
> > IMA.  What makes you think we should start restricting securityfs
> > to LSMs only?  That's not been the policy up to now.
> 
> Well that was the original intent of the filesystem when it was
> created, but I guess it's really up to the LSM maintainers now what
> they want it for.
> 
> > >  If you want your own filesystem to play around with stuff like
> > > this, great, write your own, it's only 200 lines or less these
> > > days.  We used to do it all the time until people realized they
> > > should just use sysfs for driver stuff.
> > 
> > This is a security purpose (injected key retrieval), so securityfs
> > seems to be the best choice.  It's certainly possible to create a
> > new filesystem, but I really think things with a security purpose
> > should use securityfs so people know where to look for them.
> 
> knowing where to look should not be an issue, as that should be
> documented in Documentation/ABI/ anyway, right?
> 
> It's just the overlap / overreach of using an existing filesystem for
> things that don't seem to be LSM-related that feels odd to me.
> 
> Why not just make a cocofs if those people want a filesystem
> interface?
> It's 200 lines or so these days, if not less, and that way you only
> mount what you actually need for the system.

Secrets transfer is actually broader than confidential computing,
although confidential computing is a first proposed use, so I think
cocofs would be too narrow.

> Why force this into securityfs if it doesn't have to be?

It's not being forced.  Secrets transfer is a security function in the
same way the bios log is.

James



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 15:19       ` James Bottomley
@ 2021-09-02 16:09         ` Greg KH
  2021-09-02 16:19           ` James Bottomley
  0 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2021-09-02 16:09 UTC (permalink / raw)
  To: James Bottomley
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
	Serge E. Hallyn, Andi Kleen, Dr. David Alan Gilbert,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Thu, Sep 02, 2021 at 08:19:51AM -0700, James Bottomley wrote:
> On Thu, 2021-09-02 at 17:05 +0200, Greg KH wrote:
> > On Thu, Sep 02, 2021 at 07:35:10AM -0700, James Bottomley wrote:
> > > On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
> > > [...]
> > > > Wait, why are you using securityfs for this?
> > > > 
> > > > securityfs is for LSMs to use. 
> > > 
> > > No it isn't ... at least not exclusively; we use it for non LSM
> > > security purposes as well, like for the TPM BIOS log and for
> > > IMA.  What makes you think we should start restricting securityfs
> > > to LSMs only?  That's not been the policy up to now.
> > 
> > Well that was the original intent of the filesystem when it was
> > created, but I guess it's really up to the LSM maintainers now what
> > they want it for.
> > 
> > > >  If you want your own filesystem to play around with stuff like
> > > > this, great, write your own, it's only 200 lines or less these
> > > > days.  We used to do it all the time until people realized they
> > > > should just use sysfs for driver stuff.
> > > 
> > > This is a security purpose (injected key retrieval), so securityfs
> > > seems to be the best choice.  It's certainly possible to create a
> > > new filesystem, but I really think things with a security purpose
> > > should use securityfs so people know where to look for them.
> > 
> > knowing where to look should not be an issue, as that should be
> > documented in Documentation/ABI/ anyway, right?
> > 
> > It's just the overlap / overreach of using an existing filesystem for
> > things that don't seem to be LSM-related that feels odd to me.
> > 
> > Why not just make a cocofs if those people want a filesystem
> > interface?
> > It's 200 lines or so these days, if not less, and that way you only
> > mount what you actually need for the system.
> 
> Secrets transfer is actually broader than confidential computing,
> although confidential computing is a first proposed use, so I think
> cocofs would be too narrow.
> 
> > Why force this into securityfs if it doesn't have to be?
> 
> It's not being forced.  Secrets transfer is a security function in the
> same way the bios log is.

Is the bios log in securityfs today?

Anyway, it's up to the securityfs maintainer (i.e. not me), but
personally, I think this should be a separate filesystem as that would
probably make things easier in the long run...

good luck!

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 16:09         ` Greg KH
@ 2021-09-02 16:19           ` James Bottomley
  2021-09-02 16:31             ` Greg KH
  0 siblings, 1 reply; 18+ messages in thread
From: James Bottomley @ 2021-09-02 16:19 UTC (permalink / raw)
  To: Greg KH
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
	Serge E. Hallyn, Andi Kleen, Dr. David Alan Gilbert,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Thu, 2021-09-02 at 18:09 +0200, Greg KH wrote:
> On Thu, Sep 02, 2021 at 08:19:51AM -0700, James Bottomley wrote:
> > On Thu, 2021-09-02 at 17:05 +0200, Greg KH wrote:
> > > On Thu, Sep 02, 2021 at 07:35:10AM -0700, James Bottomley wrote:
> > > > On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
> > > > [...]
> > > > > Wait, why are you using securityfs for this?
> > > > > 
> > > > > securityfs is for LSMs to use. 
> > > > 
> > > > No it isn't ... at least not exclusively; we use it for non LSM
> > > > security purposes as well, like for the TPM BIOS log and for
> > > > IMA.  What makes you think we should start restricting
> > > > securityfs to LSMs only?  That's not been the policy up to now.
> > > 
> > > Well that was the original intent of the filesystem when it was
> > > created, but I guess it's really up to the LSM maintainers now
> > > what they want it for.
> > > 
> > > > >  If you want your own filesystem to play around with stuff
> > > > > like this, great, write your own, it's only 200 lines or less
> > > > > these days.  We used to do it all the time until people
> > > > > realized they should just use sysfs for driver stuff.
> > > > 
> > > > This is a security purpose (injected key retrieval), so
> > > > securityfs seems to be the best choice.  It's certainly
> > > > possible to create a new filesystem, but I really think things
> > > > with a security purpose should use securityfs so people know
> > > > where to look for them.
> > > 
> > > knowing where to look should not be an issue, as that should be
> > > documented in Documentation/ABI/ anyway, right?
> > > 
> > > It's just the overlap / overreach of using an existing filesystem
> > > for things that don't seem to be LSM-related that feels odd to
> > > me.
> > > 
> > > Why not just make a cocofs if those people want a filesystem
> > > interface?
> > > It's 200 lines or so these days, if not less, and that way you
> > > only mount what you actually need for the system.
> > 
> > Secrets transfer is actually broader than confidential computing,
> > although confidential computing is a first proposed use, so I think
> > cocofs would be too narrow.
> > 
> > > Why force this into securityfs if it doesn't have to be?
> > 
> > It's not being forced.  Secrets transfer is a security function in
> > the same way the bios log is.
> 
> Is the bios log in securityfs today?

Yes. It's under /sys/kernel/security/tpm0/  All the ima policy control
and its log is under /sys/kernel/security/ima/  that's why I think
declaring securityfs as being for anything security related is already
our de facto (if not de jure) policy.

> Anyway, it's up to the securityfs maintainer (i.e. not me), but
> personally, I think this should be a separate filesystem as that
> would probably make things easier in the long run...

I know Al likes this business of loads of separate filesystems, but
personally I'm not in favour.  For every one you do, you not only have
to document it all, you also have to find a preferred mount point that
the distributions can agree on and also have them agree to enable the
mount for, which often takes months of negotiation.  Having fewer
filesystems grouped by common purpose which have agreed mount points
that distros actually mount seems a far easier approach to enablement.

James



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 0/3] Allow access to confidential computing secret area in SEV guests
  2021-09-02 16:19           ` James Bottomley
@ 2021-09-02 16:31             ` Greg KH
  0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2021-09-02 16:31 UTC (permalink / raw)
  To: James Bottomley
  Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
	Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
	Serge E. Hallyn, Andi Kleen, Dr. David Alan Gilbert,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel

On Thu, Sep 02, 2021 at 09:19:13AM -0700, James Bottomley wrote:
> On Thu, 2021-09-02 at 18:09 +0200, Greg KH wrote:
> > On Thu, Sep 02, 2021 at 08:19:51AM -0700, James Bottomley wrote:
> > > On Thu, 2021-09-02 at 17:05 +0200, Greg KH wrote:
> > > > On Thu, Sep 02, 2021 at 07:35:10AM -0700, James Bottomley wrote:
> > > > > On Thu, 2021-09-02 at 14:57 +0200, Greg KH wrote:
> > > > > [...]
> > > > > > Wait, why are you using securityfs for this?
> > > > > > 
> > > > > > securityfs is for LSMs to use. 
> > > > > 
> > > > > No it isn't ... at least not exclusively; we use it for non LSM
> > > > > security purposes as well, like for the TPM BIOS log and for
> > > > > IMA.  What makes you think we should start restricting
> > > > > securityfs to LSMs only?  That's not been the policy up to now.
> > > > 
> > > > Well that was the original intent of the filesystem when it was
> > > > created, but I guess it's really up to the LSM maintainers now
> > > > what they want it for.
> > > > 
> > > > > >  If you want your own filesystem to play around with stuff
> > > > > > like this, great, write your own, it's only 200 lines or less
> > > > > > these days.  We used to do it all the time until people
> > > > > > realized they should just use sysfs for driver stuff.
> > > > > 
> > > > > This is a security purpose (injected key retrieval), so
> > > > > securityfs seems to be the best choice.  It's certainly
> > > > > possible to create a new filesystem, but I really think things
> > > > > with a security purpose should use securityfs so people know
> > > > > where to look for them.
> > > > 
> > > > knowing where to look should not be an issue, as that should be
> > > > documented in Documentation/ABI/ anyway, right?
> > > > 
> > > > It's just the overlap / overreach of using an existing filesystem
> > > > for things that don't seem to be LSM-related that feels odd to
> > > > me.
> > > > 
> > > > Why not just make a cocofs if those people want a filesystem
> > > > interface?
> > > > It's 200 lines or so these days, if not less, and that way you
> > > > only mount what you actually need for the system.
> > > 
> > > Secrets transfer is actually broader than confidential computing,
> > > although confidential computing is a first proposed use, so I think
> > > cocofs would be too narrow.
> > > 
> > > > Why force this into securityfs if it doesn't have to be?
> > > 
> > > It's not being forced.  Secrets transfer is a security function in
> > > the same way the bios log is.
> > 
> > Is the bios log in securityfs today?
> 
> Yes. It's under /sys/kernel/security/tpm0/  All the ima policy control
> and its log is under /sys/kernel/security/ima/  that's why I think
> declaring securityfs as being for anything security related is already
> our de facto (if not de jure) policy.
> 
> > Anyway, it's up to the securityfs maintainer (i.e. not me), but
> > personally, I think this should be a separate filesystem as that
> > would probably make things easier in the long run...
> 
> I know Al likes this business of loads of separate filesystems, but
> personally I'm not in favour.  For every one you do, you not only have
> to document it all,

Wait, why would you not have to document your new files no matter what?
That should not be an issue either way.

> you also have to find a preferred mount point that
> the distributions can agree on and also have them agree to enable the
> mount for,

You create that yourself, just like tracefs does, and set the standard
right away, not an issue.

> which often takes months of negotiation.

Enabling it does take time, which is good because if they do not think
it should be present because they do not want to use it, then it will
not be, which means either they do not need your new feature, or you
have not made it useful enough.

So again, not an issue.
And you can even mount it yourself from the kernel if you insist on it
always being present.

> Having fewer
> filesystems grouped by common purpose which have agreed mount points
> that distros actually mount seems a far easier approach to enablement.

The issue is that random things gets added to those filesystems,
exposing stuff that perhaps some systems do NOT want exposed to
userspace.  Making it explicit as to what they have to mount to get
access to that is a good thing because you have less of an "attack
surface" and all of that.

So again, this should not be an issue.  If coco stuff is so important
that people need it, then having them have to add it to their init
scripts just to mount the filesystem is not an issue as there are other
userspace components of all of this mess that they had to install
anyway.  Just make it part of the userspace tools that are going to be
accessing these files because you have to get those onto the systems no
matter what.

greg k-h

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets
  2021-09-02 12:59   ` Greg KH
@ 2021-09-02 18:14     ` Dov Murik
  0 siblings, 0 replies; 18+ messages in thread
From: Dov Murik @ 2021-09-02 18:14 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
	Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
	Andi Kleen, Dr. David Alan Gilbert, James Bottomley,
	Tobin Feldman-Fitzthum, Jim Cadden, linux-coco,
	linux-security-module, linux-kernel, Dov Murik



On 02/09/2021 15:59, Greg KH wrote:
> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote:
>> The new sev_secret module exposes the confidential computing (coco)
>> secret area via securityfs interface.
>>
>> When the module is loaded (and securityfs is mounted, typically under
>> /sys/kernel/security), a "coco/sev_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/sev_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>
>> ---
>>  drivers/virt/Kconfig                      |   3 +
>>  drivers/virt/Makefile                     |   1 +
>>  drivers/virt/coco/sev_secret/Kconfig      |  11 +
>>  drivers/virt/coco/sev_secret/Makefile     |   2 +
>>  drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++
>>  5 files changed, 330 insertions(+)
>>  create mode 100644 drivers/virt/coco/sev_secret/Kconfig
>>  create mode 100644 drivers/virt/coco/sev_secret/Makefile
>>  create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c
>>
>> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
>> index 8061e8ef449f..6f73672f593f 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/sev_secret/Kconfig"
>> +
>>  endif
>> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
>> index 3e272ea60cd9..2a7d472478bd 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_AMD_SEV_SECRET)	+= coco/sev_secret/
>> diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig
>> new file mode 100644
>> index 000000000000..76cfb4f405e0
>> --- /dev/null
>> +++ b/drivers/virt/coco/sev_secret/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config AMD_SEV_SECRET
>> +	tristate "AMD SEV secret area securityfs support"
>> +	depends on AMD_MEM_ENCRYPT && EFI
>> +	select SECURITYFS
>> +	help
>> +	  This is a driver for accessing the AMD SEV secret area via
>> +	  securityfs.
>> +
>> +	  To compile this driver as a module, choose M here.
>> +	  The module will be called sev_secret.
>> diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile
>> new file mode 100644
>> index 000000000000..dca0ed3f8f94
>> --- /dev/null
>> +++ b/drivers/virt/coco/sev_secret/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o
>> diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c
>> new file mode 100644
>> index 000000000000..d9a60166b142
>> --- /dev/null
>> +++ b/drivers/virt/coco/sev_secret/sev_secret.c
>> @@ -0,0 +1,313 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * sev_secret module
>> + *
>> + * Copyright (C) 2021 IBM Corporation
>> + * Author: Dov Murik <dovmurik@linux.ibm.com>
>> + */
>> +
>> +/**
>> + * DOC: sev_secret: Allow reading confidential computing (coco) secret area via
>> + * securityfs interface.
>> + *
>> + * When the module is loaded (and securityfs is mounted, typically under
>> + * /sys/kernel/security), a "coco/sev_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>
>> +
>> +#define SEV_SECRET_NUM_FILES 64
>> +
>> +#define EFI_SEVSECRET_TABLE_HEADER_GUID \
>> +	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
>> +
>> +struct sev_secret {
>> +	struct dentry *coco_dir;
>> +	struct dentry *fs_dir;
>> +	struct dentry *fs_files[SEV_SECRET_NUM_FILES];
>> +	struct linux_efi_coco_secret_area *secret_area;
>> +};
>> +
>> +/*
>> + * Structure of the SEV 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)
> 
> Why isn't all of this documented in Documentation/ABI/ which is needed
> for any new user/kernel api that you come up with like this.  We have to
> have it documented somewhere, otherwise how will you know how to use
> these files?

Yes, you're right, I'll add such documentation.

Note that the ABI (for userspace programs) is the filesystem paths and
usage (read + unlink), and not the GUIDed table explained above your
comment.  That GUIDed table is passed from the Guest Owner via SEV
secret injection into OVMF and from there to the kernel memory (patches
1+2 in this series).  So userspace doesn't see this GUIDed table
structure at all.

I should probably add this story to this file's header comment, or some
other place which will document this module (suggestions welcome).

-Dov

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2021-09-02 18:14 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-09 19:01 [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Dov Murik
2021-08-09 19:01 ` [PATCH 1/3] efi/libstub: Copy confidential computing secret area Dov Murik
2021-08-09 19:01 ` [PATCH 2/3] efi: Reserve " Dov Murik
2021-08-09 19:01 ` [PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
2021-08-13 13:05   ` Andrew Scull
2021-08-16  9:56     ` Ard Biesheuvel
2021-08-19 13:02       ` Andrew Scull
2021-08-20 18:36         ` Dov Murik
2021-08-23 19:21           ` Andrew Scull
2021-09-02 12:59   ` Greg KH
2021-09-02 18:14     ` Dov Murik
2021-09-02 12:57 ` [PATCH 0/3] Allow access to confidential computing secret area in SEV guests Greg KH
2021-09-02 14:35   ` James Bottomley
2021-09-02 15:05     ` Greg KH
2021-09-02 15:19       ` James Bottomley
2021-09-02 16:09         ` Greg KH
2021-09-02 16:19           ` James Bottomley
2021-09-02 16:31             ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).