| Message ID | 20220201124413.1093099-1-dovmurik@linux.ibm.com |
|---|---|
| Headers | show |
| Series | Allow guest access to EFI confidential computing secret area | expand |
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 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, 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: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
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