Re: [PATCH v7 16/16] PCI/TSM: wait for vdevice contexts before removing a DSM

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] PCI/TSM: UAF due to bypassed drain wait in pci_tsm_unregister()
--

commit 70b40a7c8d41eb1bbe070cea07e7f252f47338e8
Author: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>

    PCI/TSM: wait for vdevice contexts before removing a DSM
    
    A PF0 DSM can be removed while a sibling function still has a vdevice.
    The context pins both the pci dev, but those references do not keep the
    PF0 DOE mailbox alive. Ignoring -EBUSY from link disconnect lets PCI
    continue to pci_doe_destroy(), leaving the vdevice with a stale mailbox
    pointer.

> diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm.c
> index c8603867e09df..8aa74ba31932b 100644
> --- a/drivers/pci/tsm.c
> +++ b/drivers/pci/tsm.c
[ ... ]
> @@ -751,8 +769,44 @@ static void __pci_tsm_destroy(struct pci_dev *pdev, struct tsm_dev *tsm_dev)
>  
>  void pci_tsm_destroy(struct pci_dev *pdev)
>  {
> -	guard(rwsem_write)(&pci_tsm_rwsem);
> +	struct pci_tsm_pf0 *pf0 = NULL;
> +	struct completion *drained;
> +	bool interrupted = false;
> +	long rc;
> +
> +	down_write(&pci_tsm_rwsem);
> +	if (pdev->tsm && is_link_tsm(pdev->tsm->tsm_dev) &&
> +	    is_pci_tsm_pf0(pdev)) {
> +		pf0 = to_pci_tsm_pf0(pdev->tsm);
> +		drained = &pf0->contexts_drained;
> +		mutex_lock(&pf0->lock);
> +		pf0->removing = true;
> +		mutex_unlock(&pf0->lock);
> +
> +		/* An unused DSM may never have completed contexts_drained. */
> +		rc = pf0->context_users ?
> +			try_wait_for_completion(drained) : 1;
> +		/* Context release needs the read side of pci_tsm_rwsem. */
> +		up_write(&pci_tsm_rwsem);
> +		while (rc <= 0) {
[ ... ]
> +			if (!rc)
> +				pci_warn(pdev, "TSM connection is in use, waiting for vdevices\n");
> +		}
> +		down_write(&pci_tsm_rwsem);
> +	}
>  	__pci_tsm_destroy(pdev, NULL);
> +	up_write(&pci_tsm_rwsem);
>  }

[Severity: High]
This adds the context drain wait loop when the PCI device is destroyed, but
what happens when the TSM provider device is unbound and calls
pci_tsm_unregister() first?

Looking at pci_tsm_unregister() in drivers/pci/tsm.c, it directly calls
__pci_tsm_destroy(pdev, tsm_dev) bypassing this new wait loop:

drivers/pci/tsm.c:pci_tsm_unregister() {
    ...
	for_each_pci_dev_reverse(pdev)
		__pci_tsm_destroy(pdev, tsm_dev);
}

If contexts are active, __pci_tsm_destroy() skips clearing pdev->tsm
because pci_tsm_disconnect() will return -EBUSY:

drivers/pci/tsm.c:__pci_tsm_destroy() {
    ...
	if (is_link_tsm(tsm_dev) && is_pci_tsm_pf0(pdev)) {
		if (pci_tsm_disconnect(pdev))
			pci_warn(pdev, "TSM connection is still in use\n");
	} else {
    ...
}

tsm_unregister() then drops its reference, and the provider frees the
underlying pci_tsm memory. Later, when the PCI core calls this modified
pci_tsm_destroy(), won't it dereference the already freed pdev->tsm and
pdev->tsm->tsm_dev pointers here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008055955.4014342-1-aneesh.kumar@kernel.org?part=16