Re: [PATCH] PCI: switchtec: Fix use-after-free in switchtec_pci_remove due to race condition

[Pei Xiao]

在 2026/8/5 03:48, Logan Gunthorpe 写道:
> 
> 
> On 2026-08-03 21:24, Pei Xiao wrote:
>> In stdev_create, &stdev->mrpc_work is bound with mrpc_event_work, and
>> &stdev->link_event_work is bound with link_event_work. The IRQ handlers
>> switchtec_event_isr and switchtec_dma_mrpc_isr can schedule these works
>> on system_wq (via schedule_work() in the ISRs and via
>> check_link_state_events()).
>>
>> If we remove the device, switchtec_pci_remove makes cleanup and the
>> memory allocated for stdev is released by put_device() ->
>> stdev_release() -> kfree(stdev), while the works mentioned above may
>> still be pending or running. The sequence of operations that may lead
>> to a UAF bug is as follows:
>>
>> CPU0                                      CPU1
>>
>>                                           | switchtec_event_isr
>>                                           | schedule_work(&stdev->mrpc_work)
>> switchtec_pci_remove                      |
>> cdev_device_del(&stdev->cdev,             |
>>                 &stdev->dev)              |
>> stdev_kill(stdev)                         |
>> switchtec_exit_pci(stdev)                 |
>> pci_dev_put(stdev->pdev)                  |
>> put_device(&stdev->dev)                   |
>> // stdev_release -> kfree(stdev)          |
>>                                           | mrpc_event_work
>>                                           | // use stdev (use-after-free)
>>
>> Fix it by canceling the works after the sources that can schedule them
>> have been stopped: stdev_kill() first clears PCI bus mastering, which
>> prevents the MSI/MSI-X based IRQ handlers from firing and scheduling
>> new works, and the works are then canceled before the remaining
>> cleanup and the release of stdev. This also covers the probe error
>> path, which calls stdev_kill().
>>
>> Fixes: 080b47def5e5 ("MicroSemi Switchtec management interface driver")
>> Fixes: 48c302dc8f3a ("NTB: switchtec: Add link event notifier callback")
>> Assisted-by: Codex:deepseek-v4-flash
>> Signed-off-by: Pei Xiao <xiaopei01@xxxxxxxxxx>
>> ---
>>  drivers/pci/switch/switchtec.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
>> index 5711aaa5df11..f339ea54aa38 100644
>> --- a/drivers/pci/switch/switchtec.c
>> +++ b/drivers/pci/switch/switchtec.c
>> @@ -1319,6 +1319,8 @@ static void stdev_kill(struct switchtec_dev *stdev)
>>  	pci_clear_master(stdev->pdev);
>>  
>>  	cancel_delayed_work_sync(&stdev->mrpc_timeout);
>> +	cancel_work_sync(&stdev->mrpc_work);
>> +	cancel_work_sync(&stdev->link_event_work);
> 
> I'm wondering if these should come before the mrpc_timeout sync.
> Otherwise, hypothetically, new work could be added and another timeout
> could be in progress.
> 
yes,
  	
+	cancel_work_sync(&stdev->mrpc_work);
+	cancel_work_sync(&stdev->link_event_work);
	cancel_delayed_work_sync(&stdev->mrpc_timeout);

> Also, I'm not sure, but seems like the interrupt should be disabled
> before this as well?
I looked it up and it appears that pci_clear_master cannot disable
interrupt enabling. Should I use devm_free_irq?

Thanks！
Pei.
> 
> Thanks!
> 
> Logan