Re: [PATCH v6 12/25] PCI/CMA: Add a PCI TSM CMA driver using SPDM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] PCI/CMA: pci_cma_tsm_probe() misattaches subordinate and
  sibling functions
- [Low] PCI/CMA: write-only static variable pci_cma_tsm_dev in
  pci_cma_tsm_init()
--

commit 8928bf8b82ed0687cad0e8da916dd5dcfd9c6e73
Author: Alistair Francis <alistair.francis@wdc.com>

PCI/CMA: Add a PCI TSM CMA driver using SPDM

This commit adds a PCI TSM driver for Component Measurement and
Authentication (CMA-SPDM) over PCI DOE mailboxes, enabling device
authentication via SPDM.

> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> new file mode 100644
> index 0000000000000..3f0c989b52c5d
> --- /dev/null
> +++ b/drivers/pci/cma.c
[ ... ]
> +static struct pci_tsm *pci_cma_tsm_probe(struct tsm_dev *tsm_dev,
> +					 struct pci_dev *pdev)
> +{
> +	struct pci_cma_tsm *cma;
> +	int rc;
> +
> +	if (!pdev->doe_cma || pdev->is_virtfn)
> +		return NULL;
> +
> +	cma = kzalloc(sizeof(*cma), GFP_KERNEL);
> +	if (!cma)
> +		return NULL;
> +
> +	rc = pci_tsm_host_constructor(pdev, &cma->host, tsm_dev);
> +	if (rc) {
> +		kfree(cma);
> +		return NULL;
> +	}
> +
> +	cma->spdm = spdm_create(&pdev->dev, pci_doe_transport, cma->host.doe_mb,
> +				PCI_DOE_MAX_PAYLOAD, NULL);
> +	if (!cma->spdm) {
> +		pci_tsm_host_destructor(&cma->host);
> +		kfree(cma);
> +		return NULL;
> +	}
> +
> +	return &cma->host.base_tsm;
> +}

[Severity: High]
Can pci_cma_tsm_probe() attach subordinate or sibling functions without
authenticating them, and bind sibling functions' pci_tsm_host contexts to
function 0?

pci_cma_tsm_probe() succeeds for any physical function with pdev->doe_cma
set, even when called from probe_fn() for a subordinate function where
pci_cma_tsm_connect() is never run:

drivers/pci/tsm.c:probe_fn() {
	...
	pdev->tsm = ops->probe(dsm_dev->tsm->tsm_dev, pdev);
	pci_dbg(pdev, "setup TSM context: DSM: %s status: %s\n",
		pci_name(dsm_dev), pdev->tsm ? "success" : "failed");
	if (pdev->tsm)
		link_sysfs_enable(pdev);
	return 0;
}

When a TEE-capable function 0 or upstream switch port is connected to
pci_cma_tsm (via pci_tsm_connect() -> pci_tsm_walk_fns()), or when a
sibling PF or downstream device is later initialized via pci_tsm_init()
while function 0 or the upstream port is connected, probe_fn() populates
pdev->tsm without calling spdm_authenticate().  Because authenticated_show()
reports the TSM device name whenever pdev->tsm is non-NULL, sysfs reports
those unauthenticated functions as authenticated and hides their tsm/connect
and tsm/disconnect attributes.

In addition, once function 0 is connected to pci_cma_tsm, is_dsm(pf0) is
true even on a non-TEE multi-function device:

drivers/pci/tsm.c:find_dsm_dev() {
	...
	if (pf0 && pf0 != pdev && is_dsm(pf0))
		return pf0;
	...
}

When a sibling PF is then connected via pci_tsm_connect() ->
pci_tsm_host_constructor() -> pci_tsm_link_constructor(), find_dsm_dev()
sets the sibling PF's base_tsm.dsm_dev to pf0 instead of the sibling PF
itself.  That breaks the struct pci_tsm_host invariant (dsm_dev == pdev),
causing to_pci_tsm_host() and pci_tsm_doe_transfer() on the sibling PF to
use function 0's lock and DOE mailbox, hiding the sibling PF's disconnect
attribute, and causing __pci_tsm_destroy() to tear down the sibling PF via
pci_tsm_fn_exit() without holding tsm_host->lock.

[ ... ]
> +static struct tsm_dev *pci_cma_tsm_dev;
> +
> +static int __init pci_cma_tsm_init(void)
> +{
> +	struct tsm_dev *tsm_dev;
> +
> +	tsm_dev = tsm_register(NULL, &pci_cma_tsm_ops);
> +	if (IS_ERR(tsm_dev))
> +		return PTR_ERR(tsm_dev);
> +
> +	pci_cma_tsm_dev = tsm_dev;
> +	return 0;
> +}

[Severity: Low]
pci_cma_tsm_dev is assigned in pci_cma_tsm_init() but never read anywhere,
which triggers a -Wunused-but-set-variable warning under W=1.

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