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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for Use-After-Free: `cancel_work_sync()` is called without synchronizing or freeing active IRQ handlers, allowing ISRs to execute after `stdev` is freed and re-schedule works.
- [High] Incorrect cancellation ordering allows `mrpc_timeout` delayed work to be re-armed by concurrent userspace IO after `cancel_delayed_work_sync()`, leading to Use-After-Free.
--

--- Patch [1]: [PATCH] PCI: switchtec: Fix use-after-free in switchtec_pci_remove due to race condition ---
commit e4d2baee32009f95ba28f6ef409e3f3b5a2a15e3
Author: Pei Xiao <xiaopei01@kylinos.cn>
PCI: switchtec: Fix use-after-free in switchtec_pci_remove due to race condition

This commit attempts to fix a use-after-free bug by canceling the mrpc_work
and link_event_work works during stdev_kill(). It ensures works are canceled
after pci_clear_master() stops the endpoint from issuing further interrupts.

> diff --git a/drivers/pci/switch/switchtec.c b/drivers/pci/switch/switchtec.c
> index 41fc4b5127081..8426d5a9c1c7d 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);

[Severity: High]
This is a pre-existing issue, but does canceling this delayed work here leave
a race window where concurrent userspace operations can re-arm it?

Since cancel_delayed_work_sync() is called before the mrpc_mutex is acquired
and stdev->alive is set to false, can a concurrent switchtec_dev_write()
verify stdev->alive == true and call mrpc_cmd_submit()?

mrpc_cmd_submit()
  schedule_delayed_work(&stdev->mrpc_timeout, msecs_to_jiffies(500));

If userspace then closes the file descriptor, dropping the final kobject
reference and freeing stdev, won't the delayed work timer expire and execute
mrpc_timeout_work() on the freed stdev?

> +	cancel_work_sync(&stdev->mrpc_work);
> +	cancel_work_sync(&stdev->link_event_work);

[Severity: High]
This isn't a bug introduced by this patch, but does pci_clear_master() wait
for currently executing interrupt handlers to finish?

During device unbind, switchtec_pci_remove() calls stdev_kill(). While
pci_clear_master() prevents new interrupts from being initiated, it doesn't
flush pending interrupts or wait for executing handlers like
switchtec_event_isr().

Could a handler continue running concurrently, and after these works are
canceled, schedule the work again?

switchtec_event_isr()
  schedule_work(&stdev->mrpc_work);

If put_device(&stdev->dev) then frees stdev before the driver's devres (which
owns the interrupt) runs, won't this result in a use-after-free when the
handler places the freed work struct onto the workqueue?

>  
>  	/* Mark the hardware as unavailable and complete all completions */
>  	scoped_guard (mutex, &stdev->mrpc_mutex) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6fdcbfa869f707cad783c7f92d1681f0a2bd9a7c.1785813784.git.xiaopei01@kylinos.cn?part=1