Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[James Bottomley]

[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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

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...

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[James Bottomley]

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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Dr. David Alan Gilbert]

* 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
> 
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

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?

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

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.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

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.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Ard Biesheuvel]

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.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

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.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Greg KH]

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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Nayna]

[-- Attachment #1: Type: text/plain, Size: 5156 bytes --]


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.

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

[-- Attachment #2: Type: text/html, Size: 7117 bytes --]

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Nayna]

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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Gerd Hoffmann]

  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

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Matthew Garrett]

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.

Re: [PATCH v7 0/5] Allow guest access to EFI confidential computing secret area

[Dov Murik]

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