Re: [RFC PATCH 5/5] PCI/TSM: Authenticate devices via platform TSM

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

Jonathan Cameron wrote:
> On Tue, 30 Jan 2024 01:24:14 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
> 
> > The PCIe 6.1 specification, section 11, introduces the Trusted
> > Execution Environment (TEE) Device Interface Security Protocol (TDISP).
> > This interface definition builds upon CMA, component measurement and
> > authentication, and IDE, link integrity and data encryption. It adds
> > support for establishing virtual functions within a device that can be
> > assigned 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 device security manager (DSM) and
> > system software in both a VMM and a VM. 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.
> > 
> > Similar to the PCI core extensions to support CONFIG_PCI_CMA,
> > CONFIG_PCI_TSM builds upon that to reuse the "authenticated" sysfs
> > attribute, and add more properties + controls in a tsm/ subdirectory of
> > the PCI device sysfs interface. Unlike CMA that can depend on a local to
> > the PCI core implementation, 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 depend on the common TSM device
> > uevent to know when the PCI core has TSM services enabled. The PCI
> > device tsm/ subdirectory is supplemented by the TSM device pci/
> > directory for platform global TSM properties + controls.
> > 
> > All vendor TSM implementations share the property of asking the VMM to
> > perform DOE mailbox operations on behalf of the TSM. That common
> > capability is centralized in PCI core code that invokes an ->exec()
> > operation callback potentially multiple times to service a given request
> > (struct pci_tsm_req). Future operations / verbs will be handled
> > similarly with the "request + exec" model. For now, only "connect" and
> > "disconnect" are implemented which at a minimum is expected to establish
> > IDE for the link.
> > 
> > In addition to requests the low-level TSM implementation is notified of
> > device arrival and departure events so that it can filter devices that
> > the TSM is not prepared to support, or otherwise setup and teardown
> > per-device context.
> > 
> > Cc: Wu Hao <hao.wu@intel.com>
> > Cc: Yilun Xu <yilun.xu@intel.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Cc: Alexey Kardashevskiy <aik@amd.com>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> 
> Superficial comments inline - noticed whilst getting my head
> around the basic structure.
> 
> 
> 
> 
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..f74de0ee49a0
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
> > @@ -0,0 +1,346 @@
> 
> > +
> > +DEFINE_FREE(req_free, struct pci_tsm_req *, if (_T) tsm_ops->req_free(_T))
> > +
> > +static int pci_tsm_disconnect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm_req *req __free(req_free) = NULL;
> 
> As below. I'll stop commenting on these.

Hey, they are fair game, will find a way to rework this and not use the
confusing pattern.

> > +
> > +	/* opportunistic state checks to skip allocating a request */
> > +	if (pdev->tsm->state < PCI_TSM_CONNECT)
> > +		return 0;
> > +
> > +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_DISCONNECT);
> > +	if (!req)
> > +		return -ENOMEM;
> > +
> > +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> > +		enum pci_tsm_op_status status;
> > +
> > +		/* revalidate state */
> > +		if (pdev->tsm->state < PCI_TSM_CONNECT)
> > +			return 0;
> > +		if (pdev->tsm->state < PCI_TSM_INIT)
> > +			return -ENXIO;
> > +
> > +		do {
> > +			status = tsm_ops->exec(pdev, req);
> > +			req->seq++;
> > +			/* TODO: marshal SPDM request */
> > +		} while (status == PCI_TSM_SPDM_REQ);
> > +
> > +		if (status == PCI_TSM_FAIL)
> > +			return -EIO;
> > +		pdev->tsm->state = PCI_TSM_INIT;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int pci_tsm_connect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm_req *req __free(req_free) = NULL;
> 
> Push down a few lines to put it where the allocation happens.
> 
> > +
> > +	/* opportunistic state checks to skip allocating a request */
> > +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +		return 0;
> > +
> > +	req = tsm_ops->req_alloc(pdev, PCI_TSM_OP_CONNECT);
> > +	if (!req)
> > +		return -ENOMEM;
> > +
> > +	scoped_cond_guard(mutex_intr, return -EINTR, tsm_ops->lock) {
> 
> What could possibly go wrong? Everyone loves scoped_cond_guard ;)

Indeed.

> 
> > +		enum pci_tsm_op_status status;
> > +
> > +		/* revalidate state */
> > +		if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +			return 0;
> > +		if (pdev->tsm->state < PCI_TSM_INIT)
> > +			return -ENXIO;
> > +
> > +		do {
> > +			status = tsm_ops->exec(pdev, req);
> > +			req->seq++;
> > +		} while (status == PCI_TSM_SPDM_REQ);
> > +
> > +		if (status == PCI_TSM_FAIL)
> > +			return -EIO;
> > +		pdev->tsm->state = PCI_TSM_CONNECT;
> > +	}
> > +	return 0;
> > +}
> 
> ...
> 
> > + size_t connect_mode_store(struct device *dev,
> > +				  struct device_attribute *attr,
> > +				  const char *buf, size_t len)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	int i;
> > +
> > +	guard(mutex)(tsm_ops->lock);
> > +	if (pdev->tsm->state >= PCI_TSM_CONNECT)
> > +		return -EBUSY;
> > +	for (i = 0; i < ARRAY_SIZE(pci_tsm_modes); i++)
> > +		if (sysfs_streq(buf, pci_tsm_modes[i]))
> > +			break;
> > +	if (i == PCI_TSM_MODE_LINK) {
> > +		if (pdev->tsm->link_capable)
> > +			pdev->tsm->mode = PCI_TSM_MODE_LINK;
> 
> Error in all real paths paths?

Uh... yeah, did I mention that this untested. Will fix.

> Also, maybe a switch will be more sensible here.

Sure.

> 
> > +		return -EOPNOTSUPP;
> > +	} else if (i == PCI_TSM_MODE_SELECTIVE) {
> > +		if (pdev->tsm->selective_capable)
> > +			pdev->tsm->mode = PCI_TSM_MODE_SELECTIVE;
> > +		return -EOPNOTSUPP;
> > +	} else
> > +		return -EINVAL;
> > +	return len;
> > +}
> 
> 
> > +
> > +void pci_tsm_init(struct pci_dev *pdev)
> > +{
> > +	u16 ide_cap;
> > +	int rc;
> > +
> > +	if (!pdev->cma_capable)
> > +		return;
> > +
> > +	ide_cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_IDE);
> > +	if (!ide_cap)
> > +		return;
> > +
> > +	struct pci_tsm *tsm __free(kfree) = kzalloc(sizeof(*tsm), GFP_KERNEL);
> > +	if (!tsm)
> > +		return;
> > +
> > +	tsm->ide_cap = ide_cap;
> > +
> > +	rc = xa_insert(&pci_tsm_devs, (unsigned long)pdev, pdev, GFP_KERNEL);
> 
> This is an unusual pattern with the key and the value matching.
> Feels like xarray might for once not be the best choice of structure.

This is the "use xarray instead of a linked list patterni". It would be
useful to maybe make the key be the Segment+BDF, but I did not take the
time to figure out if that can fit in an unsigned long. In the meantime
this saves needing to embed a linked list node in 'struct pci_dev'.

[..]
> > diff --git a/drivers/virt/coco/tsm/class.c b/drivers/virt/coco/tsm/class.c
> > index a569fa6b09eb..a459e51c0892 100644
> > --- a/drivers/virt/coco/tsm/class.c
> > +++ b/drivers/virt/coco/tsm/class.c
> 
> > +static const struct attribute_group *tsm_attr_groups[] = {
> > +#ifdef CONFIG_PCI_TSM
> > +	&tsm_pci_attr_group,
> > +#endif
> > +	NULL,
> Trivial but, no point in that comma as we will never chase it with anything.
> > +};
> > +
> >  static int __init tsm_init(void)
> >  {
> >  	tsm_class = class_create("tsm");
> > @@ -86,6 +97,7 @@ static int __init tsm_init(void)
> >  		return PTR_ERR(tsm_class);
> >  
> >  	tsm_class->dev_release = tsm_release;
> > +	tsm_class->dev_groups = tsm_attr_groups;
> >  	return 0;
> >  }
> >  module_init(tsm_init)
> > diff --git a/drivers/virt/coco/tsm/pci.c b/drivers/virt/coco/tsm/pci.c
> > new file mode 100644
> > index 000000000000..b3684ad7114f
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm/pci.c
> 
> > +
> > +static bool tsm_pci_group_visible(struct kobject *kobj)
> > +{
> > +	struct device *dev = kobj_to_dev(kobj);
> > +	struct tsm_subsys *subsys = container_of(dev, typeof(*subsys), dev);
> Give this a macro probably.
>  dev_to_tsm_subsys(kobj_to_dev(kobj));

Sure.

> 
> > +
> > +	if (subsys->info->pci_ops)
> > +		return true;
> 
> 	return subsys->info->pci->ops;
> maybe

True, maybe with a !! for good measure.

[..]
> > diff --git a/drivers/virt/coco/tsm/tsm.h b/drivers/virt/coco/tsm/tsm.h
> > new file mode 100644
> > index 000000000000..407c388a109b
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm/tsm.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_CORE_H
> > +#define __TSM_CORE_H
> > +
> > +#include <linux/device.h>
> > +
> > +struct tsm_info;
> > +struct tsm_subsys {
> > +	struct device dev;
> > +	const struct tsm_info *info;
> > +};
> 
> I know people like to build up patch sets, but I think defining this here
> from patch 3 would just reduce noise.

Ok.

> 
> > +
> > +#ifdef CONFIG_PCI_TSM
> > +int tsm_pci_init(const struct tsm_info *info);
> > +void tsm_pci_destroy(const struct tsm_info *info);
> > +extern const struct attribute_group tsm_pci_attr_group;
> > +#else
> > +static inline int tsm_pci_init(const struct tsm_info *info)
> > +{
> > +	return 0;
> > +}
> > +static inline void tsm_pci_destroy(const struct tsm_info *info)
> > +{
> > +}
> > +#endif
> > +
> > +#endif /* TSM_CORE_H */
> 
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > index 8cb8a661ba41..f5dbdfa65d8d 100644
> > --- a/include/linux/tsm.h
> > +++ b/include/linux/tsm.h
> > @@ -4,11 +4,15 @@
> >  
> >  #include <linux/sizes.h>
> >  #include <linux/types.h>
> > +#include <linux/mutex.h>
> 
> struct mutex;
> instead given you only have a pointer I think.

True, but see below I expect this lock to move somewhere else in the
next version.

> 
> 
> >  
> >  struct tsm_info {
> >  	const char *name;
> >  	struct device *host;
> >  	const struct attribute_group **groups;
> > +	const struct tsm_pci_ops *pci_ops;
> > +	unsigned int nr_selective_streams;
> > +	unsigned int link_stream_capable:1;
> >  };
> 
> 
> > +struct pci_dev;
> > +/**
> > + * struct tsm_pci_ops - Low-level TSM-exported interface to the PCI core
> > + * @add: accept device for tsm operation, locked
> > + * @del: teardown tsm context for @pdev, locked
> > + * @req_alloc: setup context for given operation, unlocked
> > + * @req_free: teardown context for given request, unlocked
> > + * @exec: run @req, may be invoked multiple times per @req, locked
> > + * @lock: tsm work is one device and one op at a time
> > + */
> > +struct tsm_pci_ops {
> > +	int (*add)(struct pci_dev *pdev);
> > +	void (*del)(struct pci_dev *pdev);
> > +	struct pci_tsm_req *(*req_alloc)(struct pci_dev *pdev,
> > +					 enum pci_tsm_op op);
> > +	struct pci_tsm_req *(*req_free)(struct pci_tsm_req *req);
> > +	enum pci_tsm_op_status (*exec)(struct pci_dev *pdev,
> > +				       struct pci_tsm_req *req);
> > +	struct mutex *lock;
> 
> Mixing data with what would otherwise be likely to be constant pointers
> probably best avoided.  Maybe wrap the lock in another op instead?

After chatting with Alexey this lock is too coarse and will move to a
per device lock rather than locking the entire interface.