Re: [PATCH 0/4] keys: Introduce a keys frontend for attestation reports

[Dan Williams <dan.j.williams@intel.com>]

Sathyanarayanan Kuppuswamy wrote:
> 
> 
> On 8/8/23 7:19 AM, James Bottomley wrote:
> > On Mon, 2023-08-07 at 16:33 -0700, Dan Williams wrote:
> >> James Bottomley wrote:
> >>> On Fri, 2023-08-04 at 19:37 -0700, Dan Williams wrote:
> >>>> James Bottomley wrote:
> >>>> [..]
> >>>>>> This report interface on the other hand just needs a single
> >>>>>> ABI to retrieve all these vendor formats (until industry
> >>>>>> standardization steps in) and it needs to be flexible (within
> >>>>>> reason) for all the TSM-specific options to be conveyed. I do
> >>>>>> not trust my ioctl ABI minefield avoidance skills to get that
> >>>>>> right. Key blob instantiation feels up to the task.
> >>>>>
> >>>>> To repeat: there's nothing keylike about it.
> >>>>
> >>>> From that perspective there's nothing keylike about user-keys
> >>>> either.
> >>>
> >>> Whataboutism may be popular in politics at the moment, but it
> >>> shouldn't be a justification for API abuse: Just because you might
> >>> be able to argue something else is an abuse of an API doesn't give
> >>> you the right to abuse it further.
> >>
> >> That appears to be the disagreement, that the "user" key type is an
> >> abuse of the keyctl subsystem. Is that the general consensus that it
> >> was added as a mistake that is not be repeated?
> > 
> > I didn't say anything about your assertion, just that you seemed to be
> > trying to argue it.  However, if you look at the properties of keys:
> > 
> > https://www.kernel.org/doc/html/v5.0/security/keys/core.html
> > 
> > You'll see that none of them really applies to the case you're trying
> > to add.
> > 
> >> Otherwise there is significant amount of thought that has gone into
> >> keyctl including quotas, permissions, and instantiation flows.
> >>
> >>
> >>>> Those are just blobs that userspace gets to define how they are
> >>>> used and the keyring is just a transport. I also think that this
> >>>> interface *is* key-like in that it is used in the flow of
> >>>> requesting other key material. The ability to set policy on who
> >>>> can request and instantiate these pre-requisite reports can be
> >>>> controlled by request-key policy.
> >>>
> >>> I thought we agreed back here:
> >>>
> >>> https://lore.kernel.org/linux-coco/64c5ed6eb4ca1_a88b2942a@dwillia2-xfh.jf.intel.com.notmuch/
> >>>
> >>> That it ended up as "just a transport interface".  Has something
> >>> changed that?
> >>
> >> This feedback cast doubt on the assumption that attestation reports
> >> are infrequently generated:
> >>
> >> http://lore.kernel.org/r/CAAH4kHbsFbzL=0gn71qq1-1kL398jiS2rd3as1qUFnLTCB5mHQ@mail.gmail.com
> > 
> > Well, I just read attestation would be called more than once at boot. 
> > That doesn't necessarily require a concurrent interface.
> > 
> 
> Agree. Currently, both sev-guest and tdx-guest (Quote generation part) IOCTL
> drivers use a mutex to serialize the cmd requests. By design, TDX GET_QUOTE
> hypercall also does not support concurrent requests. Since the attestation
> request is expected to be less frequent and not time-critical, I  don't see a
> need to support concurrent interfaces.

At least that was not the level of concurrency I was worried about. The
sysfs approach makes it so that concurrency problem of option-writing vs
report-reading is pushed to userspace.

For example, take the following script:

$ cat -n get_report
     1	#!/bin/bash
     2	tsm=/sys/class/tsm/tsm0
     3	echo $1 > ${tsm}/privlevel
     4	echo $2 > ${tsm}/format
     5	echo "hex encoded attestation report for: $(cat ${tsm}/provider)"
     6	xxd -p -c 0 -r ${tsm}/report

The kernel handles the concurrency of line 6 where it synchronizes
against new writes to the options for the duration of emitting a
coherent report. However, if you do:

$ get_report 2 extended > reportA & get_report 0 default > reportB

...there is race between those invocations to set the options and read
the report.

So to solve that concurrency problem userspace needs to be well behaved
and only have one thread at a time configuring the options and reading
out the report before the next agent is allowed to proceed. There are
several ways to do that, but the kernel only guarantees that the state
of the options is snapshotted for the duration of 6.