2022-02-02 12:15:51

by James Bottomley

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

[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/[email protected]

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/[email protected]/

> 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



2022-02-03 11:01:20

by Dr. David Alan Gilbert

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

* James Bottomley ([email protected]) 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/[email protected]
>
> 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/[email protected]/
>
> > 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 / [email protected] / Manchester, UK

2022-02-03 11:29:40

by Greg Kroah-Hartman

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

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/[email protected]
>
> 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/[email protected]/

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

2022-02-04 00:03:50

by Matthew Garrett

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

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?