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

在 2026/8/17 13:53, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Use-After-Free window in the `switchtec_pci_probe()` error path due to premature freeing of `stdev` while `event_irq` is active.
> - [High] Use-After-Free via TOCTOU race in `switchtec_dev_write` due to incorrect teardown ordering in `stdev_kill()`.
> --
> 
> --- Patch [1]: [PATCH v3] PCI: switchtec: Fix use-after-free in switchtec_pci_remove due to race condition ---
> commit a18ebaceba03ea5f94a521c5ced89b052f121ea9
> Author: Pei Xiao <[email protected]>
> Subject: PCI: switchtec: Fix use-after-free in switchtec_pci_remove due to race condition
> 
> This patch attempts to fix a use-after-free bug during device removal by
> quiescing interrupt sources before canceling pending work items. It adds
> logic to stdev_kill() to explicitly free IRQs and cancel work structures.
> It also stores the IRQ numbers explicitly in the switchtec_dev structure.
> 
> Link: https://lore.kernel.org/lkml/[email protected]/#t
> 
>> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
>> index 41fc4b5127081..91d44acfe1193 100644
>> --- a/drivers/pci/switch/switchtec.c
>> +++ b/drivers/pci/switch/switchtec.c
> 
> [ ... ]
> 
>> @@ -1318,6 +1318,13 @@ static void stdev_kill(struct switchtec_dev *stdev)
>>  
>>  	pci_clear_master(stdev->pdev);
>>  
>> +	if (stdev->event_irq)
>> +		devm_free_irq(&stdev->pdev->dev, stdev->event_irq, stdev);
>> +	if (stdev->dma_mrpc_irq)
>> +		devm_free_irq(&stdev->pdev->dev, stdev->dma_mrpc_irq, stdev);
>> +
>> +	cancel_work_sync(&stdev->mrpc_work);
>> +	cancel_work_sync(&stdev->link_event_work);
>>  	cancel_delayed_work_sync(&stdev->mrpc_timeout);
> [Severity: High]
> This is a pre-existing issue, but does canceling this delayed work before
> setting the alive flag allow it to be re-armed?
> 
> If userspace performs a concurrent write during device removal, it can call
> switchtec_dev_write(), which does:
> 
> drivers/pci/switch/switchtec.c:switchtec_dev_write() {
>     ...
>     rc = lock_mutex_and_test_alive(stdev);
>     if (rc)
>         return rc;
>     ...
>     rc = mrpc_queue_cmd(stuser);
>     ...
> }
> 
> Because cancel_delayed_work_sync() executes outside of mrpc_mutex and before
> stdev->alive is set to false, mrpc_queue_cmd() could successfully re-arm
> stdev->mrpc_timeout after it was just canceled.
> 
> When stdev is subsequently freed, could this newly armed delayed work execute
> on freed memory?
I have send this patch for this bug:
https://lore.kernel.org/lkml/e640c79072065c1a347ed25f7ce0fe1ba565f4fe.1786017236.git.xiaopei01@kylinos.cn/
> 
>>  
>>  	/* Mark the hardware as unavailable and complete all completions */
> 
> [ ... ]
> 
>> @@ -1513,6 +1522,7 @@ static int switchtec_init_isr(struct switchtec_dev *stdev)
>>  
>>  	if (rc)
>>  		return rc;
>> +	stdev->event_irq = event_irq;
> [Severity: High]
> This is a pre-existing issue, but is there a use-after-free window in the
> probe error path?
> 
> When switchtec_init_isr() requests event_irq, it uses devm_request_irq()
> bound to the PCI device:
> 
> drivers/pci/switch/switchtec.c:switchtec_init_isr() {
>     ...
>     rc = devm_request_irq(&stdev->pdev->dev, event_irq,
>                           switchtec_event_isr, 0, KBUILD_MODNAME, stdev);
>     ...
> }
> 
> If a subsequent initialization step fails during switchtec_pci_probe(),
> the err_put path will free the stdev structure:
> 
> drivers/pci/switch/switchtec.c:switchtec_pci_probe() {
>     ...
> err_put:
>     ida_free(&switchtec_minor_ida, MINOR(stdev->dev.devt));
>     put_device(&stdev->dev);
>     return rc;
> }
> 
> Since put_device() immediately frees stdev without explicitly freeing the IRQ,
> and devres cleanup doesn't run until the probe function returns, could an
> interrupt fire during this window and cause switchtec_event_isr() to
> dereference the already freed stdev?
> 
>>  
>>  	if (!stdev->dma_mrpc)
>>  		return rc;
> 
> [ ... ]
>