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

[Aneesh Kumar K.V <aneesh.kumar@kernel.org>]

sashiko-bot@kernel.org writes:

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

With this series, vdevices hold a reference to their parent vIOMMU,
which pins arm-cca-host. sysfs unbind is disabled, and the SMCCC
provider device has no hot-unplug path.

-aneesh