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

[Alexey Kardashevskiy <aik@amd.com>]

On 29/8/25 11:06, dan.j.williams@intel.com wrote:
> 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.

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.


> [..]
>>> +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.


> 
> 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?

>>> +	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.

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

>>> +		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.


Nah, I meant that the is_link_tsm() and is_devsec_tsm() checks below will fail if ops==NULL.

> 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.


> 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.


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

>>> +	/*
>>> +	 * 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.


>>> +		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]

sorry, my bad, I am typing the answers all day as I go with the merge, missed this one. Thanks, :)

> 
> PCI_EXP_DEVCAP_TEE indicates potential presence of a DSM.



-- 
Alexey