Re: [PATCH v5 04/10] PCI/TSM: Authenticate devices via platform TSM

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

Alexey Kardashevskiy wrote:
[..]
> > +/*
> > + * Count of TSMs registered that support physical link operations vs device
> > + * security state management.
> > + */
> > +static int pci_tsm_link_count;
> > +static int pci_tsm_devsec_count;
> 
> This one is not checked anywhere.

Yes, included here for symmetry, it gets used by the devsec_tsm
enabling.

[..]
> > +static int probe_fn(struct pci_dev *pdev, void *dsm)
> > +{
> > +	struct pci_dev *dsm_dev = dsm;
> > +	const struct pci_tsm_ops *ops = dsm_dev->tsm->ops;
> > +
> > +	pdev->tsm = ops->probe(pdev);
> 
> 
> Looks like this is going to initialize pci_dev::tsm for all VFs under
> an IDE (or TEE) capable PF0, even for those VFs which do not have
> PCI_EXP_DEVCAP_TEE, which does not seem right.

Maybe, but it limits the flexibility of a DSM for no pressing reason.
The spec only talks about that bit being set for Endpoint Upstream Ports
and Root Ports. In the guest, QEMU is emulating / mediating, the config
space of Endpoint Upstream Ports.

If the DSM accepts that interface-ID for TDISP requests then the bit
being set on the SRIOV or downstream interface device does not matter.
So the initialization is only to allow future DSM request attempts, it
is not making a claim about those DSM attempts being successful.

Effectively, Linux can be robust, liberal in what it accepts, with no
significant cost that I can currently see.

[..]
> > +static ssize_t connect_store(struct device *dev, struct device_attribute *attr,
> > +			     const char *buf, size_t len)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct tsm_dev *tsm_dev;
> > +	int rc, id;
> > +
> > +	rc = sscanf(buf, "tsm%d\n", &id);
> > +	if (rc != 1)
> > +		return -EINVAL;
> > +
> > +	ACQUIRE(rwsem_write_kill, lock)(&pci_tsm_rwsem);
> > +	if ((rc = ACQUIRE_ERR(rwsem_write_kill, &lock)))
> > +		return rc;
> > +
> > +	if (pdev->tsm)
> > +		return -EBUSY;
> > +
> > +	tsm_dev = find_tsm_dev(id);
> > +	if (!is_link_tsm(tsm_dev))
> 
> There would be no "connect" in sysfs if !is_link_tsm().

Given this implementation makes no restriction on number or type of TSMs
the "connect" attribute could theoretically be visible and a
"!is_link_tsm()" instance could be requested via this interface.

This is the case with the sample smoke test:

http://lore.kernel.org/20250827035259.1356758-8-dan.j.williams@intel.com

[..]
> > +static void pf0_sysfs_enable(struct pci_dev *pdev)
> > +{
> > +	bool tee = pdev->devcap & PCI_EXP_DEVCAP_TEE;
> 
> IDE cap, not PCI_EXP_DEVCAP_TEE.

??

> > +	pci_dbg(pdev, "Device Security Manager detected (%s%s%s)\n",
> > +		pdev->ide_cap ? "IDE" : "", pdev->ide_cap && tee ? " " : "",

IDE cap is checked here.

IDE is not mandatory for TDISP.

> > +		tee ? "TEE" : "");
> > +
> > +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_auth_attr_group);
> > +	sysfs_update_group(&pdev->dev.kobj, &pci_tsm_attr_group);
> > +}
> > +
> > +int pci_tsm_register(struct tsm_dev *tsm_dev)
> > +{
> > +	struct pci_dev *pdev = NULL;
> > +
> > +	if (!tsm_dev)
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * The TSM device must have pci_ops, and only implement one of link_ops
> > +	 * or devsec_ops.
> > +	 */
> > +	if (!tsm_pci_ops(tsm_dev))
> > +		return -EINVAL;
> 
> Not needed.

At this point pci_tsm_register() is an exported symbol, it is ok for it
to do input validation and document the interface.

However, given the realization in the other thread about
tsm_ide_stream_register() not needing to be exported this too does not
need to be exported and can assume that ops are always set by in-kernel
callers.
 
> > +	if (!is_link_tsm(tsm_dev) && !is_devsec_tsm(tsm_dev))
> > +		return -EINVAL;
> > +
> > +	if (is_link_tsm(tsm_dev) && is_devsec_tsm(tsm_dev))
> > +		return -EINVAL;
> > +
> > +	guard(rwsem_write)(&pci_tsm_rwsem);
> > +
> > +	/* on first enable, update sysfs groups */
> > +	if (is_link_tsm(tsm_dev) && pci_tsm_link_count++ == 0) {
> > +		for_each_pci_dev(pdev)
> > +			if (is_pci_tsm_pf0(pdev))
> > +				pf0_sysfs_enable(pdev);
> 
> You could safely run this loop in the guest too, is_pci_tsm_pf0() would not find IDE-capable PF.

I think it burns a reader's time to look at the top-level loop that
always runs in the guest and realize only after digging deep that the
whole thing is a nop because IDE-capable PF is never set.

Also recall that IDE is optional.

> > +	} else if (is_devsec_tsm(tsm_dev)) {
> > +		pci_tsm_devsec_count++;
> > +	}
> 
> nit: a bunch of is_link_tsm()/is_devsec_tsm() hurts to read.
> 
> Instead of routing calls to pci_tsm_register() via tsm_register() and
> doing all these checks here, we could have cleaner
> pci_tsm_link_register() and pci_tsm_devsev_register() and call those
> directly from where tsm_register() is called as those TSM drivers (or
> devsec samples) know what they are.

I am not opposed to a front end for the TSM drivers to have a:

pci_tsm_<type>_register()

...rather than

tsm_register(..., <type-specific ops>)

...but that does not really effect the backend where the TSM core
interfaces with the PCI core especially because they called at making
the "tsm/" sysfs group visible.

> (well, I'd love pci_tsm_{host|guest}_register or pci_tsm_{hv|vm}_register as their roles are distinct...)

I pulled back from "host" / "guest" or "hv/vm" when you and Jason
highlighted this non TVM case:

http://lore.kernel.org/926022a3-3985-4971-94bd-05c09e40c404@amd.com

So the names of the facilities should match what they do, not who uses
them. A Link TSM manages sessions and physical links details and a
Devsec TSM manages security state transitions and acceptance.

[..]
> > +/*
> > + * struct pci_tsm_ops - manage confidential links and security state
> > + * @link_ops: Coordinate PCIe SPDM and IDE establishment via a platform TSM.
> > + *	      Provide a secure session transport for TDISP state management
> > + *	      (typically bare metal physical function operations).
> > + * @sec_ops: Lock, unlock, and interrogate the security state of the
> > + *	     function via the platform TSM (typically virtual function
> > + *	     operations).
> > + * @owner: Back reference to the TSM device that owns this instance.
> > + *
> > + * This operations are mutually exclusive either a tsm_dev instance
> > + * manages physical link properties or it manages function security
> > + * states like TDISP lock/unlock.
> > + */
> > +struct pci_tsm_ops {
> > +	/*
> > +	 * struct pci_tsm_link_ops - Manage physical link and the TSM/DSM session
> > +	 * @probe: allocate context (wrap 'struct pci_tsm') for follow-on link
> > +	 *	   operations
> > +	 * @remove: destroy link operations context
> > +	 * @connect: establish / validate a secure connection (e.g. IDE)
> > +	 *	     with the device
> > +	 * @disconnect: teardown the secure link
> > +	 *
> > +	 * Context: @probe, @remove, @connect, and @disconnect run under
> > +	 * pci_tsm_rwsem held for write to sync with TSM unregistration and
> > +	 * mutual exclusion of @connect and @disconnect. @connect and
> > +	 * @disconnect additionally run under the DSM lock (struct
> > +	 * pci_tsm_pf0::lock) as well as @probe and @remove of the subfunctions.
> > +	 */
> > +	struct_group_tagged(pci_tsm_link_ops, link_ops,
> 
> Why not pci_tsm_dsm_ops? DSM and TDI are used all over the place in the PCIe spec, why not use those?

Because the acronym soup is already unmanageable, please lets pick human
readable names for major concepts.

> > +		struct pci_tsm *(*probe)(struct pci_dev *pdev);
> > +		void (*remove)(struct pci_tsm *tsm);
> > +		int (*connect)(struct pci_dev *pdev);
> 
> Why is this one not pci_tsm?

Unlike probe/remove which have an alloc/free relationship with each
other, connect/disconnect operate on the device. That said I think it is
arbitrary and have no real concern about flipping it if you can get
Aneesh or Yilun to weigh in as well about their preference.

> > +	/*
> > +	 * struct pci_tsm_security_ops - Manage the security state of the function
> > +	 * @lock: probe and initialize the device in the LOCKED state
> > +	 * @unlock: destroy TSM context and return device to UNLOCKED state
> > +	 *
> > +	 * Context: @lock and @unlock run under pci_tsm_rwsem held for write to
> > +	 * sync with TSM unregistration and each other
> > +	 */
> > +	struct_group_tagged(pci_tsm_security_ops, devsec_ops,
> 
> Why not pci_tsm_tdi_ops? Or even pci_tdi_ops? pci_tsm_link_ops::connect is also about security.

On some of this feedback I can not tell if you are asking for
understanding and asking for code changes and find the current naming
unacceptable.

In this case for a major concept I want a name and not an acronym. I am
open to better names if you have suggestions, but please lets not use
acronyms for something fundamental like the split between TSM
personalities.

> > +		struct pci_tsm *(*lock)(struct pci_dev *pdev);
> 
> pci_tdi?
> 
> Or why have pci_dev reference in pci_tsm and pci_tdi then.
> 
> > +		void (*unlock)(struct pci_dev *pdev);
> 
> 
> So we put host and guest in the same ops anyway, what does it buy us?

Identical lookup mechanism pdev->tsm->ops as there's no need to waste
another ops pointer.

[..]
> > +{
> > +	if (!pci_is_pcie(pdev))
> > +		return false;
> > +
> > +	if (pdev->is_virtfn)
> > +		return false;
> > +
> > +	/*
> > +	 * Allow for a Device Security Manager (DSM) associated with function0
> > +	 * of an Endpoint to coordinate TDISP requests for other functions
> > +	 * (physical or virtual) of the device, or allow for an Upstream Port
> > +	 * DSM to accept TDISP requests for the Endpoints downstream of the
> > +	 * switch.
> > +	 */
> > +	switch (pci_pcie_type(pdev)) {
> > +	case PCI_EXP_TYPE_ENDPOINT:
> > +	case PCI_EXP_TYPE_UPSTREAM:
> > +	case PCI_EXP_TYPE_RC_END:
> > +		if (pdev->ide_cap || (pdev->devcap & PCI_EXP_DEVCAP_TEE))
> 
> PCI_EXP_DEVCAP_TEE says nothing about "connect".

[You already said "Thanks," above and then did not trim your reply, so I
almost missed this, please trim]

PCI_EXP_DEVCAP_TEE indicates potential presence of a DSM.