Re: [PATCH 00/12] PCI device authentication

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 10 Oct 2023 15:07:41 +1100
Alexey Kardashevskiy <aik@amd.com> wrote:

> On 10/10/23 00:49, Lukas Wunner wrote:
> > On Mon, Oct 09, 2023 at 12:33:35PM +0100, Jonathan Cameron wrote:  
> >> On Sat, 7 Oct 2023 12:04:33 +0200 Lukas Wunner <lukas@wunner.de> wrote:  
> >>> On Fri, Oct 06, 2023 at 09:06:13AM -0700, Dan Williams wrote:  
> >>>> Linux also has an interest in accommodating opt-in to using platform
> >>>> managed keys, so the design requires that key management and session
> >>>> ownership is a system owner policy choice.  
> >>>
> >>> You're pointing out a gap in the specification:
> >>>
> >>> There's an existing mechanism to negotiate which PCI features are
> >>> handled natively by the OS and which by platform firmware and that's
> >>> the _OSC Control Field (PCI Firmware Spec r3.3 table 4-5 and 4-6).
> >>>
> >>> There are currently 10 features whose ownership is negotiated with _OSC,
> >>> examples are Hotplug control and DPC configuration control.
> >>>
> >>> I propose adding an 11th bit to negotiate ownership of the CMA-SPDM
> >>> session.
> >>>
> >>> Once that's added to the PCI Firmware Spec, amending the implementation
> >>> to honor it is trivial:  Just check for platform ownership at the top
> >>> of pci_cma_init() and return.  
> >>
> >> This might want to be a control over the specific DOE instance instead
> >> of a general purpose CMA control (or maybe we want both).
> >>
> >> There is no safe way to access a DOE to find out if it supports CMA
> >> that doesn't potentially break another entity using the mailbox.
> >> Given the DOE instances might be for something entirely different we
> >> can't just decide not to use them at all based on a global control.  
> > 
> > Per PCIe r6.1 sec 6.31.3, the DOE instance used for CMA-SPDM must support
> > "no other data object protocol(s)" besides DOE discovery, CMA-SPDM and
> > Secured CMA-SPDM.
> > 
> > So if the platform doesn't grant the OS control over that DOE instance,
> > unrelated DOE instances and protocols (such as CDAT retrieval) are not
> > affected.
> > 
> > E.g. PCI Firmware Spec r3.3 table 4-5 could be amended with something
> > along the lines of:
> > 
> >    Control Field Bit Offset: 11
> > 
> >    Interpretation: PCI Express Component Measurement and Authentication control
> > 
> >    The operating system sets this bit to 1 to request control over the
> >    DOE instance supporting the CMA-SPDM feature.
> > 
> > You're right that to discover the DOE instance for CMA-SPDM in the
> > first place, it needs to be accessed, which might interfere with the
> > firmware using it.  Perhaps this can be solved with the DOE Busy bit.
> > 
> >   
> >> Any such control becomes messy when hotplug is taken into account.
> >> I suppose we could do a _DSM based on BDF / path to device (to remain
> >> stable across reenumeration) and config space offset to allow the OS
> >> to say 'Hi other entity / firmware are you using this DOE instance?"
> >> Kind of an OSC with parameters.  Also includes the other way around that
> >> the question tells the firmware that if it says "no you can't" the OS
> >> will leave it alone until a reboot or similar - that potentially avoids
> >> the problem that we access DOE instances already without taking care
> >> about this  
> > 
> > PCI Firmware Spec r3.3 table 4-7 lists a number of _DSM Definitions for
> > PCI.  Indeed that could be another solution.  E.g. a newly defined _DSM
> > might return the offset in config space of DOE instance(s) which the OS
> > is not permitted to use.
> > 
> >   
> >> (I dropped ball on this having raised it way back near start
> >> of us adding DOE support.)  
> > 
> > Not your fault.  I think the industry got a bit ahead of itself in
> > its "confidential computing" frenzy and forgot to specify these very
> > basic things.
> > 
> >   
> >> If we do want to do any of these, which spec is appropriate?  Link it to PCI
> >> and propose a PCI firmware spec update? (not sure they have a code
> >> first process available) or make it somewhat generic and propose an
> >> ACPI Code first change?  
> > 
> > PCI Firmware Spec would seem to be appropriate.  However this can't
> > be solved by the kernel community.  
> 
> How so? It is up to the user to decide whether it is SPDM/CMA in the 
> kernel   or   the firmware + coco, both are quite possible (it is IDE 
> which is not possible without the firmware on AMD but we are not there yet).
> 
> But the way SPDM is done now is that if the user (as myself) wants to 
> let the firmware run SPDM - the only choice is disabling CONFIG_CMA 
> completely as CMA is not a (un)loadable module or built-in (with some 
> "blacklist" parameters), and does not provide a sysfs knob to control 
> its tentacles. Kinda harsh.

Not necessarily sufficient unfortunately - if you have a CXL type3 device,
we will run the discovery protocol on the DOE to find out what it supports
(looking for table access protocol used for CDAT). If that hits at wrong point it
will likely break your CMA usage unless you have some hardware lockout of
the relevant PCI config space (in which case that will work with CONFIG_CMA
enabled).

Now you might not care about CXL type 3 devices today, but pretty sure someone
will at somepoint.  Or one of the other uses of DOEs will be relevant.
You might be fine assuming only drivers you've bound ever access the devices
config space, but much nicer to have something standard to ensure that if
we can (and driver specific stuff will deal with it in the short term).

Jonathan

> 
> Note, this PSP firmware is not BIOS (which runs on the same core and has 
> same access to PCI as the host OS), it is a separate platform processor 
> which only programs IDE keys to the PCI RC (via some some internal bus 
> mechanism) but does not do anything on the bus itself and relies on the 
> host OS proxying DOE, and there is no APCI between the core and the psp.
> 
> 
> >  We need to talk to our confidential
> > computing architects and our representatives at the PCISIG to get the
> > spec amended.
> > 
> > Thanks,
> > 
> > Lukas  
>