Re: [PATCH 05/11] PCI/TSM: Authenticate devices via platform TSM

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

Alexey Kardashevskiy wrote:
> On 6/12/24 09:23, Dan Williams wrote:
> > The PCIe 6.1 specification, section 11, introduces the Trusted Execution
> > Environment (TEE) Device Interface Security Protocol (TDISP).  This
> > interface definition builds upon Component Measurement and
> > Authentication (CMA), and link Integrity and Data Encryption (IDE). It
> > adds support for assigning devices (PCI physical or virtual function) to
> > a confidential VM such that the assigned device is enabled to access
> > guest private memory protected by technologies like Intel TDX, AMD
> > SEV-SNP, RISCV COVE, or ARM CCA.
> > 
> > The "TSM" (TEE Security Manager) is a concept in the TDISP specification
> > of an agent that mediates between a "DSM" (Device Security Manager) and
> > system software in both a VMM and a confidential VM. A VMM uses TSM ABIs
> > to setup link security and assign devices. A confidential VM uses TSM
> > ABIs to transition an assigned device into the TDISP "RUN" state and
> > validate its configuration. From a Linux perspective the TSM abstracts
> > many of the details of TDISP, IDE, and CMA. Some of those details leak
> > through at times, but for the most part TDISP is an internal
> > implementation detail of the TSM.
> > 
> > CONFIG_PCI_TSM adds an "authenticated" attribute and "tsm/" subdirectory
> > to pci-sysfs. The work in progress CONFIG_PCI_CMA (software
> > kernel-native PCI authentication) that can depend on a local to the PCI
> > core implementation, CONFIG_PCI_TSM needs to be prepared for late
> > loading of the platform TSM driver. Consider that the TSM driver may
> > itself be a PCI driver. Userspace can watch /sys/class/tsm/tsm0/uevent
> > to know when the PCI core has TSM services enabled.
> > 
> > The common verbs that the low-level TSM drivers implement are defined by
> > 'struct pci_tsm_ops'. For now only 'connect' and 'disconnect' are
> > defined for secure session and IDE establishment. The 'probe' and
> > 'remove' operations setup per-device context representing the device's
> > security manager (DSM). Note that there is only one DSM expected per
> > physical PCI function, and that coordinates a variable number of
> > assignable interfaces to CVMs.
> > 
> > The locking allows for multiple devices to be executing commands
> > simultaneously, one outstanding command per-device and an rwsem flushes
> > all in-flight commands when a TSM low-level driver/device is removed.
> > 
> > Thanks to Wu Hao for his work on an early draft of this support.
> > 
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > Co-developed-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> >   Documentation/ABI/testing/sysfs-bus-pci |   42 ++++
> >   MAINTAINERS                             |    2
> >   drivers/pci/Kconfig                     |   13 +
> >   drivers/pci/Makefile                    |    1
> >   drivers/pci/pci-sysfs.c                 |    4
> >   drivers/pci/pci.h                       |   10 +
> >   drivers/pci/probe.c                     |    1
> >   drivers/pci/remove.c                    |    3
> >   drivers/pci/tsm.c                       |  293 +++++++++++++++++++++++++++++++
> >   drivers/virt/coco/host/tsm-core.c       |   19 ++
> 
> It is sooo small, make me wonder why we need it at all...

I expect it to grow as more common cross-vendor host TSM functionality
is added.

> > +static int pci_tsm_connect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm *pci_tsm = pdev->tsm;
> > +	int rc;
> > +
> > +	lockdep_assert_held(&pci_tsm_rwsem);
> > +	if_not_guard(mutex_intr, &pci_tsm->lock)
> > +		return -EINTR;
> > +
> > +	if (pci_tsm->state >= PCI_TSM_CONNECT)
> > +		return 0;
> > +	if (pci_tsm->state < PCI_TSM_INIT)
> > +		return -ENXIO;
> > +
> > +	rc = tsm_ops->connect(pdev);
> 
> I thought ages ago it was suggested that DOE/SPDM loop happens in a 
> common place and not in the platform driver implementing 
> tsm_ops->connect() (but I may have missed the point then).

That's still the plan, but I would expect that to be a common helper
that TSM drivers can use and does not need to be enforced as a midlayer
detail in pci/tsm.c. We can add that to pci/doe.c or somewhere more
appropriate for SPDM transport helpers.

[..]
> > diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> > new file mode 100644
> > index 000000000000..beb0d68129bc
> > --- /dev/null
> > +++ b/include/linux/pci-tsm.h
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __PCI_TSM_H
> > +#define __PCI_TSM_H
> > +#include <linux/mutex.h>
> > +
> > +struct pci_dev;
> > +
> > +/**
> > + * struct pci_dsm - Device Security Manager context
> > + * @pdev: physical device back pointer
> > + */
> > +struct pci_dsm {
> > +	struct pci_dev *pdev;
> > +};
> > +
> > +enum pci_tsm_state {
> > +	PCI_TSM_ERR = -1,
> > +	PCI_TSM_INIT,
> > +	PCI_TSM_CONNECT,
> > +};
> > +
> > +/**
> > + * struct pci_tsm - Platform TSM transport context
> > + * @state: reflect device initialized, connected, or bound
> > + * @lock: protect @state vs pci_tsm_ops invocation
> > + * @doe_mb: PCIe Data Object Exchange mailbox
> > + * @dsm: TSM driver device context established by pci_tsm_ops.probe
> > + */
> > +struct pci_tsm {
> > +	enum pci_tsm_state state;
> > +	struct mutex lock;
> > +	struct pci_doe_mb *doe_mb;
> > +	struct pci_dsm *dsm;
> > +};
> 
> doe_mb and state look are device's attribures so will look more 
> appropriate in pci_dsm ("d" from "dsm" is "device"), and pci_tsm would 
> be some intimate knowledge of the ccp.ko (==PSP) about PCI PFs ("t" == 
> "TEE" == TCB == PSP). Or I got it all wrong?

I typed up a long reply only to realize I think this can be made simpler
by only having one common context and drop this subtle 'struct pci_dsm'
distinction.

So, 'struct pci_tsm' is just the common core context / handle for
drivers/pci/tsm.c to communicate with low level TSM driver
implementation. It is allocated by pci_tsm_ops->probe() and freed by
pci_tsm_ops->remove().

A low-level TSM driver can optionally wrap that core context with its
own data, i.e. enforce a container_of() relationship between the core
context and the low level context.

[..]
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 50811b7655dd..a0900e7d2012 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -535,6 +535,9 @@ struct pci_dev {
> >   	u16		ide_cap;	/* Link Integrity & Data Encryption */
> >   	u16		sel_ide_cap;	/* - Selective Stream register block */
> >   	int		nr_ide_mem;	/* - Address range limits for streams */
> > +#endif
> > +#ifdef CONFIG_PCI_TSM
> > +	struct pci_tsm *tsm;		/* TSM operation state */
> >   #endif
> >   	u16		acs_cap;	/* ACS Capability offset */
> >   	u8		supported_speeds; /* Supported Link Speeds Vector */
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > index 1a97459fc23e..46b9a0c6ea4e 100644
> > --- a/include/linux/tsm.h
> > +++ b/include/linux/tsm.h
> > @@ -111,7 +111,9 @@ struct tsm_report_ops {
> >   int tsm_report_register(const struct tsm_report_ops *ops, void *priv);
> >   int tsm_report_unregister(const struct tsm_report_ops *ops);
> >   struct tsm_subsys;
> > +struct pci_tsm_ops;
> >   struct tsm_subsys *tsm_register(struct device *parent,
> > -				const struct attribute_group **groups);
> > +				const struct attribute_group **groups,
> > +				const struct pci_tsm_ops *ops);
> >   void tsm_unregister(struct tsm_subsys *subsys);
> >   #endif /* __TSM_H */
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index 9635b27d2485..19bba65a262c 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -499,6 +499,7 @@
> >   #define  PCI_EXP_DEVCAP_PWR_VAL	0x03fc0000 /* Slot Power Limit Value */
> >   #define  PCI_EXP_DEVCAP_PWR_SCL	0x0c000000 /* Slot Power Limit Scale */
> >   #define  PCI_EXP_DEVCAP_FLR     0x10000000 /* Function Level Reset */
> > +#define  PCI_EXP_DEVCAP_TEE     0x40000000 /* TEE I/O (TDISP) Support */
> >   #define PCI_EXP_DEVCTL		0x08	/* Device Control */
> >   #define  PCI_EXP_DEVCTL_CERE	0x0001	/* Correctable Error Reporting En. */
> >   #define  PCI_EXP_DEVCTL_NFERE	0x0002	/* Non-Fatal Error Reporting Enable */
> > 
> 
> 
> I am trying to wrap my head around your tsm. here is what I got in my tree:
> https://github.com/aik/linux/blob/tsm/include/linux/tsm.h
> 
> Shortly:
> 
> drivers/virt/coco/tsm.ko does sysfs (including "connect" and "bind" to 
> control and "certs"/"report" to attest) and implements tsm_dev/tsm_tdi, 
> it does not know pci_dev;
> 
> drivers/pci/tsm-pci.ko creates/destroys tsm_dev/tsm_dev using tsm.ko;
> 
> drivers/crypto/ccp/ccp.ko (the PSP guy) registers:
> - tsm_subsys in tsm.ko (which does "connect" and "bind" and
> - tsm_bus_subsys in tsm-pci.ko (which does "spdm_forward")
> ccp.ko knows about pci_dev and whatever else comes in the future, and 
> ccp.ko's "connect" implementation calls the IDE library (I am adopting 
> yours now, with some tweaks).
> 
> tsm-dev and tsm-tdi embed struct dev each and are added as children to 
> PCI devices: no hide/show attrs, no additional TSM pointer in struct 
> device or pci_dev, looks like:

The motivation for building awareness of device-security properties
natively into 'struct pci_dev' is the recognition that TSM-based
security is not the only model that Linux needs to contend. The TSM
flow is a superset of PCI-CMA and maybe PCI-IDE in the future (although
Intel seems to be the only architecture that has a concept of allowing
IDE establishment without a TSM).

I understand your motivations to make all of TSM functionality bolted
onto the side of the PCI core. It has some nice properties. However, I
think that is a SEV-TIO centric view of the world. PCI device security
attributes are PCI device attributes and have reason to exist with and
without a TSM. In other words, certificates and measurements should not
be placed behind a TSM ABI because certificates and measurements are
expected to have a native PCI-CMA ABI.

It would be a useful property if software written to retrieve
measurement and certificate chains did that relative to the PCI dev
independent of TSM presence.

> aik@sc ~> ls  /sys/bus/pci/devices/0000:e1:04.0/tsm-tdi/tdi:0000:e1:04.0/
> device  power  subsystem  tsm_report  tsm_report_user  tsm_tdi_bind 
> tsm_tdi_status  tsm_tdi_status_user  uevent
> 
> aik@sc ~> ls  /sys/bus/pci/devices/0000:e1:04.0/tsm_dev/
> device  power  subsystem  tsm_certs  tsm_cert_slot  tsm_certs_user 
> tsm_dev_connect  tsm_dev_status  tsm_meas  tsm_meas_user  uevent
> 
> aik@sc ~> ls /sys/class/tsm/tsm0/
> device  power  stream0:0000:e1:00.0  subsystem  uevent
> 
> aik@sc ~> ls /sys/class/tsm-dev/
> tdev:0000:c0:01.1  tdev:0000:e0:01.1  tdev:0000:e1:00.0
> 
> aik@sc ~> ls /sys/class/tsm-tdi/
> tdi:0000:c0:01.1  tdi:0000:e0:01.1  tdi:0000:e1:00.0  tdi:0000:e1:04.0 
> tdi:0000:e1:04.1  tdi:0000:e1:04.2  tdi:0000:e1:04.3

Right, so I remain unconvinced that Linux needs to contend with new "tsm"
class devs vs PCI device objects with security properties especially
when those security properties have a "TSM" and non-"TSM" flavor.