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

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

Alexey Kardashevskiy wrote:
[..]
> >> 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.
> 
> okay, then may be put a comment there so when I forget and will be about to ask this question again - I'll see the comment and stop.

Done.

diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
index 8fa51eb6dd05..c25935a6c059 100644
--- a/drivers/pci/tsm.c
+++ b/drivers/pci/tsm.c
@@ -180,6 +180,12 @@ static int pci_tsm_connect(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
         * dependent functions. Failure to probe a function is not fatal
         * to connect(), it just disables subsequent security operations
         * for that function.
+        *
+        * Note this is done unconditionally, without regard to finding
+        * PCI_EXP_DEVCAP_TEE on the dependent function, for robustness. The DSM
+        * is the ultimate arbiter of security state relative to a given
+        * interface id, and if it says it can manage TDISP state of a function,
+        * let it.
         */
        pci_tsm_walk_fns(pdev, probe_fn, pdev);
        return 0;

> > [..]
> >>> +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.
> 
> But how? There is this pci_tsm_link_count counter which controls whether "connect" is present or not.
> "if (!WARN_ON_ONCE(is_link_tsm(tsm_dev)))" at least.

In "[PATCH 5/7] PCI/TSM: Add Device Security (TVM Guest) operations
support" [1], it does this:

 static bool pci_tsm_group_visible(struct kobject *kobj)
 {
-       return pci_tsm_link_group_visible(kobj);
+       return pci_tsm_link_group_visible(kobj) ||
+              pci_tsm_devsec_group_visible(kobj);
 }
 DEFINE_SYSFS_GROUP_VISIBLE(pci_tsm);

...which means if both "link" and "devsec" TSMs registered, userspace could
attempt "connect" with a "devsec" TSM.

[1]: http://lore.kernel.org/20250827035259.1356758-6-dan.j.williams@intel.com

This property of being able to register both "link" and "devsec" TSMs at
the same time is useful for testing.

> > 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.
> > 
> > ??
> If there is no "IDE", what do we "connect" then? This is all about PCI now, can we have active TDISP without IDE?

TDISP without IDE still needs to do all of SPDM (Component Measurement and
Authentication), and the TDISP state machine.

> >>> +	pci_dbg(pdev, "Device Security Manager detected (%s%s%s)\n",
> >>> +		pdev->ide_cap ? "IDE" : "", pdev->ide_cap && tee ? " " : "",
> > 
> > IDE cap is checked here.
> 
> "connect" appears regardless IDE presence.
> 
> > IDE is not mandatory for TDISP.
> 
> Is PCI_EXP_DEVCAP_TEE mandatory for IDE? I have seen a multifunction
> device with no SRIOV on PF0 (only IDE) but SRIOV on PF1 (and those VFs
> have PCI_EXP_DEVCAP_TEE). Not sure PF0 has PCI_EXP_DEVCAP_TEE there or
> has to have it.

IDE without TDISP does only needs ide_cap and is_pci_tsm_pf0() says "yes"
if either is set.

> Do you show "connect" in the VM too? There will be PCI_EXP_DEVCAP_TEE but not IDE.

The "connect" attribute only shows if the VM loads a "link" TSM driver.
This should not happen outside of testing, but if it does that is handled
by being explicit about which TSM device is passed to the attribute.

> >>> +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.
> 
> Nah, I meant that the is_link_tsm() and is_devsec_tsm() checks below will fail if ops==NULL.

Oh, yes, deleted.

> 
> > 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.
> 
> 
> Burns time too to read the code to realize that pci_tsm_register() does
> nothing really for the guest at all - the counter is not used (at least
> here) and other checks are very likely to pass and the function does not
> really register anything even on the host.

This was done with "[PATCH 5/7] PCI/TSM: Add Device Security (TVM Guest) operations
support" in mind. I am happy to move more of that complexity into the later
patch where it makes more sense when two TSM device cases are being handled
in the same path.

> > 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.
> 
> Ah fair point. It is just that when I saw "link", I had a flashback -
> "link" vs "selective". imho "phys" or "bare" would do better but I do not
> insist.

I picked "link" to try to encompass both a physical PCIe link, and a
logical "link" representing the SPDM session. Maybe "transport"... "link"
is still my frontrunner.

[..]
> > 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.
> It just seems rather useless to have pdev back refs if you still pass pdev everywhere.

The pdev is not passed everywhere, e.g. to_pci_tsm_pf0() (for type
checking) and tsm_remove() (for simplifying cleanup).  I.e. some places
only have 'struct pci_tsm'.

> >>> +	/*
> >>> +	 * 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.
> It is "both" pretty much always.
> 
> > 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.
> 
> pci_tsm_devsec_ops seems more reasonable then.

Changed.

[..]