| Message ID | 20220201124413.1093099-1-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Allow guest access to EFI confidential computing secret area | expand |
On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote: > Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted > Virtualization) allows guest owners to inject secrets into the VMs > memory without the host/hypervisor being able to read them. In SEV, > secret injection is performed early in the VM launch process, before the > guest starts running. > > OVMF already reserves designated area for secret injection (in its > AmdSev package; see edk2 commit 01726b6d23d4 "OvmfPkg/AmdSev: Expose the > Sev Secret area using a configuration table" [1]), but the secrets were > not available in the guest kernel. > > The patch series keeps the address of the EFI-provided memory for > injected secrets, and exposes the secrets to userspace via securityfs > using a new efi_secret kernel module. The module is autoloaded (by the > EFI driver) if the secret area is populated. > > The first patch in EFI keeps the address of the secret area as passed in > the EFI configuration table. The second patch is a quirk fix for older > firmwares didn't mark the secrets page as EFI_RESERVED_TYPE. The third > patch introduces the new efi_secret module that exposes the content of > the secret entries as securityfs files, and allows clearing out secrets > with a file unlink interface. The fourth patch auto-loads the > efi_secret module during startup if the injected secrets area is > populated. The last patch documents the data flow of confidential > computing secret injection. > > As a usage example, consider a guest performing computations on > encrypted files. The Guest Owner provides the decryption key (= secret) > using the secret injection mechanism. The guest application reads the > secret from the efi_secret filesystem and proceeds to decrypt the files > into memory and then performs the needed computations on the content. > > In this example, the host can't read the files from the disk image > because they are encrypted. Host can't read the decryption key because > it is passed using the secret injection mechanism (= secure channel). > Host can't read the decrypted content from memory because it's a > confidential (memory-encrypted) guest. > > This has been tested with AMD SEV 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. > > To enable this functionality, set CONFIG_EFI_SECRET=m when building the > guest kernel. > > Here is a simple example for usage of the efi_secret module in a guest > to which an EFI secret area with 4 secrets was injected during launch: > > # ls -la /sys/kernel/security/coco/efi_secret > total 0 > drwxr-xr-x 2 root root 0 Jun 28 11:54 . > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b > -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6 > -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2 > -r--r----- 1 root root 0 Jun 28 11:54 e6f5a162-d67f-4750-a67c-5d065f2a9910 > > # xxd /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910 > 00000000: 7468 6573 652d 6172 652d 7468 652d 6b61 these-are-the-ka > 00000010: 7461 2d73 6563 7265 7473 0001 0203 0405 ta-secrets...... > 00000020: 0607 .. > > # rm /sys/kernel/security/coco/efi_secret/e6f5a162-d67f-4750-a67c-5d065f2a9910 > > # ls -la /sys/kernel/security/coco/efi_secret > total 0 > drwxr-xr-x 2 root root 0 Jun 28 11:55 . > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec-06879ce3da0b > -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e-d3a0b54312c6 > -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d-ff17f78864d2 Please see my comments on the powerpc version of this type of thing: https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com You all need to work together to come up with a unified place for this and stop making it platform-specific. Until then, we can't take this. sorry, greg k-h
[cc's added] On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote: [...] > > # ls -la /sys/kernel/security/coco/efi_secret > > total 0 > > drwxr-xr-x 2 root root 0 Jun 28 11:55 . > > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > > -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec- > > 06879ce3da0b > > -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e- > > d3a0b54312c6 > > -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d- > > ff17f78864d2 > > Please see my comments on the powerpc version of this type of thing: > > https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com If you want a debate, actually cc'ing the people on the other thread would have been a good start ... For those added, this patch series is at: https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/ > You all need to work together to come up with a unified place for > this and stop making it platform-specific. I'm not entirely sure of that. If you look at the differences between EFI variables and the COCO proposal: the former has an update API which, in the case of signed variables, is rather complex and a UC16 content requirement. The latter is binary data with read only/delete. Plus each variable in EFI is described by a GUID, so having a directory of random guids, some of which behave like COCO secrets and some of which are EFI variables is going to be incredibly confusing (and also break all our current listing tools which seems somewhat undesirable). So we could end up with <common path prefix>/efivar <common path prefix>/coco To achieve the separation, but I really don't see what this buys us. Both filesystems would likely end up with different backends because of the semantic differences and we can easily start now in different places (effectively we've already done this for efi variables) and unify later if that is the chosen direction, so it doesn't look like a blocker. > Until then, we can't take this. I don't believe anyone was asking you to take it. James
On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > [cc's added] > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote: > [...] > > > # ls -la /sys/kernel/security/coco/efi_secret > > > total 0 > > > drwxr-xr-x 2 root root 0 Jun 28 11:55 . > > > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > > > -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec- > > > 06879ce3da0b > > > -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e- > > > d3a0b54312c6 > > > -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d- > > > ff17f78864d2 > > > > Please see my comments on the powerpc version of this type of thing: > > > > https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com > > If you want a debate, actually cc'ing the people on the other thread > would have been a good start ... > > For those added, this patch series is at: > > https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/ Thanks for adding everyone. > > You all need to work together to come up with a unified place for > > this and stop making it platform-specific. > > I'm not entirely sure of that. If you look at the differences between > EFI variables and the COCO proposal: the former has an update API > which, in the case of signed variables, is rather complex and a UC16 > content requirement. The latter is binary data with read only/delete. > Plus each variable in EFI is described by a GUID, so having a directory > of random guids, some of which behave like COCO secrets and some of > which are EFI variables is going to be incredibly confusing (and also > break all our current listing tools which seems somewhat undesirable). > > So we could end up with > > <common path prefix>/efivar > <common path prefix>/coco The powerpc stuff is not efi. But yes, that is messy here. But why doesn't the powerpc follow the coco standard? > To achieve the separation, but I really don't see what this buys us. > Both filesystems would likely end up with different backends because of > the semantic differences and we can easily start now in different > places (effectively we've already done this for efi variables) and > unify later if that is the chosen direction, so it doesn't look like a > blocker. > > > Until then, we can't take this. > > I don't believe anyone was asking you to take it. I was on the review list...
On Tue, 2022-02-01 at 15:41 +0100, Greg KH wrote: > On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > > [cc's added] > > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: [...] > > > You all need to work together to come up with a unified place for > > > this and stop making it platform-specific. > > > > I'm not entirely sure of that. If you look at the differences > > between EFI variables and the COCO proposal: the former has an > > update API which, in the case of signed variables, is rather > > complex and a UC16 content requirement. The latter is binary data > > with read only/delete. Plus each variable in EFI is described by a > > GUID, so having a directory of random guids, some of which behave > > like COCO secrets and some of which are EFI variables is going to > > be incredibly confusing (and also break all our current listing > > tools which seems somewhat undesirable). > > > > So we could end up with > > > > <common path prefix>/efivar > > <common path prefix>/coco > > The powerpc stuff is not efi. But yes, that is messy here. But why > doesn't the powerpc follow the coco standard? There is no coco standard for EFI variables. There's only a UEFI variable standard which, I believe, power tries to follow in some measure since the variables are mostly used for its version of secure boot. Certainly you're either a power or UEFI platform but not both, so they could live at the same location ... that's not true with the coco ones. I added the cc's to see if there are other ideas, but I really think the use cases are too disjoint. As Daniel has previously proposed, it might be possible to unify the power and UEFI implementations ... useful if we want them to respond to the same tooling, but we'll do that by giving them the same EFI semantics. The semantics and source of the coco secrets will still be immutable and completely alien to whatever backend does the non volatile power/efi authenticated variables, so we'll still need two different backends and then it's just a question of arguing about path, which doesn't make sense as a blocker. > > To achieve the separation, but I really don't see what this buys > > us. Both filesystems would likely end up with different backends > > because of the semantic differences and we can easily start now in > > different places (effectively we've already done this for efi > > variables) and unify later if that is the chosen direction, so it > > doesn't look like a blocker. > > > > > Until then, we can't take this. > > > > I don't believe anyone was asking you to take it. > > I was on the review list... You raised a doc/API concenrn. I think you were on the review list to ensure it got addressed. James
* James Bottomley (jejb@linux.ibm.com) wrote: > [cc's added] > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > On Tue, Feb 01, 2022 at 12:44:08PM +0000, Dov Murik wrote: > [...] > > > # ls -la /sys/kernel/security/coco/efi_secret > > > total 0 > > > drwxr-xr-x 2 root root 0 Jun 28 11:55 . > > > drwxr-xr-x 3 root root 0 Jun 28 11:54 .. > > > -r--r----- 1 root root 0 Jun 28 11:54 736870e5-84f0-4973-92ec- > > > 06879ce3da0b > > > -r--r----- 1 root root 0 Jun 28 11:54 83c83f7f-1356-4975-8b7e- > > > d3a0b54312c6 > > > -r--r----- 1 root root 0 Jun 28 11:54 9553f55d-3da2-43ee-ab5d- > > > ff17f78864d2 > > > > Please see my comments on the powerpc version of this type of thing: > > > > https://lore.kernel.org/r/20220122005637.28199-1-nayna@linux.ibm.com > > If you want a debate, actually cc'ing the people on the other thread > would have been a good start ... > > For those added, this patch series is at: > > https://lore.kernel.org/all/20220201124413.1093099-1-dovmurik@linux.ibm.com/ > > > You all need to work together to come up with a unified place for > > this and stop making it platform-specific. > > I'm not entirely sure of that. If you look at the differences between > EFI variables and the COCO proposal: the former has an update API > which, in the case of signed variables, is rather complex and a UC16 > content requirement. The latter is binary data with read only/delete. > Plus each variable in EFI is described by a GUID, so having a directory > of random guids, some of which behave like COCO secrets and some of > which are EFI variables is going to be incredibly confusing (and also > break all our current listing tools which seems somewhat undesirable). > > So we could end up with > > <common path prefix>/efivar > <common path prefix>/coco > > To achieve the separation, but I really don't see what this buys us. > Both filesystems would likely end up with different backends because of > the semantic differences and we can easily start now in different > places (effectively we've already done this for efi variables) and > unify later if that is the chosen direction, so it doesn't look like a > blocker. > > > Until then, we can't take this. > > I don't believe anyone was asking you to take it. I have some sympathy in wanting some unification; (I'm not sure that list of comparison even includes the TDX world). But I'm not sure if they're the same thing - these are strictly constants, they're not changable. But it is a messy list of differences - especially things like the UTF-16 stuff I guess the PowerVM key naming contains nul and / can be ignored - if anyone is silly enough to create keys with those names then they can not access them; so at least that would solve that problem. I don't really understand the talk of 32bit attributes in either the uEFI or PowerVM key store case. Is that GOOGLE_SMI stuff already there? If so I guess there's not much we can do - but it's a shame that there's the directory per variable. Dave > James > >
On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > You all need to work together to come up with a unified place for > > this and stop making it platform-specific. We're talking about things that have massively different semantics. How do we expose that without an unwieldy API that has to try to be a superset of everything implemented, which then has to be extended when yet another implementation shows up with another behavioural quirk? EFI variables already need extremely careful handling to avoid rm -rf /sys bricking the system - should we impose that on everything, or should we allow the underlying implementation to leak through in some ways?
On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote: > On Tue, Feb 01, 2022 at 09:24:50AM -0500, James Bottomley wrote: > > On Tue, 2022-02-01 at 14:50 +0100, Greg KH wrote: > > > You all need to work together to come up with a unified place for > > > this and stop making it platform-specific. > > We're talking about things that have massively different semantics. I see lots of different platforms trying to provide access to their "secure" firmware data to userspace in different ways. That feels to me like they are the same thing that userspace would care about in a unified way. Unless we expeect userspace tools to have to be platform-specific for all of this? That does not seem wise. what am I missing here? thanks, greg k-h
On Wed, Feb 02, 2022 at 07:10:02AM +0100, Greg KH wrote: > On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote: > > We're talking about things that have massively different semantics. > > I see lots of different platforms trying to provide access to their > "secure" firmware data to userspace in different ways. That feels to me > like they are the same thing that userspace would care about in a > unified way. EFI variables are largely for the OS to provide information to the firmware, while this patchset is to provide information from the firmware to the OS. I don't see why we'd expect to use the same userland tooling for both. In the broader case - I don't think we *can* use the same userland tooling for everything. For example, the patches to add support for manipulating the Power secure boot keys originally attempted to make it look like efivars, but the underlying firmware semantics are sufficiently different that even exposing the same kernel interface wouldn't be a sufficient abstraction and userland would still need to behave differently. Exposing an interface that looks consistent but isn't is arguably worse for userland than exposing explicitly distinct interfaces.
On Wed, Feb 02, 2022 at 06:54:43AM +0000, Matthew Garrett wrote: > On Wed, Feb 02, 2022 at 07:10:02AM +0100, Greg KH wrote: > > On Wed, Feb 02, 2022 at 04:01:57AM +0000, Matthew Garrett wrote: > > > We're talking about things that have massively different semantics. > > > > I see lots of different platforms trying to provide access to their > > "secure" firmware data to userspace in different ways. That feels to me > > like they are the same thing that userspace would care about in a > > unified way. > > EFI variables are largely for the OS to provide information to the > firmware, while this patchset is to provide information from the > firmware to the OS. I don't see why we'd expect to use the same userland > tooling for both. I totally agree, I'm not worried about EFI variables here, I don't know why that came up. > In the broader case - I don't think we *can* use the same userland > tooling for everything. For example, the patches to add support for > manipulating the Power secure boot keys originally attempted to make it > look like efivars, but the underlying firmware semantics are > sufficiently different that even exposing the same kernel interface > wouldn't be a sufficient abstraction and userland would still need to > behave differently. Exposing an interface that looks consistent but > isn't is arguably worse for userland than exposing explicitly distinct > interfaces. So what does userspace really need here? Just the ability to find if the platform has blobs that it cares about, and how to read/write them. I see different platform patches trying to stick these blobs in different locations and ways to access (securityfs, sysfs, char device node), which seems crazy to me. Why can't we at least pick one way to access these to start with, and then have the filesystem layout be platform-specific as needed, which will give the correct hints to userspace as to what it needs to do here? thanks, greg k-h
On Wed, Feb 02, 2022 at 08:05:23AM +0100, Greg KH wrote: > I see different platform patches trying to stick these blobs in > different locations and ways to access (securityfs, sysfs, char device > node), which seems crazy to me. Why can't we at least pick one way to > access these to start with, and then have the filesystem layout be > platform-specific as needed, which will give the correct hints to > userspace as to what it needs to do here? Which other examples are you thinking of? I think this conversation may have accidentally become conflated with a different prior one and now we're talking at cross purposes.
On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > On Wed, Feb 02, 2022 at 08:05:23AM +0100, Greg KH wrote: > > > I see different platform patches trying to stick these blobs in > > different locations and ways to access (securityfs, sysfs, char device > > node), which seems crazy to me. Why can't we at least pick one way to > > access these to start with, and then have the filesystem layout be > > platform-specific as needed, which will give the correct hints to > > userspace as to what it needs to do here? > > Which other examples are you thinking of? I think this conversation may > have accidentally become conflated with a different prior one and now > we're talking at cross purposes. This came up a while ago during review of one of the earlier revisions of this patch set. https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/ which describes another two variations on the theme, for pKVM guests as well as Android bare metal.
On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote: > On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > Which other examples are you thinking of? I think this conversation may > > have accidentally become conflated with a different prior one and now > > we're talking at cross purposes. > > This came up a while ago during review of one of the earlier revisions > of this patch set. > > https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/ > > which describes another two variations on the theme, for pKVM guests > as well as Android bare metal. Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, so thought this was related to the efivars/Power secure boot. My apologies, sorry for the noise. In that case, given the apparent agreement between the patch owners that a consistent interface would work for them, I think I agree with Greg that we should strive for that. Given the described behaviour of the Google implementation, it feels like the semantics in this implementation would be sufficient for them as well, but having confirmation of that would be helpful. On the other hand, I also agree that a new filesystem for this is overkill. I did that for efivarfs and I think the primary lesson from that is that people who aren't familiar with the vfs shouldn't be writing filesystems. Securityfs seems entirely reasonable, and it's consistent with other cases where we expose firmware-provided data that's security relevant. The only thing I personally struggle with here is whether "coco" is the best name for it, and whether there are reasonable use cases that wouldn't be directly related to confidential computing (eg, if the firmware on a bare-metal platform had a mechanism for exposing secrets to the OS based on some specific platform security state, it would seem reasonable to expose it via this mechanism but it may not be what we'd normally think of as Confidential Computing). But I'd also say that while we only have one implementation currently sending patches, it's fine for the code to live in that implementation and then be abstracted out once we have another.
On Wed, Feb 02, 2022 at 08:04:01AM +0000, Matthew Garrett wrote: > On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote: > > On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote: > > > Which other examples are you thinking of? I think this conversation may > > > have accidentally become conflated with a different prior one and now > > > we're talking at cross purposes. > > > > This came up a while ago during review of one of the earlier revisions > > of this patch set. > > > > https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/ > > > > which describes another two variations on the theme, for pKVM guests > > as well as Android bare metal. > > Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, > so thought this was related to the efivars/Power secure boot. My > apologies, sorry for the noise. In that case, given the apparent > agreement between the patch owners that a consistent interface would > work for them, I think I agree with Greg that we should strive for that. > Given the described behaviour of the Google implementation, it feels > like the semantics in this implementation would be sufficient for them > as well, but having confirmation of that would be helpful. > > On the other hand, I also agree that a new filesystem for this is > overkill. I did that for efivarfs and I think the primary lesson from > that is that people who aren't familiar with the vfs shouldn't be > writing filesystems. Securityfs seems entirely reasonable, and it's > consistent with other cases where we expose firmware-provided data > that's security relevant. > > The only thing I personally struggle with here is whether "coco" is the > best name for it, and whether there are reasonable use cases that > wouldn't be directly related to confidential computing (eg, if the > firmware on a bare-metal platform had a mechanism for exposing secrets > to the OS based on some specific platform security state, it would seem > reasonable to expose it via this mechanism but it may not be what we'd > normally think of as Confidential Computing). > > But I'd also say that while we only have one implementation currently > sending patches, it's fine for the code to live in that implementation > and then be abstracted out once we have another. Well right now the Android code looks the cleanest and should be about ready to be merged into my tree. But I can almost guarantee that that interface is not what anyone else wants to use, so if you think somehow that everyone else is going to want to deal with a char device node and a simple mmap, with a DT description of the thing, hey, I'm all for it :) Seriously, people need to come up with something sane or this is going to be a total mess. thanks, greg k-h
Hi, > The only thing I personally struggle with here is whether "coco" is the > best name for it, and whether there are reasonable use cases that > wouldn't be directly related to confidential computing (eg, if the > firmware on a bare-metal platform had a mechanism for exposing secrets > to the OS based on some specific platform security state, it would seem > reasonable to expose it via this mechanism but it may not be what we'd > normally think of as Confidential Computing). > > But I'd also say that while we only have one implementation currently > sending patches, it's fine for the code to live in that implementation > and then be abstracted out once we have another. The implementation can be sorted later when a second implementation shows up, but it'll be better if we don't have to change the naming convention. "coco/efi_secrets" doesn't look like a good choice to me, given that it is rather likely we see more users for this showing up. Having a "secrets/" directory looks good to me. Then the individual implementations can either add files to the directory, i.e. efi_secrets would create "secrets/<guid>" files. Or each implementation creates a subdirectory with the secrets, i.e. "secrets/coco/" and "secrets/coco/<guid>". Longer-term (i.e once we have more than one implementation) we probably need a separate module which owns and manages the "secrets/" directory, and possibly provides some common helper functions too. take care, Gerd
On Wed, Feb 02, 2022 at 09:36:53AM +0100, Gerd Hoffmann wrote: > Having a "secrets/" directory looks good to me. Then the individual > implementations can either add files to the directory, i.e. efi_secrets > would create "secrets/<guid>" files. Or each implementation creates a > subdirectory with the secrets, i.e. "secrets/coco/" and > "secrets/coco/<guid>". I prefer a subdirectory, on the basis that we could conceivably end up with more than one implementation on a single device at some point, and also because it makes it trivial for userland to determine what the source is which may make a semantic difference under certain circumstances. > Longer-term (i.e once we have more than one implementation) we probably > need a separate module which owns and manages the "secrets/" directory, > and possibly provides some common helper functions too. Agree.
On 02/02/2022 10:45, Matthew Garrett wrote: > On Wed, Feb 02, 2022 at 09:36:53AM +0100, Gerd Hoffmann wrote: > >> Having a "secrets/" directory looks good to me. Then the individual >> implementations can either add files to the directory, i.e. efi_secrets >> would create "secrets/<guid>" files. Or each implementation creates a >> subdirectory with the secrets, i.e. "secrets/coco/" and >> "secrets/coco/<guid>". > > I prefer a subdirectory, on the basis that we could conceivably end up > with more than one implementation on a single device at some point, and > also because it makes it trivial for userland to determine what the > source is which may make a semantic difference under certain > circumstances. > OK, sounds good. In the next round of the series the module will create the files in <securityfs>/secrets/coco/ . >> Longer-term (i.e once we have more than one implementation) we probably >> need a separate module which owns and manages the "secrets/" directory, >> and possibly provides some common helper functions too. > > Agree. Yes; one candidate for such helper function is a filesystem that implements the "wipe file content from memory on unlink". -Dov
On 2/2/22 03:25, Greg KH wrote: > On Wed, Feb 02, 2022 at 08:04:01AM +0000, Matthew Garrett wrote: >> On Wed, Feb 02, 2022 at 08:22:03AM +0100, Ard Biesheuvel wrote: >>> On Wed, 2 Feb 2022 at 08:10, Matthew Garrett <mjg59@srcf.ucam.org> wrote: >>>> Which other examples are you thinking of? I think this conversation may >>>> have accidentally become conflated with a different prior one and now >>>> we're talking at cross purposes. >>> This came up a while ago during review of one of the earlier revisions >>> of this patch set. >>> >>> https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/ >>> >>> which describes another two variations on the theme, for pKVM guests >>> as well as Android bare metal. >> Oh, I see! That makes much more sense - sorry, I wasn't Cc:ed on that, >> so thought this was related to the efivars/Power secure boot. My >> apologies, sorry for the noise. In that case, given the apparent >> agreement between the patch owners that a consistent interface would >> work for them, I think I agree with Greg that we should strive for that. >> Given the described behaviour of the Google implementation, it feels >> like the semantics in this implementation would be sufficient for them >> as well, but having confirmation of that would be helpful. >> >> On the other hand, I also agree that a new filesystem for this is >> overkill. I did that for efivarfs and I think the primary lesson from >> that is that people who aren't familiar with the vfs shouldn't be >> writing filesystems. Securityfs seems entirely reasonable, and it's >> consistent with other cases where we expose firmware-provided data >> that's security relevant. >> >> The only thing I personally struggle with here is whether "coco" is the >> best name for it, and whether there are reasonable use cases that >> wouldn't be directly related to confidential computing (eg, if the >> firmware on a bare-metal platform had a mechanism for exposing secrets >> to the OS based on some specific platform security state, it would seem >> reasonable to expose it via this mechanism but it may not be what we'd >> normally think of as Confidential Computing). >> >> But I'd also say that while we only have one implementation currently >> sending patches, it's fine for the code to live in that implementation >> and then be abstracted out once we have another. > Well right now the Android code looks the cleanest and should be about > ready to be merged into my tree. > > But I can almost guarantee that that interface is not what anyone else > wants to use, so if you think somehow that everyone else is going to > want to deal with a char device node and a simple mmap, with a DT > description of the thing, hey, I'm all for it :) > > Seriously, people need to come up with something sane or this is going > to be a total mess. > Thanks for adding us to this discussion. I think somehow my last post got html content and didn't make to mailing list, so am posting it again. Sorry to those who are receiving it twice. If I have understood the discussion right, the key idea discussed here is to unify multiple different interfaces(this one, and [1]) exposing secrets for confidential computing usecase via securityfs. And the suggestion is to see if the proposed pseries interface [2] can unify with the coco interface. At high level, pseries interface is reading/writing/adding/deleting variables using the sysfs interface, but the underlying semantics and actual usecases are quite different. The variables exposed via pseries proposed interface are: * Variables owned by firmware as read-only. * Variables owned by bootloader as read-only. * Variables owned by OS and get updated as signed updates. These support both read/write. * Variables owned by OS and get directly updated(unsigned) eg config information or some boot variables. These support both read/write. It can be extended to support variables which contain secrets like symmetric keys, are owned by OS and stored in platform keystore. Naming convention wise also, there are differences like pseries variables do not use GUIDs. The initial patchset discusses secure boot usecase, but it would be extended for other usecases as well. First, I feel the purpose itself is different here. If we still continue, I fear if we will get into similar situation as Matthew mentioned in context of efivars - "the patches to add support for manipulating the Power secure boot keys originally attempted to make it look like efivars, but the underlying firmware semantics are sufficiently different that even exposing the same kernel interface wouldn't be a sufficient abstraction and userland would still need to behave differently. Exposing an interface that looks consistent but isn't is arguably worse for userland than exposing explicitly distinct interfaces." With that, I believe the scope of pseries interface is different and much broader than being discussed here. So, I wonder if it would be better to still keep pseries interface separate from this and have its own platform specific interface. I would be happy to hear the ideas. [1] https://lore.kernel.org/linux-efi/YRZuIIVIzMfgjtEl@google.com/ [2] https://lore.kernel.org/all/20220122005637.28199-1-nayna@linux.ibm.com/ Thanks & Regards, - Nayna