Re: [PATCH v3 11/21] PCI/TSM: Support connecting to PCIe CMA devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional overwrite of `pdev->tsm` in `probe_fn()` causes memory leak and state corruption for independent sibling CMA hosts.
- [High] Expanding `is_pci_tsm_host()` to return true for dependent functions with CMA breaks DSM resolution, causing type confusion and preventing context cleanup.

Pre-existing issues:
- [High] AB-BA lock inversion between `pci_bus_sem` and `tsm_host->lock` in `__pci_tsm_disconnect` for upstream switch ports.
--

commit 1f366330a43aef4dbb8b7e338fe088b357fa13a0
Author: Alistair Francis <alistair.francis@wdc.com>

PCI/TSM: Support connecting to PCIe CMA devices

This patch adds support for connecting to PCIe CMA devices by modifying
is_pci_tsm_host() to allow CMA capable devices. It caches the CMA support
in struct pci_dev to avoid repeated mailbox lookups.

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> index 10c9c6696624d..440f818ac5699 100644
> --- a/drivers/pci/tsm.c
> +++ b/drivers/pci/tsm.c

[ ... ]

> @@ -456,7 +475,7 @@ static void __pci_tsm_disconnect(struct pci_dev *pdev)
>  	 * teardown
>  	 */
>  	guard(mutex)(&tsm_host->lock);
> -	pci_tsm_walk_fns_reverse(pdev, remove_fn, NULL);
> +	pci_tsm_walk_fns_reverse(pdev, remove_fn, pdev);

[Severity: High]
This is a pre-existing issue, but does __pci_tsm_disconnect() create an
AB-BA lock inversion between pci_bus_sem and tsm_host->lock?

First, pci_tsm_unbind_all() calls pci_tsm_walk_fns_reverse(). For upstream
switch ports, this invokes pci_walk_bus_reverse(), acquiring pci_bus_sem
for read. Inside the walk, the callback __pci_tsm_unbind() acquires
tsm_host->lock, establishing a pci_bus_sem -> tsm_host->lock ordering.

Immediately after, __pci_tsm_disconnect() explicitly acquires tsm_host->lock
via guard(mutex), and then calls pci_tsm_walk_fns_reverse() (which invokes
pci_walk_bus_reverse()), attempting to acquire pci_bus_sem for read.

This reverses the acquisition order to tsm_host->lock -> pci_bus_sem, which
could lead to a deadlock when racing with bus modifications.

>  	ops->disconnect(pdev);
>  }

> diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> index 950e2c36a4cae..f504fa6803157 100644
> --- a/include/linux/pci-tsm.h
> +++ b/include/linux/pci-tsm.h

[ ... ]

> @@ -142,6 +143,15 @@ static inline bool is_pci_tsm_host(struct pci_dev *pdev)
>  	if (pdev->is_virtfn)
>  		return false;
>  
> +	/*
> +	 * Report capable if CMA is supported, which can be supported on any PCIe
> +	 * device.
> +	 */
> +#ifdef CONFIG_PCI_DOE
> +	if (pdev->doe_cma)
> +		return true;
> +#endif
> +

[Severity: High]
Does expanding is_pci_tsm_host() to return true for dependent functions with
CMA introduce a memory leak and state corruption?

If userspace connects a dependent function (like PF1) as a CMA host first,
pci_tsm_connect() allocates PF1->tsm. If userspace subsequently connects a
TEE host (DSM) on the same device (e.g. PF0), pci_tsm_connect(PF0) walks
dependent functions and calls probe_fn(PF1, PF0).

In drivers/pci/tsm.c:probe_fn(), it unconditionally overwrites pdev->tsm:

	pdev->tsm = ops->probe(dsm_dev->tsm->tsm_dev, pdev);

Could this overwrite the existing CMA TSM context without checking or freeing
it, leaking the memory and corrupting the state?

[Severity: High]
Additionally, does returning true for dependent functions with CMA break
DSM resolution in find_dsm_dev()?

When the parent TEE DSM (PF0) calls probe_fn(PF1), the allocation path
calls find_dsm_dev(PF1). Since is_pci_tsm_host(PF1) now evaluates to true,
find_dsm_dev() incorrectly returns PF1 instead of the actual DSM (PF0).

This causes PF1 to self-identify as its own host, meaning
PF1->tsm->dsm_dev == PF1.

Later, when the parent DSM (PF0) is disconnected, remove_fn() has:

	if (!pdev->tsm || pdev->tsm->dsm_dev != host)
		return 0;

This will skip PF1 because its dsm_dev no longer matches the host, causing
the TEE context to leak. Furthermore, upon device removal,
__pci_tsm_destroy(PF1) would incorrectly treat PF1 as a host and execute the
host-only teardown path (pci_tsm_disconnect), causing type confusion.

>  	/*
>  	 * Allow for a Device Security Manager (DSM) associated with function0
>  	 * of an Endpoint to coordinate TDISP requests for other functions

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901010347.2614656-1-alistair.francis@wdc.com?part=11