diff mbox series

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

Message ID 20210628183431.953934-4-dovmurik@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series Allow access to confidential computing secret area | expand

Commit Message

Dov Murik June 28, 2021, 6:34 p.m. UTC
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).

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

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/virt/Kconfig                 |   2 +
 drivers/virt/Makefile                |   1 +
 drivers/virt/sev_secret/Kconfig      |  11 +
 drivers/virt/sev_secret/Makefile     |   2 +
 drivers/virt/sev_secret/sev_secret.c | 298 +++++++++++++++++++++++++++
 5 files changed, 314 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

Comments

Borislav Petkov June 28, 2021, 7:30 p.m. UTC | #1
On Mon, Jun 28, 2021 at 06:34:31PM +0000, Dov Murik wrote:
> 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).
> 
> Removing (unlinking) files in the "sev_secret" directory will zero out
> the secret in memory, and remove the filesystem entry.  If the module
> is removed and loaded again, that secret will not appear in the
> filesystem.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  drivers/virt/Kconfig                 |   2 +
>  drivers/virt/Makefile                |   1 +
>  drivers/virt/sev_secret/Kconfig      |  11 +
>  drivers/virt/sev_secret/Makefile     |   2 +
>  drivers/virt/sev_secret/sev_secret.c | 298 +++++++++++++++++++++++++++
>  5 files changed, 314 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

Same question here: maybe have 

drivers/virt/coco/

and put all coco guest stuff in there.

> 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

That probably needs to depend on CONFIG_AMD_MEM_ENCRYPT - otherwise
what's the point for it.
Dov Murik June 29, 2021, 7:23 a.m. UTC | #2
On 28/06/2021 22:30, Borislav Petkov wrote:
> On Mon, Jun 28, 2021 at 06:34:31PM +0000, Dov Murik wrote:
>> 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).
>>
>> Removing (unlinking) files in the "sev_secret" directory will zero out
>> the secret in memory, and remove the filesystem entry.  If the module
>> is removed and loaded again, that secret will not appear in the
>> filesystem.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  drivers/virt/Kconfig                 |   2 +
>>  drivers/virt/Makefile                |   1 +
>>  drivers/virt/sev_secret/Kconfig      |  11 +
>>  drivers/virt/sev_secret/Makefile     |   2 +
>>  drivers/virt/sev_secret/sev_secret.c | 298 +++++++++++++++++++++++++++
>>  5 files changed, 314 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
> 
> Same question here: maybe have 
> 
> drivers/virt/coco/
> 
> and put all coco guest stuff in there.
> 

I agree, according to what comes up of the conversation we have in
replies to the cover letter.


>> 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
> 
> That probably needs to depend on CONFIG_AMD_MEM_ENCRYPT - otherwise
> what's the point for it.
> 

But not if it's a generic driver that is useful for other confidential
computing implementations.  Consider some non-encrypting guest memory
isolation mechanism where the host can't read most guest pages; this
module might be useful there too.

Also, isn't it a bit weird to depend on CONFIG_AMD_MEM_ENCRYPT but not
use any of its APIs?


-Dov
Borislav Petkov June 29, 2021, 10:48 p.m. UTC | #3
On Tue, Jun 29, 2021 at 10:23:49AM +0300, Dov Murik wrote:
> But not if it's a generic driver that is useful for other confidential
> computing implementations.  Consider some non-encrypting guest memory
> isolation mechanism where the host can't read most guest pages; this
> module might be useful there too.

Anything concrete or just hypothetical?

In any case, if this thing is generic, it should not have "AMD" and
"SEV" in the strings.

> Also, isn't it a bit weird to depend on CONFIG_AMD_MEM_ENCRYPT but not
> use any of its APIs?

Yeah, see above.

Thx.
diff mbox series

Patch

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..11a2d41f4711
--- /dev/null
+++ b/drivers/virt/sev_secret/sev_secret.c
@@ -0,0 +1,298 @@ 
+// 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 (or NULL_GUID if this secret entry was deleted)
+ * @len:	Length of secret entry, including its guid and len fields
+ * @data:	The secret data (full of zeros if this secret entry was deleted)
+ */
+struct secret_entry {
+	efi_guid_t guid;
+	u32 len;
+	u8 data[];
+} __attribute((packed));
+
+static size_t secret_entry_data_len(struct secret_entry *e)
+{
+	return e->len - sizeof(*e);
+}
+
+static struct sev_secret the_sev_secret;
+
+static inline struct sev_secret *sev_secret_get(void)
+{
+	return &the_sev_secret;
+}
+
+static int sev_secret_bin_file_show(struct seq_file *file, void *data)
+{
+	struct secret_entry *e = file->private;
+
+	if (e)
+		seq_write(file, e->data, secret_entry_data_len(e));
+
+	return 0;
+}
+DEFINE_SHOW_ATTRIBUTE(sev_secret_bin_file);
+
+static int sev_secret_unlink(struct inode *dir, struct dentry *dentry)
+{
+	struct sev_secret *s = sev_secret_get();
+	struct inode *inode = d_inode(dentry);
+	struct secret_entry *e = (struct secret_entry *)inode->i_private;
+        int i;
+
+        if (e) {
+		/* Zero out the secret data */
+		memzero_explicit(e->data, secret_entry_data_len(e));
+		e->guid = NULL_GUID;
+        }
+
+        inode->i_private = NULL;
+
+	for (i = 0; i < SEV_SECRET_NUM_FILES; i++)
+		if (s->fs_files[i] == dentry)
+                	s->fs_files[i] = NULL;
+
+        /* securityfs_remove tries to lock the directory's inode, but we reach
+         * the unlink callback when it's already locked */
+	inode_unlock(dir);
+	securityfs_remove(dentry);
+	inode_lock(dir);
+
+	return 0;
+}
+
+static const struct inode_operations sev_secret_dir_inode_operations = {
+	.lookup         = simple_lookup,
+	.unlink         = sev_secret_unlink,
+};
+
+static int sev_secret_map_area(void)
+{
+	struct sev_secret *s = sev_secret_get();
+	struct linux_efi_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--) {
+		securityfs_remove(s->fs_files[i]);
+		s->fs_files[i] = NULL;
+	}
+
+	securityfs_remove(s->fs_dir);
+	s->fs_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->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 err=%ld", 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");