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

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

Jonathan Cameron wrote:
> On Thu, 05 Dec 2024 14:23:45 -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 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. 
> 
> I don't follow the previous sentence. Perhaps consider a rewrite?

I will just drop it for now as the entanglements with native PCI CMA are
not relevant in this changelog especially as is not clear which series
will land first.

The rewrite would have been:

--
CONFIG_PCI_TSM adds an "authenticated" attribute and "tsm/" subdirectory
to pci-sysfs. If native authentication is enabled then the existing
"authenticated" attribute is replaced. The presence of the "tsm/"
directory indicates what device authentication method is in effect.
--

> 
> > 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>
> A few minor things inline.
> 
> Jonathan
[..]
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> > new file mode 100644
> > index 000000000000..04e9257a6e41
> > --- /dev/null
> > +++ b/drivers/pci/tsm.c
> > @@ -0,0 +1,293 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * TEE Security Manager for the TEE Device Interface Security Protocol
> > + * (TDISP, PCIe r6.1 sec 11)
> > + *
> > + * Copyright(c) 2024 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#define dev_fmt(fmt) "TSM: " fmt
> > +
> > +#include <linux/pci.h>
> > +#include <linux/pci-doe.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/xarray.h>
> > +#include <linux/pci-tsm.h>
> > +#include <linux/bitfield.h>
> 
> Odd header ordering.  Anything consistent is fine by me but
> this just feels random.

Grouped the PCI ones together and X-mas-treed it.

> > +#include "pci.h"
> > +
> > +/*
> > + * Provide a read/write lock against the init / exit of pdev tsm
> > + * capabilities and arrival/departure of a tsm instance
> > + */
> > +static DECLARE_RWSEM(pci_tsm_rwsem);
> > +static const struct pci_tsm_ops *tsm_ops;
> > +
> > +/* supplemental attributes to surface when pci_tsm_attr_group is active */
> > +static const struct attribute_group *pci_tsm_owner_attr_group;
> > +
> > +static int pci_tsm_disconnect(struct pci_dev *pdev)
> > +{
> > +	struct pci_tsm *pci_tsm = pdev->tsm;
> > +
> > +	lockdep_assert_held(&pci_tsm_rwsem)
> > +	if_not_guard(mutex_intr, &pci_tsm->lock)
> 
> Sadly that got dropped.

RIP.

> 
> > +		return -EINTR;
> > +
> > +	if (pci_tsm->state < PCI_TSM_CONNECT)
> > +		return 0;
> > +	if (pci_tsm->state < PCI_TSM_INIT)
> > +		return -ENXIO;
> > +
> > +	tsm_ops->disconnect(pdev);
> > +	pci_tsm->state = PCI_TSM_INIT;
> > +
> > +	return 0;
> > +}
> > +
> 
> > +static struct attribute *pci_tsm_attrs[] = {
> > +	&dev_attr_connect.attr,
> > +	NULL,
> 
> Trivia but no comma given it's a terminator and nothing will
> ever come after it.

Ok.

> 
> 
> > +};
> > +
> > +const struct attribute_group pci_tsm_attr_group = {
> > +	.name = "tsm",
> > +	.attrs = pci_tsm_attrs,
> > +	.is_visible = SYSFS_GROUP_VISIBLE(pci_tsm),
> > +};
> > +
> > +static ssize_t authenticated_show(struct device *dev,
> > +				  struct device_attribute *attr, char *buf)
> > +{
> > +	/*
> > +	 * When device authentication is TSM owned, 'authenticated' is
> > +	 * identical to the connect state.
> > +	 */
> > +	return connect_show(dev, attr, buf);
> > +}
> > +static DEVICE_ATTR_RO(authenticated);
> > +
> > +static struct attribute *pci_tsm_auth_attrs[] = {
> > +	&dev_attr_authenticated.attr,
> > +	NULL,
> no comma

Ok.

> 
> > +};
> 
> 
> > +void pci_tsm_destroy(struct pci_dev *pdev)
> > +{
> > +	guard(rwsem_write)(&pci_tsm_rwsem);
> > +	__pci_tsm_destroy(pdev);
> > +}
> > +
> > +void pci_tsm_unregister(const struct pci_tsm_ops *ops)
> 
> I'd try to name things so it is clearer when a function
> is about the TSM coming and going vs a particular PCI
> device coming and going after the TSM is loaded.

I will add "core" to the ones that are registering ops from the TSM core
to the PCI core.

> At least that's what I'm assuming is the difference between
> pci_tsm_unregister() tsm going vs
> pci_tsm_destroy() particular PCI device driver being unbound
> (which I don't think gets called, so maybe drop for now?)

pci_destroy_dev() calls it.