Alexey Kardashevskiy wrote:
> On 30/1/24 20:24, 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 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.
>
>
> It's a good start but I am still digesting this scaffolding.
>
[..]
> > diff --git a/Documentation/ABI/testing/sysfs-class-tsm b/Documentation/ABI/testing/sysfs-class-tsm
> > index 304b50b53e65..77957882738a 100644
> > --- a/Documentation/ABI/testing/sysfs-class-tsm
> > +++ b/Documentation/ABI/testing/sysfs-class-tsm
> > @@ -10,3 +10,26 @@ Description:
> > For software TSMs instantiated by a software module, @host is a
> > directory with attributes for that TSM, and those attributes are
> > documented below.
> > +
> > +
> > +What: /sys/class/tsm/tsm0/pci/link_capable
> > +Date: January, 2024
> > +Contact: linux-coco@xxxxxxxxxxxxxxx
> > +Description:
> > + (RO) When present this returns "1\n" to indicate that the TSM
> > + supports establishing Link IDE with a given root-port attached
> > + device. See "tsm/connect_mode" in
> > + Documentation/ABI/testing/sysfs-bus-pci
>
> I am struggling to make sense of "a given root-port attached device".
> There is one CCP device on AMD SEV and therefore one /sys/class/tsm/tsmX
> but still many root ports. How do root ports relate to /sys/class/tsm/tsm0 ?
This is a property of the TSM for whether it supports Link IDE,
Selective IDE, or both. Since Link IDE is a point-to-point protocol, the
TSM can only report whether Link IDE is available for Link IDE capable
root-ports.
For example, I believe some platform vendors are only supporting
Selective IDE with their TSM, while others additionally support Link
IDE.
For simplicity, I will likely drop Link IDE details out of this proposal
as most known use cases specify Selective IDE. Link IDE complexity can
always be added back later.
> > +
> > +
> > +What: /sys/class/tsm/tsm0/pci/selective_streams
> > +Date: January, 2024
> > +Contact: linux-coco@xxxxxxxxxxxxxxx
> > +Description:
> > + (RO) When present this returns the number of currently available
> > + selective IDE streams available to the TSM. When a stream id is
> > + allocated this number is decremented and a link to the PCI
> > + device(s) consuming the stream(s) appears alonside this
>
> s/alonside/alongside/
thx.
> > + attribute in the /sys/class/tsm/tsm0/pci/ directory. See
> > + "tsm/connect" and "tsm/connect_mode" in
> > + Documentation/ABI/testing/sysfs-bus-pci.
> > diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> > index a5c3cadddd6f..11d788038d19 100644
> > --- a/drivers/pci/Kconfig
> > +++ b/drivers/pci/Kconfig
> > @@ -129,6 +129,21 @@ config PCI_CMA
> > A PCI DOE mailbox is used as transport for DMTF SPDM based
> > authentication, measurement and secure channel establishment.
> >
> > +config PCI_TSM
> > + bool "TEE Security Manager for Device Security"
>
> (discussed elsewhere, I'll rant here once more and then will shut up)
>
> It is bool and not tristate :(
Yes.
> CMA, DOE are the same, quite annoying (as in these early days I am
> adding printks here and there and rmmod+modpbobe saves time but builtins
> mean reboot) and imho no really necessary as (from 4/5) "only next
> generation server hosts will start to include a platform TSM".
A couple observations:
* A significant portion of the overall TSM driver complexity lies in
low-level the platform vendor specific init paths. Those
initializations paths happen on demand via a loadable module.
Additionally the successful outcome of cross-vendor collaboration is
that the vendor specific runtime paths stay thin, and those will also be
included the demand loaded vendor platform module.
* The vmlinux .text size increase to turn on CONFIG_PCI_CMA and
CONFIG_PCI_TSM is in the noise (0.01% increase). The runtime memory
utilization when the feature is not used is just a pointer in a 'struct
pci_dev', i.e. only a few hundred bytes for a reasonable laptop.
So the cost of maintaining this facility in the PCI core is negligible
from a memory overhead overhead perspective and the bulk of the
complexity that would benefit from being runtime replaceable as a module
(low level TSM driver) remains modular.
For that small cost we gain a facility that is easy for the rest of the
kernel to reason about and audit. All authentication state linked off of
the PCI device, all sysfs attributes centrally defined. The fundamental
object lifetime and sysfs lifetime is still pci_init_capabilities() and
pci_destroy_dev(), and TSM facilities can be tightly coupled with CMA
which had zero concerns being added to the PCI core.
[..]
> > 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 @@
> > +// 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/tsm.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/xarray.h>
> > +#include "pci.h"
> > +
> > +/* collect tsm capable devices to rendezvous with the tsm driver */
> > +static DEFINE_XARRAY(pci_tsm_devs);
>
> Not used anywhere.
pci_tsm_register(), pci_tsm_unregister(), pci_tsm_init()
[..]
> > +static int pci_tsm_add(struct pci_dev *pdev)
>
> Nothing checks the returned value.
Yeah, it turns out that if the tsm rejects the device for whatever
reason then its state will never advance past PCI_TSM_INIT. Likely I
will make pci_tsm_add() return void in the next round.
[..]
> > 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
[..]
> > @@ -74,4 +78,77 @@ int tsm_report_register(const struct tsm_report_ops *ops, void *priv,
> > int tsm_report_unregister(const struct tsm_report_ops *ops);
> > int tsm_register(const struct tsm_info *info);
> > void tsm_unregister(const struct tsm_info *info);
> > +
> > +enum pci_tsm_op_status {
> > + PCI_TSM_FAIL = -1,
> > + PCI_TSM_OK,
> > + PCI_TSM_SPDM_REQ,
>
> Secure SPDM is also needed here. In my toy TSM project [1] I am just
> using negatives for errors, 0 for "successfully finished" and positives
> for a DOE protocol (1 for SPDM, 2 for Secure SPDM), seems alright as it
> is all about PCI anyway (although "pci" is not always present in all
> these enums and structs).
Oh, I thought the SPDM marshaling was opaque so the kernel would not
know or care if it was transporting clear-text or cipher text, I will
take a closer look at your implementation.
> > +};
> > +
> > +enum pci_tsm_op {
> > + PCI_TSM_OP_CONNECT,
> > + PCI_TSM_OP_DISCONNECT,
> > +};
> > +
> > +struct pci_tsm_req {
> > + enum pci_tsm_op op;
> > + unsigned int seq;
>
> @seq is not tested anywhere.
>
> May be move (*req_free) here.
Say more, why does each request needs its own custom way to free a
request? I am following the lead of proven mechanisms like bio_put() and
bio_free() where the bio does not have its own ->bio_free() callback.
> > +};
> > +
> > +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);
>
> The pci_tsm_req is just an @op, three hooks seems to be more than
> needed, could be just (*exec)(struct pci_dev *pdev, enum pci_tsm_op op).
The rationale for other ops beyond exec is to keep the locking context
clear.
That said I do need to rework the locking to make it finer grained, and
allow multiple physical functions to be acted upon at the same time.
> Or the idea is to extend pci_tsm_req with some void *platform_req_data,
> is not it?
Exactly. req_alloc() is where the low level TSM driver gets to append
its data to the request before execution, and req_free() lets the low
level TSM driver unwind that.
> There is one "op" in flight per a physical device allowed in
> SEV TIO, I suspect that is likely to be the case for others so such data
> can be managed by the platform code in the platform data of a TEE-IO device.
Yes, I think one-op per device is a common trait that can be managed by
the core.
[..]
> > +struct pci_tsm {
> > + enum pci_tsm_state state;
> > + enum pci_tsm_mode mode;
>
> Does it have to be either mode and cannot be both?
Yes, mode is "selective" vs "link" IDE. The TDISP spec also allows for a
third option of "TSM deems no IDE needed". However, in terms of what the
user can pick it needs to be "selective" if they want TEE I/O operation.
...and a TSM is free to not implement support for the Link IDE so that
capability can be hidden.
