[RFC PATCH 0/3] Allow access to confidential computing secret area

[RFC PATCH 0/3] Allow access to confidential computing secret area

[Dov Murik]

Confidential computing 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.

Support for secret injection is already available in OVMF (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.

This has been tested with AMD SEV guests, but the kernel side of
handling the secret area has no SEV-specific dependencies, and therefore
should be usable 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
secrets were injected during launch:

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

# xxd /sys/kernel/security/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                                     ..


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


Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-efi@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

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

 drivers/firmware/efi/Makefile                 |   2 +-
 drivers/firmware/efi/confidential-computing.c |  41 +++
 drivers/firmware/efi/efi.c                    |   5 +
 drivers/firmware/efi/libstub/Makefile         |   3 +-
 .../efi/libstub/confidential-computing.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                          |   2 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/sev_secret/Kconfig               |  11 +
 drivers/virt/sev_secret/Makefile              |   2 +
 drivers/virt/sev_secret/sev_secret.c          | 260 ++++++++++++++++++
 include/linux/efi.h                           |  11 +
 14 files changed, 410 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/efi/confidential-computing.c
 create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c
 create mode 100644 drivers/virt/sev_secret/Kconfig
 create mode 100644 drivers/virt/sev_secret/Makefile
 create mode 100644 drivers/virt/sev_secret/sev_secret.c


base-commit: c06a2ba62fc401b7aaefd23f5d0bc06d2457ccc1
-- 
2.25.1

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

[Dov Murik]

Confidential computing 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_CONFIDENTIAL_COMPUTING_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_CONFIDENTIAL_COMPUTING_SECRET_AREA_GUID.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/libstub/Makefile         |  3 +-
 .../efi/libstub/confidential-computing.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                           |  7 ++
 6 files changed, 83 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c

diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d0537573501e..938ed23dd135 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -55,7 +55,8 @@ 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 \
+				   confidential-computing.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/confidential-computing.c b/drivers/firmware/efi/libstub/confidential-computing.c
new file mode 100644
index 000000000000..96b97ff5d503
--- /dev/null
+++ b/drivers/firmware/efi/libstub/confidential-computing.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing 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_CONFIDENTIAL_COMPUTING_SECRET_TABLE_GUID                                         \
+	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
+
+/**
+ * struct efi_confidential_computing_secret_table - EFI config table that
+ * points to the confidential computing secret area. The guid
+ * LINUX_EFI_CONFIDENTIAL_COMPUTING_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_confidential_computing_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_confidential_computing_secret_area(void)
+{
+	efi_guid_t linux_secret_area_guid = LINUX_EFI_CONFIDENTIAL_COMPUTING_SECRET_AREA_GUID;
+	efi_status_t status;
+	struct efi_confidential_computing_secret_table *secret_table;
+	struct linux_efi_confidential_computing_secret_area *secret_area;
+
+	secret_table = get_efi_config_table(LINUX_EFI_CONFIDENTIAL_COMPUTING_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..56bcd94a387e 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_confidential_computing_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..1c02658042ea 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_confidential_computing_secret_area(void);
+
 #endif
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..c663bf47370a 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_confidential_computing_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..4f647f1ee298 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -359,6 +359,8 @@ 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_CONFIDENTIAL_COMPUTING_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 +1284,9 @@ static inline struct efi_mokvar_table_entry *efi_mokvar_entry_find(
 }
 #endif
 
+struct linux_efi_confidential_computing_secret_area {
+	u32	size;
+	u8	area[];
+};
+
 #endif /* _LINUX_EFI_H */
-- 
2.25.1

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

[Dov Murik]

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

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-efi@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/Makefile                 |  2 +-
 drivers/firmware/efi/confidential-computing.c | 41 +++++++++++++++++++
 drivers/firmware/efi/efi.c                    |  5 +++
 include/linux/efi.h                           |  4 ++
 4 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/efi/confidential-computing.c

diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 467e94259679..63f21f7351da 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 confidential-computing.o
 ifneq ($(CONFIG_EFI_CAPSULE_LOADER),)
 obj-$(CONFIG_EFI)			+= capsule.o
 endif
diff --git a/drivers/firmware/efi/confidential-computing.c b/drivers/firmware/efi/confidential-computing.c
new file mode 100644
index 000000000000..e6bb4d1e8f17
--- /dev/null
+++ b/drivers/firmware/efi/confidential-computing.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing 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_confidential_computing_secret_area_reserve(void)
+{
+	struct linux_efi_confidential_computing_secret_area *secret_area;
+	unsigned long secret_area_size;
+
+	if (efi.confidential_computing_secret == EFI_INVALID_TABLE_ADDR)
+		return 0;
+
+	secret_area = early_memremap(efi.confidential_computing_secret, sizeof(*secret_area));
+	if (!secret_area) {
+		pr_err("Failed to map confidential computing secret area\n");
+		efi.confidential_computing_secret = EFI_INVALID_TABLE_ADDR;
+		return -ENOMEM;
+	}
+
+	secret_area_size = sizeof(*secret_area) + secret_area->size;
+	memblock_reserve(efi.confidential_computing_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 4b7ee3fa9224..da36333e5c9f 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -526,6 +526,9 @@ 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_CONFIDENTIAL_COMPUTING_SECRET_AREA_GUID,
+						&efi.confidential_computing_secret,
+									"ConfCompSecret"},
 	{},
 };
 
@@ -613,6 +616,8 @@ int __init efi_config_parse_tables(const efi_config_table_t *config_tables,
 
 	efi_tpm_eventlog_init();
 
+	efi_confidential_computing_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 4f647f1ee298..e9740bd16db0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -551,6 +551,8 @@ 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			confidential_computing_secret;	/* Confidential computing */
+									/* secret table           */
 
 	efi_get_time_t			*get_time;
 	efi_set_time_t			*set_time;
@@ -1190,6 +1192,8 @@ extern int efi_tpm_final_log_size;
 
 extern unsigned long rci2_table_phys;
 
+extern int efi_confidential_computing_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

[RFC PATCH 3/3] virt: Add sev_secret module to expose confidential computing secrets

[Dov Murik]

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

When the module is loaded (and securityfs is mounted, typically under
/sys/kernel/security), an "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).

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ashish Kalra <ashish.kalra@amd.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: James Morris <jmorris@namei.org>
Cc: "Serge E. Hallyn" <serge@hallyn.com>
Cc: linux-security-module@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/virt/Kconfig                 |   2 +
 drivers/virt/Makefile                |   1 +
 drivers/virt/sev_secret/Kconfig      |  11 ++
 drivers/virt/sev_secret/Makefile     |   2 +
 drivers/virt/sev_secret/sev_secret.c | 260 +++++++++++++++++++++++++++
 5 files changed, 276 insertions(+)
 create mode 100644 drivers/virt/sev_secret/Kconfig
 create mode 100644 drivers/virt/sev_secret/Makefile
 create mode 100644 drivers/virt/sev_secret/sev_secret.c

diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index 8061e8ef449f..c222cc625891 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -36,4 +36,6 @@ source "drivers/virt/vboxguest/Kconfig"
 source "drivers/virt/nitro_enclaves/Kconfig"
 
 source "drivers/virt/acrn/Kconfig"
+
+source "drivers/virt/sev_secret/Kconfig"
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index 3e272ea60cd9..0765e5418d1d 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-y				+= sev_secret/
diff --git a/drivers/virt/sev_secret/Kconfig b/drivers/virt/sev_secret/Kconfig
new file mode 100644
index 000000000000..4505526b8ef1
--- /dev/null
+++ b/drivers/virt/sev_secret/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config AMD_SEV_SECRET_SECURITYFS
+	tristate "AMD SEV secret area securityfs support"
+	depends on 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/sev_secret/Makefile b/drivers/virt/sev_secret/Makefile
new file mode 100644
index 000000000000..9671f7bb3941
--- /dev/null
+++ b/drivers/virt/sev_secret/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_AMD_SEV_SECRET_SECURITYFS) += sev_secret.o
diff --git a/drivers/virt/sev_secret/sev_secret.c b/drivers/virt/sev_secret/sev_secret.c
new file mode 100644
index 000000000000..9fff62356e74
--- /dev/null
+++ b/drivers/virt/sev_secret/sev_secret.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * sev_secret module
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#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>
+
+/**
+ * sev_secret: Allow reading confidential computing secret area via securityfs
+ * interface.
+ *
+ * When the module is loaded (and securityfs is mounted, typically under
+ * /sys/kernel/security), an "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.
+ */
+
+#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 *fs_dir;
+	struct dentry *fs_files[SEV_SECRET_NUM_FILES];
+	struct linux_efi_confidential_computing_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
+ * @len:	Length of secret entry, including its guid and len fields
+ * @data:	The secret data
+ */
+struct secret_entry {
+	efi_guid_t guid;
+	u32 len;
+	u8 data[];
+} __attribute((packed));
+
+static struct sev_secret the_sev_secret;
+
+static size_t secret_entry_data_len(struct secret_entry *e)
+{
+	return e->len - sizeof(*e);
+}
+
+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 inline struct sev_secret *sev_secret_get(void)
+{
+	return &the_sev_secret;
+}
+
+static int sev_secret_map_area(void)
+{
+	struct sev_secret *s = sev_secret_get();
+	struct linux_efi_confidential_computing_secret_area *secret_area;
+	u32 secret_area_size;
+
+	if (efi.confidential_computing_secret == EFI_INVALID_TABLE_ADDR) {
+		pr_err("Secret area address is not available\n");
+		return -EINVAL;
+	}
+
+	secret_area =
+		memremap(efi.confidential_computing_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.confidential_computing_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--)
+		if (s->fs_files[i])
+			securityfs_remove(s->fs_files[i]);
+
+	if (s->fs_dir)
+		securityfs_remove(s->fs_dir);
+
+	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->fs_dir = NULL;
+	memset(s->fs_files, 0, sizeof(s->fs_files));
+
+	dent = securityfs_create_dir("sev_secret", NULL);
+	if (IS_ERR(dent)) {
+		pr_err("Error creating SEV secret securityfs directory entry");
+		return PTR_ERR(dent);
+	}
+	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;
+		}
+
+		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

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Brijesh Singh]

Hi Dov,


On 5/13/21 1:26 AM, Dov Murik wrote:
> Confidential computing 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.
>
> Support for secret injection is already available in OVMF (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.
>
> This has been tested with AMD SEV guests, but the kernel side of
> handling the secret area has no SEV-specific dependencies, and therefore
> should be usable 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
> secrets were injected during launch:
>
> # modprobe sev_secret
> # ls -la /sys/kernel/security/sev_secret
> total 0
> drwxr-xr-x 2 root root 0 May 12 18:03 .
> drwxr-xr-x 3 root root 0 May 12 18:02 ..
> -r--r----- 1 root root 0 May 12 18:03 736870e5-84f0-4973-92ec-06879ce3da0b
> -r--r----- 1 root root 0 May 12 18:03 83c83f7f-1356-4975-8b7e-d3a0b54312c6
> -r--r----- 1 root root 0 May 12 18:03 9553f55d-3da2-43ee-ab5d-ff17f78864d2
> -r--r----- 1 root root 0 May 12 18:03 e6f5a162-d67f-4750-a67c-5d065f2a9910
>
> # xxd /sys/kernel/security/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                                     ..

I am adding a new virt driver to help get the attestation report for the
SEV-SNP guest. I understand they both are different, in case of the SEV
the attestation is already completed and we are simply exposing the
secret provided after the attestation to the userspace, whereas in SNP,
the userspace is querying the attestation and will probably derive keys
etc based on the attestation report. I am wondering if we should merge
both the SEV secret and SNP attestation query in a single driver ?
Should we cover usecases where SEV guest is not booted under the EFI ?
Also, it appears that this driver need to be manually loaded, should we
create a platform device so that the driver binds to platform device and
use the resource structure to find the location of the secret data?

I was trying to answer some of these questions SNP series. See these patches

https://marc.info/?l=kvm&m=161978514019741&w=2

https://marc.info/?l=kvm&m=161978514119744&w=2

https://marc.info/?l=kvm&m=161978514219751&w=2


>
> [1] https://github.com/tianocore/edk2/commit/01726b6d23d4
>
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ashish Kalra <ashish.kalra@amd.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: James Morris <jmorris@namei.org>
> Cc: "Serge E. Hallyn" <serge@hallyn.com>
> Cc: linux-efi@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
>
> 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
>
>  drivers/firmware/efi/Makefile                 |   2 +-
>  drivers/firmware/efi/confidential-computing.c |  41 +++
>  drivers/firmware/efi/efi.c                    |   5 +
>  drivers/firmware/efi/libstub/Makefile         |   3 +-
>  .../efi/libstub/confidential-computing.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                          |   2 +
>  drivers/virt/Makefile                         |   1 +
>  drivers/virt/sev_secret/Kconfig               |  11 +
>  drivers/virt/sev_secret/Makefile              |   2 +
>  drivers/virt/sev_secret/sev_secret.c          | 260 ++++++++++++++++++
>  include/linux/efi.h                           |  11 +
>  14 files changed, 410 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/firmware/efi/confidential-computing.c
>  create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c
>  create mode 100644 drivers/virt/sev_secret/Kconfig
>  create mode 100644 drivers/virt/sev_secret/Makefile
>  create mode 100644 drivers/virt/sev_secret/sev_secret.c
>
>
> base-commit: c06a2ba62fc401b7aaefd23f5d0bc06d2457ccc1

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Dov Murik]

Hi Brijesh,

On 14/05/2021 16:01, Brijesh Singh wrote:
> Hi Dov,
> 
> 
> On 5/13/21 1:26 AM, Dov Murik wrote:
>> Confidential computing 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.
>>
>> Support for secret injection is already available in OVMF (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.
>>
>> This has been tested with AMD SEV guests, but the kernel side of
>> handling the secret area has no SEV-specific dependencies, and therefore
>> should be usable 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
>> secrets were injected during launch:
>>
>> # modprobe sev_secret
>> # ls -la /sys/kernel/security/sev_secret
>> total 0
>> drwxr-xr-x 2 root root 0 May 12 18:03 .
>> drwxr-xr-x 3 root root 0 May 12 18:02 ..
>> -r--r----- 1 root root 0 May 12 18:03 736870e5-84f0-4973-92ec-06879ce3da0b
>> -r--r----- 1 root root 0 May 12 18:03 83c83f7f-1356-4975-8b7e-d3a0b54312c6
>> -r--r----- 1 root root 0 May 12 18:03 9553f55d-3da2-43ee-ab5d-ff17f78864d2
>> -r--r----- 1 root root 0 May 12 18:03 e6f5a162-d67f-4750-a67c-5d065f2a9910
>>
>> # xxd /sys/kernel/security/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                                     ..
> 
> I am adding a new virt driver to help get the attestation report for the
> SEV-SNP guest. I understand they both are different, in case of the SEV
> the attestation is already completed and we are simply exposing the
> secret provided after the attestation to the userspace, whereas in SNP,
> the userspace is querying the attestation and will probably derive keys
> etc based on the attestation report. 

I don't know a lot about SNP, but are we talking about the same secrets? SEV's LAUNCH_SECRET command allows the guest owner to inject any binary blob, though through the current OVMF implementation it is limited to one page.  These can be used for anything - encryption keys, private keys for signature, passphrases, etc. The SNP secret is used for communication with the PSP only? 

Can the two co-exist?


> I am wondering if we should merge
> both the SEV secret and SNP attestation query in a single driver ?

Generally I'm OK with this, but I want to support plain SEV as well. The current suggested snp-guest driver is only activated on SNP (if I read the code correctly).  Now the question is whether a general SEV (or confidential computing) driver makes sense? It looks like only parts of its functionality will be available depending on the actual hardware available.


> Should we cover usecases where SEV guest is not booted under the EFI ?

This sounds interesting; it might be useful for Kata were we try to minimize the overhead of the VM, so fewer moving parts is better.  So far we don't know how to boot an SEV guest without OVMF (=EFI).  Is this supported with QEMU (we currently inject SEV secrets using QEMU's sev-inject-launch-secret)?  Can you share such instructions?


> Also, it appears that this driver need to be manually loaded, should we
> create a platform device so that the driver binds to platform device and
> use the resource structure to find the location of the secret data?

I designed the sev-secret driver to be hardware-indepedent (maybe a better name would be confidential-computing-secret); it just looks for the declared secrets page and creates a userspace API to read the secrets by GUID.  In that sense, binding it to specific hardware will limit its usage with other platforms that can "prepare" the secrets in the correct place.



> 
> I was trying to answer some of these questions SNP series. See these patches
> 
> https://marc.info/?l=kvm&m=161978514019741&w=2
> 
> https://marc.info/?l=kvm&m=161978514119744&w=2
> 
> https://marc.info/?l=kvm&m=161978514219751&w=2
> 
> 
>>
>> [1] https://github.com/tianocore/edk2/commit/01726b6d23d4
>>
>>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ashish Kalra <ashish.kalra@amd.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Ard Biesheuvel <ardb@kernel.org>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: "Serge E. Hallyn" <serge@hallyn.com>
>> Cc: linux-efi@vger.kernel.org
>> Cc: linux-security-module@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>>
>> 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
>>
>>  drivers/firmware/efi/Makefile                 |   2 +-
>>  drivers/firmware/efi/confidential-computing.c |  41 +++
>>  drivers/firmware/efi/efi.c                    |   5 +
>>  drivers/firmware/efi/libstub/Makefile         |   3 +-
>>  .../efi/libstub/confidential-computing.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                          |   2 +
>>  drivers/virt/Makefile                         |   1 +
>>  drivers/virt/sev_secret/Kconfig               |  11 +
>>  drivers/virt/sev_secret/Makefile              |   2 +
>>  drivers/virt/sev_secret/sev_secret.c          | 260 ++++++++++++++++++
>>  include/linux/efi.h                           |  11 +
>>  14 files changed, 410 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/firmware/efi/confidential-computing.c
>>  create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c
>>  create mode 100644 drivers/virt/sev_secret/Kconfig
>>  create mode 100644 drivers/virt/sev_secret/Makefile
>>  create mode 100644 drivers/virt/sev_secret/sev_secret.c
>>
>>
>> base-commit: c06a2ba62fc401b7aaefd23f5d0bc06d2457ccc1

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Dr. David Alan Gilbert]

* Brijesh Singh (brijesh.singh@amd.com) wrote:
> Hi Dov,
> 
> 
> On 5/13/21 1:26 AM, Dov Murik wrote:
> > Confidential computing 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.
> >
> > Support for secret injection is already available in OVMF (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.
> >
> > This has been tested with AMD SEV guests, but the kernel side of
> > handling the secret area has no SEV-specific dependencies, and therefore
> > should be usable 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
> > secrets were injected during launch:
> >
> > # modprobe sev_secret
> > # ls -la /sys/kernel/security/sev_secret
> > total 0
> > drwxr-xr-x 2 root root 0 May 12 18:03 .
> > drwxr-xr-x 3 root root 0 May 12 18:02 ..
> > -r--r----- 1 root root 0 May 12 18:03 736870e5-84f0-4973-92ec-06879ce3da0b
> > -r--r----- 1 root root 0 May 12 18:03 83c83f7f-1356-4975-8b7e-d3a0b54312c6
> > -r--r----- 1 root root 0 May 12 18:03 9553f55d-3da2-43ee-ab5d-ff17f78864d2
> > -r--r----- 1 root root 0 May 12 18:03 e6f5a162-d67f-4750-a67c-5d065f2a9910
> >
> > # xxd /sys/kernel/security/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                                     ..
> 
> I am adding a new virt driver to help get the attestation report for the
> SEV-SNP guest. I understand they both are different, in case of the SEV
> the attestation is already completed and we are simply exposing the
> secret provided after the attestation to the userspace, whereas in SNP,
> the userspace is querying the attestation and will probably derive keys
> etc based on the attestation report. I am wondering if we should merge
> both the SEV secret and SNP attestation query in a single driver ?
> Should we cover usecases where SEV guest is not booted under the EFI ?
> Also, it appears that this driver need to be manually loaded, should we
> create a platform device so that the driver binds to platform device and
> use the resource structure to find the location of the secret data?

The nice thing about Dov's device/file is that it's a simple text file
that userspace can then read the secret out of;  I'm not sure if there's
anything similar in SNP (or for that matter TDX, cc'ing in Andi)

Dave

> I was trying to answer some of these questions SNP series. See these patches
> 
> https://marc.info/?l=kvm&m=161978514019741&w=2
> 
> https://marc.info/?l=kvm&m=161978514119744&w=2
> 
> https://marc.info/?l=kvm&m=161978514219751&w=2
> 
> 
> >
> > [1] https://github.com/tianocore/edk2/commit/01726b6d23d4
> >
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ashish Kalra <ashish.kalra@amd.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: James Morris <jmorris@namei.org>
> > Cc: "Serge E. Hallyn" <serge@hallyn.com>
> > Cc: linux-efi@vger.kernel.org
> > Cc: linux-security-module@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> >
> > 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
> >
> >  drivers/firmware/efi/Makefile                 |   2 +-
> >  drivers/firmware/efi/confidential-computing.c |  41 +++
> >  drivers/firmware/efi/efi.c                    |   5 +
> >  drivers/firmware/efi/libstub/Makefile         |   3 +-
> >  .../efi/libstub/confidential-computing.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                          |   2 +
> >  drivers/virt/Makefile                         |   1 +
> >  drivers/virt/sev_secret/Kconfig               |  11 +
> >  drivers/virt/sev_secret/Makefile              |   2 +
> >  drivers/virt/sev_secret/sev_secret.c          | 260 ++++++++++++++++++
> >  include/linux/efi.h                           |  11 +
> >  14 files changed, 410 insertions(+), 2 deletions(-)
> >  create mode 100644 drivers/firmware/efi/confidential-computing.c
> >  create mode 100644 drivers/firmware/efi/libstub/confidential-computing.c
> >  create mode 100644 drivers/virt/sev_secret/Kconfig
> >  create mode 100644 drivers/virt/sev_secret/Makefile
> >  create mode 100644 drivers/virt/sev_secret/sev_secret.c
> >
> >
> > base-commit: c06a2ba62fc401b7aaefd23f5d0bc06d2457ccc1
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Andi Kleen]

On 5/20/2021 3:56 AM, Dr. David Alan Gilbert wrote:
> * Brijes
> The nice thing about Dov's device/file is that it's a simple text file
> that userspace can then read the secret out of;  I'm not sure if there's
> anything similar in SNP (or for that matter TDX, cc'ing in Andi)

In TDX there are two different mechanisms:

- One is a ACPI table (SVKL) that allows to pass small pieces of data 
like keys from the BIOS. We have a little driver to read and clear the 
SVKL data. This would only be used if the TD BIOS does the negotiation 
for the secrets, which it doesn't do currently.

- In the other model the negotiation is done by a user program, just 
using another driver to issue calls to the TDX module. The calls just 
expose the TDREPORT, which encodes the attestation data, but does not 
actually include any secret. Then the negotiation for the secrets is 
done by the program, which can then pass it out to other programs (like 
mount for encrypted file systems). In such a case the secret is never 
touched by the kernel. At least initially we'll use the second option.

-Andi

57ccc1

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Brijesh Singh]

On 5/20/21 5:02 PM, Andi Kleen wrote:
>
> On 5/20/2021 3:56 AM, Dr. David Alan Gilbert wrote:
>> * Brijes
>> The nice thing about Dov's device/file is that it's a simple text file
>> that userspace can then read the secret out of;  I'm not sure if there's
>> anything similar in SNP (or for that matter TDX, cc'ing in Andi)
>
> In TDX there are two different mechanisms:
>
> - One is a ACPI table (SVKL) that allows to pass small pieces of data
> like keys from the BIOS. We have a little driver to read and clear the
> SVKL data. This would only be used if the TD BIOS does the negotiation
> for the secrets, which it doesn't do currently.
>
> - In the other model the negotiation is done by a user program, just
> using another driver to issue calls to the TDX module. The calls just
> expose the TDREPORT, which encodes the attestation data, but does not
> actually include any secret. Then the negotiation for the secrets is
> done by the program, which can then pass it out to other programs
> (like mount for encrypted file systems). In such a case the secret is
> never touched by the kernel. At least initially we'll use the second
> option.
>
The SEV-SNP attestation approach is very similar to what Andi described
for the TDX. However, in the case of legacy SEV and ES, the attestation
verification is performed before the guest is booted. In this case, the
hyervisor puts the secret provided by the guest owner (after the
attestation) at a fixed location. Dov's driver is simply reading that
fixed location and making it available through the simple text file.

In case of the SEV-SNP and TDX, the guest OS participates during the
attestation flow; the driver working on the behalf of userspace and does
not have access to the secret, so it cannot populate the file with the
secrets in it.

-Brijesh



> -Andi
>
> 57ccc1

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[James Bottomley]

On Fri, 2021-05-21 at 10:56 -0500, Brijesh Singh wrote:
[...]
> In case of the SEV-SNP and TDX, the guest OS participates during the
> attestation flow; the driver working on the behalf of userspace and
> does not have access to the secret, so it cannot populate the file
> with the secrets in it.

OK, so for a simple encrypted VM using root on luks, how in SNP does
the boot loader obtain the disk passphrase?

In the non SNP case, it's already upstream: OVMF finds the secret page
and converts it to an EFI config table, which is passed into grub. 
It's starting to sound like we'll need a new grub module for SNP which
will do an active attestation and receive the passphrase over some
channel secure against the cloud provider.  Could you give us an
example of how you think this flow will work?

Thanks,

James

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Brijesh Singh]

On 5/21/21 11:03 AM, James Bottomley wrote:
> On Fri, 2021-05-21 at 10:56 -0500, Brijesh Singh wrote:
> [...]
>> In case of the SEV-SNP and TDX, the guest OS participates during the
>> attestation flow; the driver working on the behalf of userspace and
>> does not have access to the secret, so it cannot populate the file
>> with the secrets in it.
> OK, so for a simple encrypted VM using root on luks, how in SNP does
> the boot loader obtain the disk passphrase?

The GHCB specification v2 provides a new VMGEXIT (GUEST_REQUEST) that be
used either by the guest BIOS or guest kernel to request the attestation
report from the SEV-SNP firmware at any time. In the case of SEV-SNP,
the boot loader need to talk to SEV-SNP firmware to get the attestation
report.

Once the attestation report is available then boot loader need to
connect to guest owner to validate and obtain the secret (i.e disk
decryption key).


> In the non SNP case, it's already upstream: OVMF finds the secret page
> and converts it to an EFI config table, which is passed into grub. 
> It's starting to sound like we'll need a new grub module for SNP which
> will do an active attestation and receive the passphrase over some
> channel secure against the cloud provider.  Could you give us an
> example of how you think this flow will work?

The flow will be like this:

1. During the guest creation, a hypervsior calls LAUNCH_UPDATE (type
secret) to tell PSP to populate a secret page at a fixed guest address. 
Note that secret page is slightly different compare to what we know from
the SEV and ES. In this case the secret page contains a key used for
protecting the communication channel between the PSP and guest.

2. OVMF will create a EFI configuration table with the location of
secrets page.

3. Boot loader (or any other EFI) app can locate the secret page through
the EFI config lookup.

4. Boot loader can use the key from the secret page to communicate with
the PSP and request for the attestation report.

5. Pass the attestation report to the guest owner.

6. The guest owner validates the attestation report and releases the key
directly to the guest.

7. Boot loader can use the secret to unlock or disk or do whatever it
needs with it.


> Thanks,
>
> James
>
>

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Andi Kleen]

> The SEV-SNP attestation approach is very similar to what Andi described
> for the TDX. However, in the case of legacy SEV and ES, the attestation
> verification is performed before the guest is booted. In this case, the
> hyervisor puts the secret provided by the guest owner (after the
> attestation) at a fixed location. Dov's driver is simply reading that
> fixed location and making it available through the simple text file.

That's the same as our SVKL model.

The (not yet posted) driver is here:

https://github.com/intel/tdx/commit/62c2d9fae275d5bf50f869e8cfb71d2ca1f71363

We opted to use ioctls, with the idea that it should be just read and 
cleared once to not let the secret lying around. Typically you would 
just use it to set up dmcrypt or similar once. I think read-and-clear 
with explicit operations is a better model than some virtual file 
because of the security properties.

-Andi

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Dr. David Alan Gilbert]

* Andi Kleen (ak@linux.intel.com) wrote:
> 
> > The SEV-SNP attestation approach is very similar to what Andi described
> > for the TDX. However, in the case of legacy SEV and ES, the attestation
> > verification is performed before the guest is booted. In this case, the
> > hyervisor puts the secret provided by the guest owner (after the
> > attestation) at a fixed location. Dov's driver is simply reading that
> > fixed location and making it available through the simple text file.
> 
> That's the same as our SVKL model.
> 
> The (not yet posted) driver is here:
> 
> https://github.com/intel/tdx/commit/62c2d9fae275d5bf50f869e8cfb71d2ca1f71363
> 

Is there any way we could merge these two so that the TDX/SVKL would
look similar to SEV/ES to userspace?  If we needed some initrd glue here
for luks it would be great if we could have one piece of glue.
[I'm not sure if the numbering/naming of the secrets, and their format
are defined in the same way]

> We opted to use ioctls, with the idea that it should be just read and
> cleared once to not let the secret lying around. Typically you would just
> use it to set up dmcrypt or similar once. I think read-and-clear with
> explicit operations is a better model than some virtual file because of the
> security properties.

Do you think the ioctl is preferable to read+ftruncate/unlink ?
And if it was an ioctl, again could we get some standardisation here -
i.e.
maybe a /dev/confguest with a CONF_COMP_GET_KEY etc ?

Dave

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

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[James Bottomley]

On Mon, 2021-05-24 at 13:08 +0100, Dr. David Alan Gilbert wrote:
> * Andi Kleen (ak@linux.intel.com) wrote:
[...]
> > We opted to use ioctls, with the idea that it should be just read
> > and cleared once to not let the secret lying around. Typically you
> > would just use it to set up dmcrypt or similar once. I think read-
> > and-clear with explicit operations is a better model than some
> > virtual file because of the security properties.
> 
> Do you think the ioctl is preferable to read+ftruncate/unlink ?

I really think if we do a unified upper interface it should be file
based.  An ioctl based on will have too much temptation to expose the
architecture of the underlying system.  However, the way to explore
this would be to ask if there's anything the current ioctl based one
can do that a file based one couldn't?

I think ftruncate/unlink is a preferable interface because it puts the
control in the hands of the consumer: you don't know how far the secret
might get shared, so by doing clear on first read the driver is forcing
the user implementation to cache it instead, thus shifting the problem
not solving it.

James

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Andi Kleen]

On 5/24/2021 5:08 AM, Dr. David Alan Gilbert wrote:
> * Andi K
> Is there any way we could merge these two so that the TDX/SVKL would
> look similar to SEV/ES to userspace?  If we needed some initrd glue here
> for luks it would be great if we could have one piece of glue.
> [I'm not sure if the numbering/naming of the secrets, and their format
> are defined in the same way]
Maybe. There might well be differences in the contents as you say. So 
far SVKL doesn't really exist yet,  initially there will be the initrd 
based agents. The agents definitely will need to know about TDX.
>
> Do you think the ioctl is preferable to read+ftruncate/unlink ?
> And if it was an ioctl, again could we get some standardisation here -
> i.e.
> maybe a /dev/confguest with a CONF_COMP_GET_KEY etc ?

The advantage of the two ioctls is that they are very simple. Anything 
with a file system would be a lot more complicated. For security related 
code simplicity is a virtue.

Also since it's a really simple read and clear model I don't expect the 
value to be used widely, since it will be gone after boot anyways.

-andi

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[James Bottomley]

On Mon, 2021-05-24 at 09:31 -0700, Andi Kleen wrote:
> On 5/24/2021 5:08 AM, Dr. David Alan Gilbert wrote:
> > * Andi K
> > Is there any way we could merge these two so that the TDX/SVKL
> > would look similar to SEV/ES to userspace?  If we needed some
> > initrd glue here for luks it would be great if we could have one
> > piece of glue. [I'm not sure if the numbering/naming of the
> > secrets, and their format are defined in the same way]
> Maybe. There might well be differences in the contents as you say.
> So far SVKL doesn't really exist yet,  initially there will be the
> initrd based agents. The agents definitely will need to know about
> TDX.
> > Do you think the ioctl is preferable to read+ftruncate/unlink ?
> > And if it was an ioctl, again could we get some standardisation
> > here - i.e. maybe a /dev/confguest with a CONF_COMP_GET_KEY etc ?
> 
> The advantage of the two ioctls is that they are very simple.
> Anything with a file system would be a lot more complicated. For
> security related code simplicity is a virtue.

This RFC contained the FS code.  In size terms its very comparable to
your ioctl.

> Also since it's a really simple read and clear model I don't expect
> the value to be used widely, since it will be gone after boot
> anyways.

Enumeration looks to be problematic with your interface ... what are
you supposed to do, keep calling ACPI_SVKL_GET_KEY_INFO on an advancing
index until it gives you an error and then try to work out what key
you're interested in by one of its numeric properties?

I think a GUIDed structure actually helps here because we don't have to
have someone assign, say, u16 types to keys we're interested in and the
filesystem does all the enumeration for us.  It also means new use
cases can simply expand the properties without waiting for any
internals to catch up.

James

Re: [RFC PATCH 0/3] Allow access to confidential computing secret area

[Dov Murik]

On 24/05/2021 20:12, James Bottomley wrote:
> On Mon, 2021-05-24 at 09:31 -0700, Andi Kleen wrote:
>> On 5/24/2021 5:08 AM, Dr. David Alan Gilbert wrote:
>>> * Andi K
>>> Is there any way we could merge these two so that the TDX/SVKL
>>> would look similar to SEV/ES to userspace?  If we needed some
>>> initrd glue here for luks it would be great if we could have one
>>> piece of glue. [I'm not sure if the numbering/naming of the
>>> secrets, and their format are defined in the same way]
>> Maybe. There might well be differences in the contents as you say.
>> So far SVKL doesn't really exist yet,  initially there will be the
>> initrd based agents. The agents definitely will need to know about
>> TDX.
>>> Do you think the ioctl is preferable to read+ftruncate/unlink ?
>>> And if it was an ioctl, again could we get some standardisation
>>> here - i.e. maybe a /dev/confguest with a CONF_COMP_GET_KEY etc ?
>>
>> The advantage of the two ioctls is that they are very simple.
>> Anything with a file system would be a lot more complicated. For
>> security related code simplicity is a virtue.
> 
> This RFC contained the FS code.  In size terms its very comparable to
> your ioctl.
> 
>> Also since it's a really simple read and clear model I don't expect
>> the value to be used widely, since it will be gone after boot
>> anyways.
> 
> Enumeration looks to be problematic with your interface ... what are
> you supposed to do, keep calling ACPI_SVKL_GET_KEY_INFO on an advancing
> index until it gives you an error and then try to work out what key
> you're interested in by one of its numeric properties?
> 
> I think a GUIDed structure actually helps here because we don't have to
> have someone assign, say, u16 types to keys we're interested in and the
> filesystem does all the enumeration for us.  It also means new use
> cases can simply expand the properties without waiting for any
> internals to catch up.
> 


Following the discussion here (and in various other meetings), the next
version of this patch series will keep the securityfs interface but will
introduce an unlink operation for the securityfs entries that will do
the following:

1. Remove the file entry from the securityfs dir

2. Overwrite the secret data memory (memzero_explicit)

3. Overwrite the GUID in the data memory with some invalid GUID
(ffffffff-ffff-.... ?), so that if the module is removed and loaded
again, the securityfs dir will not contain this entry (we'll ignore
these invalid GUIDs in the iteration).



Dov