* [PATCH v2 1/4] x86: Export clean_cache_range()
2021-10-07 6:18 [PATCH v2 0/4] Allow access to confidential computing secret area in SEV guests Dov Murik
@ 2021-10-07 6:18 ` Dov Murik
2021-10-07 6:18 ` [PATCH v2 2/4] efi/libstub: Copy confidential computing secret area Dov Murik
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-07 6:18 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, Greg KH, Andrew Scull, Dr. David Alan Gilbert,
James Bottomley, Tobin Feldman-Fitzthum, Jim Cadden,
Daniele Buono, linux-coco, linux-security-module, linux-kernel
Export clean_cache_range() which is similar to the existing
clflush_cache_range() but uses the CLWB (cache line write back)
instruction instead of CLFLUSH.
Remove existing implementation of clean_cache_range() from
arch/x86/lib/usercopy_64.c .
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
arch/x86/include/asm/cacheflush.h | 1 +
arch/x86/lib/usercopy_64.c | 21 ---------------------
arch/x86/mm/pat/set_memory.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/arch/x86/include/asm/cacheflush.h b/arch/x86/include/asm/cacheflush.h
index b192d917a6d0..76452ba1bafb 100644
--- a/arch/x86/include/asm/cacheflush.h
+++ b/arch/x86/include/asm/cacheflush.h
@@ -9,5 +9,6 @@
#include <asm/special_insns.h>
void clflush_cache_range(void *addr, unsigned int size);
+void clean_cache_range(void *vaddr, unsigned int size);
#endif /* _ASM_X86_CACHEFLUSH_H */
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index 508c81e97ab1..ffd39f1d4251 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -57,27 +57,6 @@ unsigned long clear_user(void __user *to, unsigned long n)
EXPORT_SYMBOL(clear_user);
#ifdef CONFIG_ARCH_HAS_UACCESS_FLUSHCACHE
-/**
- * clean_cache_range - write back a cache range with CLWB
- * @vaddr: virtual start address
- * @size: number of bytes to write back
- *
- * Write back a cache range using the CLWB (cache line write back)
- * instruction. Note that @size is internally rounded up to be cache
- * line size aligned.
- */
-static void clean_cache_range(void *addr, size_t size)
-{
- u16 x86_clflush_size = boot_cpu_data.x86_clflush_size;
- unsigned long clflush_mask = x86_clflush_size - 1;
- void *vend = addr + size;
- void *p;
-
- for (p = (void *)((unsigned long)addr & ~clflush_mask);
- p < vend; p += x86_clflush_size)
- clwb(p);
-}
-
void arch_wb_cache_pmem(void *addr, size_t size)
{
clean_cache_range(addr, size);
diff --git a/arch/x86/mm/pat/set_memory.c b/arch/x86/mm/pat/set_memory.c
index ad8a5c586a35..8de029a21e03 100644
--- a/arch/x86/mm/pat/set_memory.c
+++ b/arch/x86/mm/pat/set_memory.c
@@ -319,6 +319,36 @@ void clflush_cache_range(void *vaddr, unsigned int size)
}
EXPORT_SYMBOL_GPL(clflush_cache_range);
+static void clean_cache_range_opt(void *vaddr, unsigned int size)
+{
+ const unsigned long clflush_size = boot_cpu_data.x86_clflush_size;
+ void *p = (void *)((unsigned long)vaddr & ~(clflush_size - 1));
+ void *vend = vaddr + size;
+
+ if (p >= vend)
+ return;
+
+ for (; p < vend; p += clflush_size)
+ clwb(p);
+}
+
+/**
+ * clean_cache_range - write back a cache range with CLWB
+ * @vaddr: virtual start address
+ * @size: number of bytes to write back
+ *
+ * CLWB (cache line write back) is an unordered instruction which needs fencing
+ * with MFENCE or SFENCE to avoid ordering issues. Note that @size is
+ * internally rounded up to be cache line size aligned.
+ */
+void clean_cache_range(void *vaddr, unsigned int size)
+{
+ mb();
+ clean_cache_range_opt(vaddr, size);
+ mb();
+}
+EXPORT_SYMBOL_GPL(clean_cache_range);
+
#ifdef CONFIG_ARCH_HAS_PMEM_API
void arch_invalidate_pmem(void *addr, size_t size)
{
--
2.25.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH v2 2/4] efi/libstub: Copy confidential computing secret area
2021-10-07 6:18 [PATCH v2 0/4] Allow access to confidential computing secret area in SEV guests Dov Murik
2021-10-07 6:18 ` [PATCH v2 1/4] x86: Export clean_cache_range() Dov Murik
@ 2021-10-07 6:18 ` Dov Murik
2021-10-07 6:18 ` [PATCH v2 3/4] efi: Reserve " Dov Murik
2021-10-07 6:18 ` [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
3 siblings, 0 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-07 6:18 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, Greg KH, Andrew Scull, Dr. David Alan Gilbert,
James Bottomley, Tobin Feldman-Fitzthum, Jim Cadden,
Daniele Buono, 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 related [flat|nested] 10+ messages in thread* [PATCH v2 3/4] efi: Reserve confidential computing secret area
2021-10-07 6:18 [PATCH v2 0/4] Allow access to confidential computing secret area in SEV guests Dov Murik
2021-10-07 6:18 ` [PATCH v2 1/4] x86: Export clean_cache_range() Dov Murik
2021-10-07 6:18 ` [PATCH v2 2/4] efi/libstub: Copy confidential computing secret area Dov Murik
@ 2021-10-07 6:18 ` Dov Murik
2021-10-07 6:18 ` [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
3 siblings, 0 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-07 6:18 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, Greg KH, Andrew Scull, Dr. David Alan Gilbert,
James Bottomley, Tobin Feldman-Fitzthum, Jim Cadden,
Daniele Buono, 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 c02ff25dd477..bfd35294703e 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 related [flat|nested] 10+ messages in thread* [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 6:18 [PATCH v2 0/4] Allow access to confidential computing secret area in SEV guests Dov Murik
` (2 preceding siblings ...)
2021-10-07 6:18 ` [PATCH v2 3/4] efi: Reserve " Dov Murik
@ 2021-10-07 6:18 ` Dov Murik
2021-10-07 13:32 ` Dave Hansen
2021-10-07 13:48 ` Dave Hansen
3 siblings, 2 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-07 6:18 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, Greg KH, Andrew Scull, Dr. David Alan Gilbert,
James Bottomley, Tobin Feldman-Fitzthum, Jim Cadden,
Daniele Buono, 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>
---
.../ABI/testing/securityfs-coco-sev_secret | 49 +++
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 | 320 ++++++++++++++++++
6 files changed, 386 insertions(+)
create mode 100644 Documentation/ABI/testing/securityfs-coco-sev_secret
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/Documentation/ABI/testing/securityfs-coco-sev_secret b/Documentation/ABI/testing/securityfs-coco-sev_secret
new file mode 100644
index 000000000000..3db1a66ff5c4
--- /dev/null
+++ b/Documentation/ABI/testing/securityfs-coco-sev_secret
@@ -0,0 +1,49 @@
+What: security/coco/sev_secret
+Date: October 2021
+Contact: Dov Murik <dovmurik@linux.ibm.com>
+Description:
+ Exposes SEV confidential computing (coco) secrets to
+ userspace via securityfs.
+
+ AMD SEV and SEV-ES allow the Guest Owner to inject secrets
+ during VM's launch. The secrets are encrypted by the Guest
+ Owner and decrypted by the AMD-SP (secure processor), and
+ therefore are not readable by the untrusted host.
+
+ The sev_secret module exposes the secrets to userspace. Each
+ secret appears as a file under <securityfs>/coco/sev_secret,
+ where the filename is the GUID of the entry in the secrets
+ table.
+
+ Two operations are supported for the files: read and unlink.
+ Reading the file returns the content of secret entry.
+ Unlinking the file overwrites the secret data with zeroes and
+ removes the entry from the filesystem. A secret cannot be read
+ after it has been unlinked.
+
+ For example, listing the available secrets::
+
+ # modprobe sev_secret
+ # ls -l /sys/kernel/security/coco/sev_secret
+ -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
+ -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
+ -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
+ -r--r----- 1 root root 0 Jun 28 11:54 e6f5a162-d67f-4750-a67c-5d065f2a9910
+
+ Reading the secret data by reading a file::
+
+ # cat /sys/kernel/security/coco/sev_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
+ the-content-of-the-secret-data
+
+ Wiping a secret by unlinking a file::
+
+ # rm /sys/kernel/security/coco/sev_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910
+ # ls -l /sys/kernel/security/coco/sev_secret
+ -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b
+ -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6
+ -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2
+
+ Note: The binary format of the secrets table injected by the
+ Guest Owner is described in
+ drivers/virt/coco/sev_secret/sev_secret.c under "Structure of
+ the SEV secret area".
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..3c83aa80f981
--- /dev/null
+++ b/drivers/virt/coco/sev_secret/sev_secret.c
@@ -0,0 +1,320 @@
+// 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>
+#include <asm/cacheflush.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 void wipe_memory(void *addr, size_t size)
+{
+ memzero_explicit(addr, size);
+ clean_cache_range(addr, size);
+}
+
+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 */
+ wipe_memory(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 related [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 6:18 ` [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
@ 2021-10-07 13:32 ` Dave Hansen
2021-10-07 16:17 ` Dr. David Alan Gilbert
2021-10-07 13:48 ` Dave Hansen
1 sibling, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2021-10-07 13:32 UTC (permalink / raw)
To: Dov Murik, linux-efi
Cc: Borislav Petkov, Ashish Kalra, Brijesh Singh, Tom Lendacky,
Ard Biesheuvel, James Morris, Serge E. Hallyn, Andi Kleen,
Greg KH, Andrew Scull, Dr. David Alan Gilbert, James Bottomley,
Tobin Feldman-Fitzthum, Jim Cadden, Daniele Buono, linux-coco,
linux-security-module, linux-kernel
On 10/6/21 11:18 PM, Dov Murik wrote:
> +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;
> + }
There doesn't seem to be anything truly SEV-specific in here at all.
Isn't this more accurately called "efi_secret" or something? What's to
prevent Intel or an ARM vendor from implementing this?
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 13:32 ` Dave Hansen
@ 2021-10-07 16:17 ` Dr. David Alan Gilbert
2021-10-08 5:40 ` Dov Murik
0 siblings, 1 reply; 10+ messages in thread
From: Dr. David Alan Gilbert @ 2021-10-07 16:17 UTC (permalink / raw)
To: Dave Hansen
Cc: Dov Murik, linux-efi, Borislav Petkov, Ashish Kalra,
Brijesh Singh, Tom Lendacky, Ard Biesheuvel, James Morris,
Serge E. Hallyn, Andi Kleen, Greg KH, Andrew Scull,
James Bottomley, Tobin Feldman-Fitzthum, Jim Cadden,
Daniele Buono, linux-coco, linux-security-module, linux-kernel
* Dave Hansen (dave.hansen@intel.com) wrote:
> On 10/6/21 11:18 PM, Dov Murik wrote:
> > +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;
> > + }
>
> There doesn't seem to be anything truly SEV-specific in here at all.
> Isn't this more accurately called "efi_secret" or something? What's to
> prevent Intel or an ARM vendor from implementing this?
I don't think anything; although the last discussion I remember on list
with Intel was that Intel preferred some interface with an ioctl to read
the secrets and stuff. I'm not quite sure if the attestation/secret
delivery order makes sense with TDX, but if it does, then if you could
persuade someone to standardise on this it would be great.
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 16:17 ` Dr. David Alan Gilbert
@ 2021-10-08 5:40 ` Dov Murik
0 siblings, 0 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-08 5:40 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Dave Hansen
Cc: linux-efi, Borislav Petkov, Ashish Kalra, Brijesh Singh,
Tom Lendacky, Ard Biesheuvel, James Morris, Serge E. Hallyn,
Andi Kleen, Greg KH, Andrew Scull, James Bottomley,
Tobin Feldman-Fitzthum, Jim Cadden, Daniele Buono, linux-coco,
linux-security-module, linux-kernel, Dov Murik
Thanks Dave and Dave for reviewing this.
On 07/10/2021 19:17, Dr. David Alan Gilbert wrote:
> * Dave Hansen (dave.hansen@intel.com) wrote:
>> On 10/6/21 11:18 PM, Dov Murik wrote:
>>> +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;
>>> + }
>>
>> There doesn't seem to be anything truly SEV-specific in here at all.
>> Isn't this more accurately called "efi_secret" or something? What's to
>> prevent Intel or an ARM vendor from implementing this?
>
> I don't think anything; although the last discussion I remember on list
> with Intel was that Intel preferred some interface with an ioctl to read
> the secrets and stuff. I'm not quite sure if the attestation/secret
> delivery order makes sense with TDX, but if it does, then if you could
> persuade someone to standardise on this it would be great.
>
I agree that this series doesn't have any SEV-specific stuff in it; in
fact, I wrote in the cover letter:
+++
This has been tested with AMD SEV and SEV-ES guests, but the kernel side
of handling the secret area has no SEV-specific dependencies, and
therefore might be usable (perhaps with minor changes) for any
confidential computing hardware that can publish the secret area via the
standard EFI config table entry.
+++
However, in previous rounds Boris said [1] that if it's only
hypothetical support for other platforms, I should add a
"depends on AMD_MEM_ENCRYPT" clause. Boris, can you please share your view?
I'm indeed in favor of making this more generic (efi_secret sounds
good), allowing for future support for other early-boot secret injection
mechanisms.
[1] https://lore.kernel.org/linux-coco/YNojYBIwk0xCHQ0v@zn.tnic/
-Dov
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 6:18 ` [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets Dov Murik
2021-10-07 13:32 ` Dave Hansen
@ 2021-10-07 13:48 ` Dave Hansen
2021-10-08 5:51 ` Dov Murik
1 sibling, 1 reply; 10+ messages in thread
From: Dave Hansen @ 2021-10-07 13:48 UTC (permalink / raw)
To: Dov Murik, linux-efi
Cc: Borislav Petkov, Ashish Kalra, Brijesh Singh, Tom Lendacky,
Ard Biesheuvel, James Morris, Serge E. Hallyn, Andi Kleen,
Greg KH, Andrew Scull, Dr. David Alan Gilbert, James Bottomley,
Tobin Feldman-Fitzthum, Jim Cadden, Daniele Buono, linux-coco,
linux-security-module, linux-kernel
On 10/6/21 11:18 PM, Dov Murik wrote:
> +static void wipe_memory(void *addr, size_t size)
> +{
> + memzero_explicit(addr, size);
> + clean_cache_range(addr, size);
> +}
What's the purpose of the clean_cache_range()? It's backed in a CLWB
instruction on x86 which seems like an odd choice. I guess the point is
that the memzero_explicit() will overwrite the contents, but might have
dirty lines in the cache. The CLWB will ensure that the lines are
actually written back to memory, clearing the secret out of memory.
Without the CLWB, the secret might live in memory until the dirtied
cachelines are written back.
Could you document this, please? It would also be nice to include some
of this motivation in the patch that exports clean_cache_range() in the
first place.
I also think clean_cache_range() an odd choice. If it were me, I
probably would have just used the already-exported
clflush_cache_range(). The practical difference between writing back
and flushing the cachelines is basically zero. The lines will never be
reused.
*If* we export anything from x86 code, I think it should be something
which is specific to the task at hand, like arch_invalidate_pmem() is.
Also, when you are modifying x86 code, including exports, it would be
nice to include (all of) the x86 maintainers. The relevant ones for
this series would probably be:
X86 ARCHITECTURE (32-BIT AND 64-BIT)
M: Thomas Gleixner <tglx@linutronix.de>
M: Ingo Molnar <mingo@redhat.com>
M: Borislav Petkov <bp@alien8.de>
M: x86@kernel.org
X86 MM
M: Dave Hansen <dave.hansen@linux.intel.com>
M: Andy Lutomirski <luto@kernel.org>
M: Peter Zijlstra <peterz@infradead.org>
There's also the handy dandy scripts/get_maintainer.pl to help.
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH v2 4/4] virt: Add sev_secret module to expose confidential computing secrets
2021-10-07 13:48 ` Dave Hansen
@ 2021-10-08 5:51 ` Dov Murik
0 siblings, 0 replies; 10+ messages in thread
From: Dov Murik @ 2021-10-08 5:51 UTC (permalink / raw)
To: Dave Hansen, linux-efi
Cc: Borislav Petkov, Ashish Kalra, Brijesh Singh, Tom Lendacky,
Ard Biesheuvel, James Morris, Serge E. Hallyn, Andi Kleen,
Greg KH, Andrew Scull, Dr. David Alan Gilbert, James Bottomley,
Tobin Feldman-Fitzthum, Jim Cadden, Daniele Buono, linux-coco,
linux-security-module, linux-kernel, Dov Murik
Thanks Dave for reviewing this.
On 07/10/2021 16:48, Dave Hansen wrote:
> On 10/6/21 11:18 PM, Dov Murik wrote:
>> +static void wipe_memory(void *addr, size_t size)
>> +{
>> + memzero_explicit(addr, size);
>> + clean_cache_range(addr, size);
>> +}
>
> What's the purpose of the clean_cache_range()? It's backed in a CLWB
> instruction on x86 which seems like an odd choice. I guess the point is
> that the memzero_explicit() will overwrite the contents, but might have
> dirty lines in the cache. The CLWB will ensure that the lines are
> actually written back to memory, clearing the secret out of memory.
> Without the CLWB, the secret might live in memory until the dirtied
> cachelines are written back.
Yes, that's the reason; as suggested by Andrew Scull in [1].
[1] https://lore.kernel.org/linux-coco/CADcWuH0mP+e6GxkUGN3ni_Yu0z8YTn-mo677obH+p-OFCL+wOQ@mail.gmail.com/
>
> Could you document this, please? It would also be nice to include some
> of this motivation in the patch that exports clean_cache_range() in the
> first place.
>
Yes, I'll add that.
> I also think clean_cache_range() an odd choice. If it were me, I
> probably would have just used the already-exported
> clflush_cache_range(). The practical difference between writing back
> and flushing the cachelines is basically zero. The lines will never be
> reused.
>
I agree that performance benefits of CLWB over CLFLUSH are negligible here
(but I have no way of measuring it). Andrew suggested [2] that the extra
invalidation that CLFLUSH does it unnecessary.
But if we all agree that the clflush_cache_range() is OK here, I'm OK
with removing patch 1 and calling clflush_cache_range() in wipe_memory()
here.
Does anyone know of other locations in the kernel where memory is needed
to be scrubbed (zeroed and flushed) - like my wipe_memory()? Maybe there's
a standard way of doing this?
[2] https://lore.kernel.org/linux-coco/CADcWuH05vbFtJ1WYSs3d+_=TGzh-MitvAXp1__d1kGJJkvkWpQ@mail.gmail.com/
> *If* we export anything from x86 code, I think it should be something
> which is specific to the task at hand, like arch_invalidate_pmem() is.
>
> Also, when you are modifying x86 code, including exports, it would be
> nice to include (all of) the x86 maintainers. The relevant ones for
> this series would probably be:
>
> X86 ARCHITECTURE (32-BIT AND 64-BIT)
> M: Thomas Gleixner <tglx@linutronix.de>
> M: Ingo Molnar <mingo@redhat.com>
> M: Borislav Petkov <bp@alien8.de>
> M: x86@kernel.org
>
> X86 MM
> M: Dave Hansen <dave.hansen@linux.intel.com>
> M: Andy Lutomirski <luto@kernel.org>
> M: Peter Zijlstra <peterz@infradead.org>
>
> There's also the handy dandy scripts/get_maintainer.pl to help.
>
You're right, sorry for missing it in this round.
But even if I remove the x86 change, I'll keep you copied anyway...
-Dov
^ permalink raw reply [flat|nested] 10+ messages in thread