| Message ID | 20210809190157.279332-4-dovmurik@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allow access to confidential computing secret area in SEV guests | expand |
On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > The new sev_secret module exposes the confidential computing (coco) > secret area via securityfs interface. > > When the module is loaded (and securityfs is mounted, typically under > /sys/kernel/security), a "coco/sev_secret" directory is created in > securityfs. In it, a file is created for each secret entry. The name > of each such file is the GUID of the secret entry, and its content is > the secret data. > > This allows applications running in a confidential computing setting to > read secrets provided by the guest owner via a secure secret injection > mechanism (such as AMD SEV's LAUNCH_SECRET command). > > Removing (unlinking) files in the "coco/sev_secret" directory will zero > out the secret in memory, and remove the filesystem entry. If the > module is removed and loaded again, that secret will not appear in the > filesystem. We've also been looking into a similar secret mechanism recently in the context of Android and protected KVM [1]. Our secrets would come from a different source, likely described as a reserved-memory node in the DT, but would need to be exposed to userspace in the same way as the SEV secrets. Originally I tried using a character device, but this approach with securityfs feels neater to me. We're also looking to pass secrets from the bootloader to Linux, outside of any virtualization or confidential compute context (at least a far as I have understood the meaning of the term). Again, this feels like it would be exposed to userspace in the same way. It would be good to be able to share the parts that would be common. I expect that would mean the operations for a secret file and for a directory of secrets at a minimum. But it might also influence the paths in securityfs; I see, looking back, that the "coco" directory was added since the RFC but would a generalized "secret" subsystem make sense? Or would it be preferable for each case to define their own path? [1] -- https://lwn.net/Articles/836693/ > +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) > +{ > + struct sev_secret *s = sev_secret_get(); > + struct inode *inode = d_inode(dentry); > + struct secret_entry *e = (struct secret_entry *)inode->i_private; > + int i; > + > + if (e) { > + /* Zero out the secret data */ > + memzero_explicit(e->data, secret_entry_data_len(e)); Would there be a benefit in flushing these zeros? > + e->guid = NULL_GUID; > + } > + > + inode->i_private = NULL; > + > + for (i = 0; i < SEV_SECRET_NUM_FILES; i++) > + if (s->fs_files[i] == dentry) > + s->fs_files[i] = NULL; > + > + /* > + * securityfs_remove tries to lock the directory's inode, but we reach > + * the unlink callback when it's already locked > + */ > + inode_unlock(dir); > + securityfs_remove(dentry); > + inode_lock(dir); > + > + return 0; > +}
On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote: > > On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > > The new sev_secret module exposes the confidential computing (coco) > > secret area via securityfs interface. > > > > When the module is loaded (and securityfs is mounted, typically under > > /sys/kernel/security), a "coco/sev_secret" directory is created in > > securityfs. In it, a file is created for each secret entry. The name > > of each such file is the GUID of the secret entry, and its content is > > the secret data. > > > > This allows applications running in a confidential computing setting to > > read secrets provided by the guest owner via a secure secret injection > > mechanism (such as AMD SEV's LAUNCH_SECRET command). > > > > Removing (unlinking) files in the "coco/sev_secret" directory will zero > > out the secret in memory, and remove the filesystem entry. If the > > module is removed and loaded again, that secret will not appear in the > > filesystem. > > We've also been looking into a similar secret mechanism recently in the > context of Android and protected KVM [1]. Our secrets would come from a > different source, likely described as a reserved-memory node in the DT, > but would need to be exposed to userspace in the same way as the SEV > secrets. Originally I tried using a character device, but this approach > with securityfs feels neater to me. > Agreed. I particularly like how deleting the file wipes the secret from memory. > We're also looking to pass secrets from the bootloader to Linux, outside > of any virtualization or confidential compute context (at least a far as > I have understood the meaning of the term). Again, this feels like it > would be exposed to userspace in the same way. > Indeed. > It would be good to be able to share the parts that would be common. I > expect that would mean the operations for a secret file and for a > directory of secrets at a minimum. But it might also influence the paths > in securityfs; I see, looking back, that the "coco" directory was added > since the RFC but would a generalized "secret" subsystem make sense? Or > would it be preferable for each case to define their own path? > I think we should avoid 'secret', to be honest. Even if protected KVM is not riding the SEV/TDX wave, I think confidential computing is still an accurate description of its semantics. > [1] -- https://lwn.net/Articles/836693/ > > > +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) > > +{ > > + struct sev_secret *s = sev_secret_get(); > > + struct inode *inode = d_inode(dentry); > > + struct secret_entry *e = (struct secret_entry *)inode->i_private; > > + int i; > > + > > + if (e) { > > + /* Zero out the secret data */ > > + memzero_explicit(e->data, secret_entry_data_len(e)); > > Would there be a benefit in flushing these zeros? > Do you mean cache clean+invalidate? Better to be precise here. > > + e->guid = NULL_GUID; > > + } > > + > > + inode->i_private = NULL; > > + > > + for (i = 0; i < SEV_SECRET_NUM_FILES; i++) > > + if (s->fs_files[i] == dentry) > > + s->fs_files[i] = NULL; > > + > > + /* > > + * securityfs_remove tries to lock the directory's inode, but we reach > > + * the unlink callback when it's already locked > > + */ > > + inode_unlock(dir); > > + securityfs_remove(dentry); > > + inode_lock(dir); > > + > > + return 0; > > +}
On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote: > > > > On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > > > The new sev_secret module exposes the confidential computing (coco) > > > secret area via securityfs interface. > > > > > > When the module is loaded (and securityfs is mounted, typically under > > > /sys/kernel/security), a "coco/sev_secret" directory is created in > > > securityfs. In it, a file is created for each secret entry. The name > > > of each such file is the GUID of the secret entry, and its content is > > > the secret data. > > > > > > This allows applications running in a confidential computing setting to > > > read secrets provided by the guest owner via a secure secret injection > > > mechanism (such as AMD SEV's LAUNCH_SECRET command). > > > > > > Removing (unlinking) files in the "coco/sev_secret" directory will zero > > > out the secret in memory, and remove the filesystem entry. If the > > > module is removed and loaded again, that secret will not appear in the > > > filesystem. > > > > We've also been looking into a similar secret mechanism recently in the > > context of Android and protected KVM [1]. Our secrets would come from a > > different source, likely described as a reserved-memory node in the DT, > > but would need to be exposed to userspace in the same way as the SEV > > secrets. Originally I tried using a character device, but this approach > > with securityfs feels neater to me. > > > > Agreed. I particularly like how deleting the file wipes the secret from memory. > > > We're also looking to pass secrets from the bootloader to Linux, outside > > of any virtualization or confidential compute context (at least a far as > > I have understood the meaning of the term). Again, this feels like it > > would be exposed to userspace in the same way. > > > > Indeed. > > > It would be good to be able to share the parts that would be common. I > > expect that would mean the operations for a secret file and for a > > directory of secrets at a minimum. But it might also influence the paths > > in securityfs; I see, looking back, that the "coco" directory was added > > since the RFC but would a generalized "secret" subsystem make sense? Or > > would it be preferable for each case to define their own path? > > > > I think we should avoid 'secret', to be honest. Even if protected KVM > is not riding the SEV/TDX wave, I think confidential computing is > still an accurate description of its semantics. I agree that protected KVM fits with the ideas of confidential computing. It was the non-virtualization context that I was less certain about. For example, the Open Profile for DICE [2] starts with a hardware secret and derives, at each boot stage, a secret that is passed to the next stage. It's a process that applies both to a VM, matching confidential compute as I understand it, but also the host Linux, which is the part that I wasn't so clear on. [2] -- https://pigweed.googlesource.com/open-dice/+/refs/heads/main/docs/specification.md > > [1] -- https://lwn.net/Articles/836693/ > > > > > +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) > > > +{ > > > + struct sev_secret *s = sev_secret_get(); > > > + struct inode *inode = d_inode(dentry); > > > + struct secret_entry *e = (struct secret_entry *)inode->i_private; > > > + int i; > > > + > > > + if (e) { > > > + /* Zero out the secret data */ > > > + memzero_explicit(e->data, secret_entry_data_len(e)); > > > > Would there be a benefit in flushing these zeros? > > > > Do you mean cache clean+invalidate? Better to be precise here. At least a clean, to have the zeros written back to memory from the cache, in order to overwrite the secret. > > > > + e->guid = NULL_GUID; > > > + } > > > + > > > + inode->i_private = NULL; > > > + > > > + for (i = 0; i < SEV_SECRET_NUM_FILES; i++) > > > + if (s->fs_files[i] == dentry) > > > + s->fs_files[i] = NULL; > > > + > > > + /* > > > + * securityfs_remove tries to lock the directory's inode, but we reach > > > + * the unlink callback when it's already locked > > > + */ > > > + inode_unlock(dir); > > > + securityfs_remove(dentry); > > > + inode_lock(dir); > > > + > > > + return 0; > > > +}
On 19/08/2021 16:02, Andrew Scull wrote: > On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote: >>> >>> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: [...] >>> >>>> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) >>>> +{ >>>> + struct sev_secret *s = sev_secret_get(); >>>> + struct inode *inode = d_inode(dentry); >>>> + struct secret_entry *e = (struct secret_entry *)inode->i_private; >>>> + int i; >>>> + >>>> + if (e) { >>>> + /* Zero out the secret data */ >>>> + memzero_explicit(e->data, secret_entry_data_len(e)); >>> >>> Would there be a benefit in flushing these zeros? >>> >> >> Do you mean cache clean+invalidate? Better to be precise here. > > At least a clean, to have the zeros written back to memory from the > cache, in order to overwrite the secret. > I agree, but not sure how to implement this: I see there's an arch_wb_cache_pmem exported function which internally (in arch/x86/lib/usercopy_64.c) calls clean_cache_range which seems to do what we want (assume the secret can be longer than the cache line). But arch_wb_cache_pmem is declared in include/linux/libnvdimm.h and guarded with #ifdef CONFIG_ARCH_HAS_PMEM_API -- both seem not related to what I'm trying to do. I see there's an exported clflush_cache_range for x86 -- but that's a clean+flush if I understand correctly. Suggestions on how to approach? I can copy the clean_cache_range implementation into the sev_secret module but hopefully there's a better way to reuse. Maybe export clean_cache_range in x86? Since this is for SEV the solution can be x86-specific, but if there's a generic way I guess it's better (I think all of sev_secret module doesn't have x86-specific stuff). -Dov >> >>>> + e->guid = NULL_GUID; >>>> + } >>>> + >>>> + inode->i_private = NULL; >>>> + >>>> + for (i = 0; i < SEV_SECRET_NUM_FILES; i++) >>>> + if (s->fs_files[i] == dentry) >>>> + s->fs_files[i] = NULL; >>>> + >>>> + /* >>>> + * securityfs_remove tries to lock the directory's inode, but we reach >>>> + * the unlink callback when it's already locked >>>> + */ >>>> + inode_unlock(dir); >>>> + securityfs_remove(dentry); >>>> + inode_lock(dir); >>>> + >>>> + return 0; >>>> +}
On Fri, 20 Aug 2021 at 19:36, Dov Murik <dovmurik@linux.ibm.com> wrote: > > > > On 19/08/2021 16:02, Andrew Scull wrote: > > On Mon, 16 Aug 2021 at 10:57, Ard Biesheuvel <ardb@kernel.org> wrote: > >> > >> On Fri, 13 Aug 2021 at 15:05, Andrew Scull <ascull@google.com> wrote: > >>> > >>> On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > > [...] > > >>> > >>>> +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) > >>>> +{ > >>>> + struct sev_secret *s = sev_secret_get(); > >>>> + struct inode *inode = d_inode(dentry); > >>>> + struct secret_entry *e = (struct secret_entry *)inode->i_private; > >>>> + int i; > >>>> + > >>>> + if (e) { > >>>> + /* Zero out the secret data */ > >>>> + memzero_explicit(e->data, secret_entry_data_len(e)); > >>> > >>> Would there be a benefit in flushing these zeros? > >>> > >> > >> Do you mean cache clean+invalidate? Better to be precise here. > > > > At least a clean, to have the zeros written back to memory from the > > cache, in order to overwrite the secret. > > > > I agree, but not sure how to implement this: > > I see there's an arch_wb_cache_pmem exported function which internally > (in arch/x86/lib/usercopy_64.c) calls clean_cache_range which seems to > do what we want (assume the secret can be longer than the cache line). > > But arch_wb_cache_pmem is declared in include/linux/libnvdimm.h and > guarded with #ifdef CONFIG_ARCH_HAS_PMEM_API -- both seem not related to > what I'm trying to do. > > I see there's an exported clflush_cache_range for x86 -- but that's a > clean+flush if I understand correctly. This would be perfectly correct, the invalidation is just unnecessary. > Suggestions on how to approach? I can copy the clean_cache_range > implementation into the sev_secret module but hopefully there's a better > way to reuse. Maybe export clean_cache_range in x86? Exporting sounds much better than duplicating. It looks like the clean-only instruction was added to x86 more recently and with persistent memory as the intended application. d9dc64f30 "x86/asm: Add support for the CLWB instruction" says: "This should be used in favor of clflushopt or clflush in cases where you require the cache line to be written to memory but plan to access the data cache line to be written to memory but plan to access the data" I don't expect the secret table would be accessed with such frequency that it would actually make a difference, but if it's just a quirk of history that the clean-only version isn't exported, now seems as good a time as any to change that! > Since this is for SEV the solution can be x86-specific, but if there's a > generic way I guess it's better (I think all of sev_secret module > doesn't have x86-specific stuff). arch_wb_cache_pmem is the closest to arch agnostic I've seen, but that has it own problems :/
On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: > The new sev_secret module exposes the confidential computing (coco) > secret area via securityfs interface. > > When the module is loaded (and securityfs is mounted, typically under > /sys/kernel/security), a "coco/sev_secret" directory is created in > securityfs. In it, a file is created for each secret entry. The name > of each such file is the GUID of the secret entry, and its content is > the secret data. > > This allows applications running in a confidential computing setting to > read secrets provided by the guest owner via a secure secret injection > mechanism (such as AMD SEV's LAUNCH_SECRET command). > > Removing (unlinking) files in the "coco/sev_secret" directory will zero > out the secret in memory, and remove the filesystem entry. If the > module is removed and loaded again, that secret will not appear in the > filesystem. > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > --- > drivers/virt/Kconfig | 3 + > drivers/virt/Makefile | 1 + > drivers/virt/coco/sev_secret/Kconfig | 11 + > drivers/virt/coco/sev_secret/Makefile | 2 + > drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++ > 5 files changed, 330 insertions(+) > create mode 100644 drivers/virt/coco/sev_secret/Kconfig > create mode 100644 drivers/virt/coco/sev_secret/Makefile > create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c > > diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig > index 8061e8ef449f..6f73672f593f 100644 > --- a/drivers/virt/Kconfig > +++ b/drivers/virt/Kconfig > @@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig" > source "drivers/virt/nitro_enclaves/Kconfig" > > source "drivers/virt/acrn/Kconfig" > + > +source "drivers/virt/coco/sev_secret/Kconfig" > + > endif > diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile > index 3e272ea60cd9..2a7d472478bd 100644 > --- a/drivers/virt/Makefile > +++ b/drivers/virt/Makefile > @@ -8,3 +8,4 @@ obj-y += vboxguest/ > > obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ > obj-$(CONFIG_ACRN_HSM) += acrn/ > +obj-$(CONFIG_AMD_SEV_SECRET) += coco/sev_secret/ > diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig > new file mode 100644 > index 000000000000..76cfb4f405e0 > --- /dev/null > +++ b/drivers/virt/coco/sev_secret/Kconfig > @@ -0,0 +1,11 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +config AMD_SEV_SECRET > + tristate "AMD SEV secret area securityfs support" > + depends on AMD_MEM_ENCRYPT && EFI > + select SECURITYFS > + help > + This is a driver for accessing the AMD SEV secret area via > + securityfs. > + > + To compile this driver as a module, choose M here. > + The module will be called sev_secret. > diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile > new file mode 100644 > index 000000000000..dca0ed3f8f94 > --- /dev/null > +++ b/drivers/virt/coco/sev_secret/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o > diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c > new file mode 100644 > index 000000000000..d9a60166b142 > --- /dev/null > +++ b/drivers/virt/coco/sev_secret/sev_secret.c > @@ -0,0 +1,313 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * sev_secret module > + * > + * Copyright (C) 2021 IBM Corporation > + * Author: Dov Murik <dovmurik@linux.ibm.com> > + */ > + > +/** > + * DOC: sev_secret: Allow reading confidential computing (coco) secret area via > + * securityfs interface. > + * > + * When the module is loaded (and securityfs is mounted, typically under > + * /sys/kernel/security), a "coco/sev_secret" directory is created in > + * securityfs. In it, a file is created for each secret entry. The name of > + * each such file is the GUID of the secret entry, and its content is the > + * secret data. > + */ > + > +#include <linux/seq_file.h> > +#include <linux/fs.h> > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/io.h> > +#include <linux/security.h> > +#include <linux/efi.h> > + > +#define SEV_SECRET_NUM_FILES 64 > + > +#define EFI_SEVSECRET_TABLE_HEADER_GUID \ > + EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b) > + > +struct sev_secret { > + struct dentry *coco_dir; > + struct dentry *fs_dir; > + struct dentry *fs_files[SEV_SECRET_NUM_FILES]; > + struct linux_efi_coco_secret_area *secret_area; > +}; > + > +/* > + * Structure of the SEV secret area > + * > + * Offset Length > + * (bytes) (bytes) Usage > + * ------- ------- ----- > + * 0 16 Secret table header GUID (must be 1e74f542-71dd-4d66-963e-ef4287ff173b) > + * 16 4 Length of bytes of the entire secret area > + * > + * 20 16 First secret entry's GUID > + * 36 4 First secret entry's length in bytes (= 16 + 4 + x) > + * 40 x First secret entry's data > + * > + * 40+x 16 Second secret entry's GUID > + * 56+x 4 Second secret entry's length in bytes (= 16 + 4 + y) > + * 60+x y Second secret entry's data > + * > + * (... and so on for additional entries) Why isn't all of this documented in Documentation/ABI/ which is needed for any new user/kernel api that you come up with like this. We have to have it documented somewhere, otherwise how will you know how to use these files? thanks greg k-h
On 02/09/2021 15:59, Greg KH wrote: > On Mon, Aug 09, 2021 at 07:01:57PM +0000, Dov Murik wrote: >> The new sev_secret module exposes the confidential computing (coco) >> secret area via securityfs interface. >> >> When the module is loaded (and securityfs is mounted, typically under >> /sys/kernel/security), a "coco/sev_secret" directory is created in >> securityfs. In it, a file is created for each secret entry. The name >> of each such file is the GUID of the secret entry, and its content is >> the secret data. >> >> This allows applications running in a confidential computing setting to >> read secrets provided by the guest owner via a secure secret injection >> mechanism (such as AMD SEV's LAUNCH_SECRET command). >> >> Removing (unlinking) files in the "coco/sev_secret" directory will zero >> out the secret in memory, and remove the filesystem entry. If the >> module is removed and loaded again, that secret will not appear in the >> filesystem. >> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> --- >> drivers/virt/Kconfig | 3 + >> drivers/virt/Makefile | 1 + >> drivers/virt/coco/sev_secret/Kconfig | 11 + >> drivers/virt/coco/sev_secret/Makefile | 2 + >> drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++ >> 5 files changed, 330 insertions(+) >> create mode 100644 drivers/virt/coco/sev_secret/Kconfig >> create mode 100644 drivers/virt/coco/sev_secret/Makefile >> create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c >> >> diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig >> index 8061e8ef449f..6f73672f593f 100644 >> --- a/drivers/virt/Kconfig >> +++ b/drivers/virt/Kconfig >> @@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig" >> source "drivers/virt/nitro_enclaves/Kconfig" >> >> source "drivers/virt/acrn/Kconfig" >> + >> +source "drivers/virt/coco/sev_secret/Kconfig" >> + >> endif >> diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile >> index 3e272ea60cd9..2a7d472478bd 100644 >> --- a/drivers/virt/Makefile >> +++ b/drivers/virt/Makefile >> @@ -8,3 +8,4 @@ obj-y += vboxguest/ >> >> obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ >> obj-$(CONFIG_ACRN_HSM) += acrn/ >> +obj-$(CONFIG_AMD_SEV_SECRET) += coco/sev_secret/ >> diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig >> new file mode 100644 >> index 000000000000..76cfb4f405e0 >> --- /dev/null >> +++ b/drivers/virt/coco/sev_secret/Kconfig >> @@ -0,0 +1,11 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +config AMD_SEV_SECRET >> + tristate "AMD SEV secret area securityfs support" >> + depends on AMD_MEM_ENCRYPT && EFI >> + select SECURITYFS >> + help >> + This is a driver for accessing the AMD SEV secret area via >> + securityfs. >> + >> + To compile this driver as a module, choose M here. >> + The module will be called sev_secret. >> diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile >> new file mode 100644 >> index 000000000000..dca0ed3f8f94 >> --- /dev/null >> +++ b/drivers/virt/coco/sev_secret/Makefile >> @@ -0,0 +1,2 @@ >> +# SPDX-License-Identifier: GPL-2.0-only >> +obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o >> diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c >> new file mode 100644 >> index 000000000000..d9a60166b142 >> --- /dev/null >> +++ b/drivers/virt/coco/sev_secret/sev_secret.c >> @@ -0,0 +1,313 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * sev_secret module >> + * >> + * Copyright (C) 2021 IBM Corporation >> + * Author: Dov Murik <dovmurik@linux.ibm.com> >> + */ >> + >> +/** >> + * DOC: sev_secret: Allow reading confidential computing (coco) secret area via >> + * securityfs interface. >> + * >> + * When the module is loaded (and securityfs is mounted, typically under >> + * /sys/kernel/security), a "coco/sev_secret" directory is created in >> + * securityfs. In it, a file is created for each secret entry. The name of >> + * each such file is the GUID of the secret entry, and its content is the >> + * secret data. >> + */ >> + >> +#include <linux/seq_file.h> >> +#include <linux/fs.h> >> +#include <linux/kernel.h> >> +#include <linux/init.h> >> +#include <linux/module.h> >> +#include <linux/io.h> >> +#include <linux/security.h> >> +#include <linux/efi.h> >> + >> +#define SEV_SECRET_NUM_FILES 64 >> + >> +#define EFI_SEVSECRET_TABLE_HEADER_GUID \ >> + EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b) >> + >> +struct sev_secret { >> + struct dentry *coco_dir; >> + struct dentry *fs_dir; >> + struct dentry *fs_files[SEV_SECRET_NUM_FILES]; >> + struct linux_efi_coco_secret_area *secret_area; >> +}; >> + >> +/* >> + * Structure of the SEV secret area >> + * >> + * Offset Length >> + * (bytes) (bytes) Usage >> + * ------- ------- ----- >> + * 0 16 Secret table header GUID (must be 1e74f542-71dd-4d66-963e-ef4287ff173b) >> + * 16 4 Length of bytes of the entire secret area >> + * >> + * 20 16 First secret entry's GUID >> + * 36 4 First secret entry's length in bytes (= 16 + 4 + x) >> + * 40 x First secret entry's data >> + * >> + * 40+x 16 Second secret entry's GUID >> + * 56+x 4 Second secret entry's length in bytes (= 16 + 4 + y) >> + * 60+x y Second secret entry's data >> + * >> + * (... and so on for additional entries) > > Why isn't all of this documented in Documentation/ABI/ which is needed > for any new user/kernel api that you come up with like this. We have to > have it documented somewhere, otherwise how will you know how to use > these files? Yes, you're right, I'll add such documentation. Note that the ABI (for userspace programs) is the filesystem paths and usage (read + unlink), and not the GUIDed table explained above your comment. That GUIDed table is passed from the Guest Owner via SEV secret injection into OVMF and from there to the kernel memory (patches 1+2 in this series). So userspace doesn't see this GUIDed table structure at all. I should probably add this story to this file's header comment, or some other place which will document this module (suggestions welcome). -Dov
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig index 8061e8ef449f..6f73672f593f 100644 --- a/drivers/virt/Kconfig +++ b/drivers/virt/Kconfig @@ -36,4 +36,7 @@ source "drivers/virt/vboxguest/Kconfig" source "drivers/virt/nitro_enclaves/Kconfig" source "drivers/virt/acrn/Kconfig" + +source "drivers/virt/coco/sev_secret/Kconfig" + endif diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile index 3e272ea60cd9..2a7d472478bd 100644 --- a/drivers/virt/Makefile +++ b/drivers/virt/Makefile @@ -8,3 +8,4 @@ obj-y += vboxguest/ obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/ obj-$(CONFIG_ACRN_HSM) += acrn/ +obj-$(CONFIG_AMD_SEV_SECRET) += coco/sev_secret/ diff --git a/drivers/virt/coco/sev_secret/Kconfig b/drivers/virt/coco/sev_secret/Kconfig new file mode 100644 index 000000000000..76cfb4f405e0 --- /dev/null +++ b/drivers/virt/coco/sev_secret/Kconfig @@ -0,0 +1,11 @@ +# SPDX-License-Identifier: GPL-2.0-only +config AMD_SEV_SECRET + tristate "AMD SEV secret area securityfs support" + depends on AMD_MEM_ENCRYPT && EFI + select SECURITYFS + help + This is a driver for accessing the AMD SEV secret area via + securityfs. + + To compile this driver as a module, choose M here. + The module will be called sev_secret. diff --git a/drivers/virt/coco/sev_secret/Makefile b/drivers/virt/coco/sev_secret/Makefile new file mode 100644 index 000000000000..dca0ed3f8f94 --- /dev/null +++ b/drivers/virt/coco/sev_secret/Makefile @@ -0,0 +1,2 @@ +# SPDX-License-Identifier: GPL-2.0-only +obj-$(CONFIG_AMD_SEV_SECRET) += sev_secret.o diff --git a/drivers/virt/coco/sev_secret/sev_secret.c b/drivers/virt/coco/sev_secret/sev_secret.c new file mode 100644 index 000000000000..d9a60166b142 --- /dev/null +++ b/drivers/virt/coco/sev_secret/sev_secret.c @@ -0,0 +1,313 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * sev_secret module + * + * Copyright (C) 2021 IBM Corporation + * Author: Dov Murik <dovmurik@linux.ibm.com> + */ + +/** + * DOC: sev_secret: Allow reading confidential computing (coco) secret area via + * securityfs interface. + * + * When the module is loaded (and securityfs is mounted, typically under + * /sys/kernel/security), a "coco/sev_secret" directory is created in + * securityfs. In it, a file is created for each secret entry. The name of + * each such file is the GUID of the secret entry, and its content is the + * secret data. + */ + +#include <linux/seq_file.h> +#include <linux/fs.h> +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/module.h> +#include <linux/io.h> +#include <linux/security.h> +#include <linux/efi.h> + +#define SEV_SECRET_NUM_FILES 64 + +#define EFI_SEVSECRET_TABLE_HEADER_GUID \ + EFI_GUID(0x1e74f542, 0x71dd, 0x4d66, 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b) + +struct sev_secret { + struct dentry *coco_dir; + struct dentry *fs_dir; + struct dentry *fs_files[SEV_SECRET_NUM_FILES]; + struct linux_efi_coco_secret_area *secret_area; +}; + +/* + * Structure of the SEV secret area + * + * Offset Length + * (bytes) (bytes) Usage + * ------- ------- ----- + * 0 16 Secret table header GUID (must be 1e74f542-71dd-4d66-963e-ef4287ff173b) + * 16 4 Length of bytes of the entire secret area + * + * 20 16 First secret entry's GUID + * 36 4 First secret entry's length in bytes (= 16 + 4 + x) + * 40 x First secret entry's data + * + * 40+x 16 Second secret entry's GUID + * 56+x 4 Second secret entry's length in bytes (= 16 + 4 + y) + * 60+x y Second secret entry's data + * + * (... and so on for additional entries) + * + * The GUID of each secret entry designates the usage of the secret data. + */ + +/** + * struct secret_header - Header of entire secret area; this should be followed + * by instances of struct secret_entry. + * @guid: Must be EFI_SEVSECRET_TABLE_HEADER_GUID + * @len: Length in bytes of entire secret area, including header + */ +struct secret_header { + efi_guid_t guid; + u32 len; +} __attribute((packed)); + +/** + * struct secret_entry - Holds one secret entry + * @guid: Secret-specific GUID (or NULL_GUID if this secret entry was deleted) + * @len: Length of secret entry, including its guid and len fields + * @data: The secret data (full of zeros if this secret entry was deleted) + */ +struct secret_entry { + efi_guid_t guid; + u32 len; + u8 data[]; +} __attribute((packed)); + +static size_t secret_entry_data_len(struct secret_entry *e) +{ + return e->len - sizeof(*e); +} + +static struct sev_secret the_sev_secret; + +static inline struct sev_secret *sev_secret_get(void) +{ + return &the_sev_secret; +} + +static int sev_secret_bin_file_show(struct seq_file *file, void *data) +{ + struct secret_entry *e = file->private; + + if (e) + seq_write(file, e->data, secret_entry_data_len(e)); + + return 0; +} +DEFINE_SHOW_ATTRIBUTE(sev_secret_bin_file); + +static int sev_secret_unlink(struct inode *dir, struct dentry *dentry) +{ + struct sev_secret *s = sev_secret_get(); + struct inode *inode = d_inode(dentry); + struct secret_entry *e = (struct secret_entry *)inode->i_private; + int i; + + if (e) { + /* Zero out the secret data */ + memzero_explicit(e->data, secret_entry_data_len(e)); + e->guid = NULL_GUID; + } + + inode->i_private = NULL; + + for (i = 0; i < SEV_SECRET_NUM_FILES; i++) + if (s->fs_files[i] == dentry) + s->fs_files[i] = NULL; + + /* + * securityfs_remove tries to lock the directory's inode, but we reach + * the unlink callback when it's already locked + */ + inode_unlock(dir); + securityfs_remove(dentry); + inode_lock(dir); + + return 0; +} + +static const struct inode_operations sev_secret_dir_inode_operations = { + .lookup = simple_lookup, + .unlink = sev_secret_unlink, +}; + +static int sev_secret_map_area(void) +{ + struct sev_secret *s = sev_secret_get(); + struct linux_efi_coco_secret_area *secret_area; + u32 secret_area_size; + + if (efi.coco_secret == EFI_INVALID_TABLE_ADDR) { + pr_err("Secret area address is not available\n"); + return -EINVAL; + } + + secret_area = memremap(efi.coco_secret, sizeof(*secret_area), MEMREMAP_WB); + if (secret_area == NULL) { + pr_err("Could not map secret area header\n"); + return -ENOMEM; + } + + secret_area_size = sizeof(*secret_area) + secret_area->size; + memunmap(secret_area); + + secret_area = memremap(efi.coco_secret, secret_area_size, MEMREMAP_WB); + if (secret_area == NULL) { + pr_err("Could not map secret area\n"); + return -ENOMEM; + } + + s->secret_area = secret_area; + return 0; +} + +static void sev_secret_securityfs_teardown(void) +{ + struct sev_secret *s = sev_secret_get(); + int i; + + for (i = (SEV_SECRET_NUM_FILES - 1); i >= 0; i--) { + securityfs_remove(s->fs_files[i]); + s->fs_files[i] = NULL; + } + + securityfs_remove(s->fs_dir); + s->fs_dir = NULL; + + securityfs_remove(s->coco_dir); + s->coco_dir = NULL; + + pr_debug("Removed sev_secret securityfs entries\n"); +} + +static int sev_secret_securityfs_setup(void) +{ + efi_guid_t tableheader_guid = EFI_SEVSECRET_TABLE_HEADER_GUID; + struct sev_secret *s = sev_secret_get(); + int ret = 0, i = 0, bytes_left; + unsigned char *ptr; + struct secret_header *h; + struct secret_entry *e; + struct dentry *dent; + char guid_str[EFI_VARIABLE_GUID_LEN + 1]; + + s->coco_dir = NULL; + s->fs_dir = NULL; + memset(s->fs_files, 0, sizeof(s->fs_files)); + + dent = securityfs_create_dir("coco", NULL); + if (IS_ERR(dent)) { + pr_err("Error creating coco securityfs directory entry err=%ld\n", PTR_ERR(dent)); + return PTR_ERR(dent); + } + s->coco_dir = dent; + + dent = securityfs_create_dir("sev_secret", s->coco_dir); + if (IS_ERR(dent)) { + pr_err("Error creating SEV secret securityfs directory entry err=%ld\n", + PTR_ERR(dent)); + return PTR_ERR(dent); + } + d_inode(dent)->i_op = &sev_secret_dir_inode_operations; + s->fs_dir = dent; + + ptr = s->secret_area->area; + h = (struct secret_header *)ptr; + if (memcmp(&h->guid, &tableheader_guid, sizeof(h->guid))) { + pr_err("SEV secret area does not start with correct GUID\n"); + ret = -EINVAL; + goto err_cleanup; + } + if (h->len < sizeof(*h)) { + pr_err("SEV secret area reported length is too small\n"); + ret = -EINVAL; + goto err_cleanup; + } + + bytes_left = h->len - sizeof(*h); + ptr += sizeof(*h); + while (bytes_left >= (int)sizeof(*e) && i < SEV_SECRET_NUM_FILES) { + e = (struct secret_entry *)ptr; + if (e->len < sizeof(*e) || e->len > (unsigned int)bytes_left) { + pr_err("SEV secret area is corrupted\n"); + ret = -EINVAL; + goto err_cleanup; + } + + /* Skip deleted entries (which will have NULL_GUID) */ + if (efi_guidcmp(e->guid, NULL_GUID)) { + efi_guid_to_str(&e->guid, guid_str); + + dent = securityfs_create_file(guid_str, 0440, s->fs_dir, (void *)e, + &sev_secret_bin_file_fops); + if (IS_ERR(dent)) { + pr_err("Error creating SEV secret securityfs entry\n"); + ret = PTR_ERR(dent); + goto err_cleanup; + } + + s->fs_files[i++] = dent; + } + ptr += e->len; + bytes_left -= e->len; + } + + pr_debug("Created %d entries in sev_secret securityfs\n", i); + return 0; + +err_cleanup: + sev_secret_securityfs_teardown(); + return ret; +} + +static void sev_secret_unmap_area(void) +{ + struct sev_secret *s = sev_secret_get(); + + if (s->secret_area) { + memunmap(s->secret_area); + s->secret_area = NULL; + } +} + +static int __init sev_secret_init(void) +{ + int ret; + + ret = sev_secret_map_area(); + if (ret) + return ret; + + ret = sev_secret_securityfs_setup(); + if (ret) + goto err_unmap; + + return ret; + +err_unmap: + sev_secret_unmap_area(); + return ret; +} + +static void __exit sev_secret_exit(void) +{ + sev_secret_securityfs_teardown(); + sev_secret_unmap_area(); +} + +module_init(sev_secret_init); +module_exit(sev_secret_exit); + +MODULE_DESCRIPTION("AMD SEV confidential computing secret area access"); +MODULE_AUTHOR("IBM"); +MODULE_LICENSE("GPL");
The new sev_secret module exposes the confidential computing (coco) secret area via securityfs interface. When the module is loaded (and securityfs is mounted, typically under /sys/kernel/security), a "coco/sev_secret" directory is created in securityfs. In it, a file is created for each secret entry. The name of each such file is the GUID of the secret entry, and its content is the secret data. This allows applications running in a confidential computing setting to read secrets provided by the guest owner via a secure secret injection mechanism (such as AMD SEV's LAUNCH_SECRET command). Removing (unlinking) files in the "coco/sev_secret" directory will zero out the secret in memory, and remove the filesystem entry. If the module is removed and loaded again, that secret will not appear in the filesystem. Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> --- drivers/virt/Kconfig | 3 + drivers/virt/Makefile | 1 + drivers/virt/coco/sev_secret/Kconfig | 11 + drivers/virt/coco/sev_secret/Makefile | 2 + drivers/virt/coco/sev_secret/sev_secret.c | 313 ++++++++++++++++++++++ 5 files changed, 330 insertions(+) create mode 100644 drivers/virt/coco/sev_secret/Kconfig create mode 100644 drivers/virt/coco/sev_secret/Makefile create mode 100644 drivers/virt/coco/sev_secret/sev_secret.c