Re: [PATCH v5 12/23] PCI/CMA: Add a PCI TSM CMA driver using SPDM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI/CMA: pci_cma_tsm_probe() initializes subfunctions without connect()
--

commit 422c44a59294e74ffe91f6512ceb7d64705e55f8
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 a PCI DOE mailbox to support device
authentication.

> diff --git a/drivers/pci/cma.c b/drivers/pci/cma.c
> new file mode 100644
> index 0000000000000..6b52b129574ec
> --- /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)
> +		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() be invoked via probe_fn() for subordinate or
sibling PCI functions without pci_cma_tsm_connect() ever being called on
them?

In drivers/pci/tsm.c, ops->probe() is called not only on the target host
device in pci_tsm_connect(), but also via probe_fn() when walking
functions in pci_tsm_connect() (when has_tee(pdev) is true) or when SR-IOV
VFs are initialized in pci_tsm_init():

drivers/pci/tsm.c:probe_fn() {
	if (pdev->tsm)
		return 0;

	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;
}

drivers/pci/tsm.c:pci_tsm_connect() {
	...
	if (has_tee(pdev))
		pci_tsm_walk_fns(pdev, probe_fn, pdev);
	return 0;
}

Because pci_cma_tsm_probe() only checks pdev->doe_cma, any walked sibling
PF, downstream device, or VF with doe_cma set will have cma->host and
cma->spdm allocated and assigned to pdev->tsm, followed by
link_sysfs_enable(), without pci_cma_tsm_connect() (and thus
spdm_authenticate()) running on that function.

Doesn't this cause a few issues for those walked functions:

1. In drivers/pci/tsm.c, authenticated_show() delegates to connect_show(),
which reports the TSM device name whenever pdev->tsm is non-NULL, falsely
reporting the unauthenticated function as authenticated in sysfs.

2. Any subsequent attempt by userspace to authenticate that function via
connect_store() will fail with -EBUSY because pdev->tsm is already
populated.

3. For sibling PFs and downstream endpoints where is_pci_tsm_host() is
true, pci_tsm_host_constructor() sets sub_pdev->tsm->dsm_dev = sub_pdev
via find_dsm_dev(). When the parent device later disconnects in
__pci_tsm_disconnect(), remove_fn() skips cleaning up sub_pdev:

drivers/pci/tsm.c:remove_fn() {
	if (!pdev->tsm || pdev->tsm->dsm_dev != host)
		return 0;
	...
}

leaving the unauthenticated sub_pdev->tsm and cma->spdm leaked and active.

4. If a VF (pdev->is_virtfn) has doe_cma set, calling
pci_tsm_host_constructor() on it violates the is_pci_tsm_host()
expectation for struct pci_tsm_host.

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