[PATCH net] pds_core: fix use-after-free on workqueue during remove

[PATCH net] pds_core: fix use-after-free on workqueue during remove

[Nikhil P. Rao]

In pdsc_remove(), the workqueue is destroyed before pdsc_teardown()
is called. If pdsc_teardown() -> pdsc_devcmd_reset() times out, the
error path in pdsc_devcmd_locked() tries to queue health_work on the
already destroyed workqueue.

Fix by setting pdsc->wq to NULL after destroying the workqueue. The
devcmd error path already checks pdsc->wq before calling queue_work().

This is safe because by this point:
- Auxiliary bus clients have been removed, so no adminq commands are
  in flight and no completion IRQ can trigger queue_work()
- timer_shutdown_sync() has run, so no new health_work can be queued
  from the watchdog timer

Fixes: c2dbb0904310 ("pds_core: health timer and workqueue")
Signed-off-by: Nikhil P. Rao <nikhil.rao@amd.com>
---
 drivers/net/ethernet/amd/pds_core/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
index 22db78343eb0..cb3ac26266aa 100644
--- a/drivers/net/ethernet/amd/pds_core/main.c
+++ b/drivers/net/ethernet/amd/pds_core/main.c
@@ -435,8 +435,10 @@ static void pdsc_remove(struct pci_dev *pdev)
 		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
 
 		timer_shutdown_sync(&pdsc->wdtimer);
-		if (pdsc->wq)
+		if (pdsc->wq) {
 			destroy_workqueue(pdsc->wq);
+			pdsc->wq = NULL;
+		}
 
 		mutex_lock(&pdsc->config_lock);
 		set_bit(PDSC_S_STOPPING_DRIVER, &pdsc->state);
-- 
2.43.0

Re: [PATCH net] pds_core: fix use-after-free on workqueue during remove

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
pds_core: fix use-after-free on workqueue during remove

This commit fixes a use-after-free on the workqueue during remove by
destroying the workqueue earlier and setting pdsc->wq to NULL.

> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c
> index 22db78343eb07..cb3ac26266aa7 100644
> --- a/drivers/net/ethernet/amd/pds_core/main.c
> +++ b/drivers/net/ethernet/amd/pds_core/main.c
> @@ -435,8 +435,10 @@ static void pdsc_remove(struct pci_dev *pdev)
>  		pdsc_auxbus_dev_del(pdsc, pdsc, &pdsc->padev);
>  
>  		timer_shutdown_sync(&pdsc->wdtimer);
> -		if (pdsc->wq)
> +		if (pdsc->wq) {
>  			destroy_workqueue(pdsc->wq);

[Severity: High]
This is a pre-existing issue, but can this destroy_workqueue() call deadlock
with pdsc_pci_reset_thread()?

The PCI subsystem holds device_lock() when calling pdsc_remove().
If pdsc_pci_reset_thread() is currently executing on this workqueue,
destroy_workqueue() will block waiting for it to finish.

However, pdsc_pci_reset_thread() calls pci_reset_function(), which attempts
to acquire the same device_lock(), resulting in a deadlock.

> +			pdsc->wq = NULL;

[Severity: High]
Can setting pdsc->wq to NULL here introduce a regression where the interrupt
handler dereferences a NULL pointer?

This assignment is executed before set_bit(PDSC_S_STOPPING_DRIVER,
&pdsc->state) and before pdsc_stop() masks hardware interrupts.

If an interrupt fires (e.g., a NotifyQ event) in this window,
pdsc_adminq_isr() will run. Because the PDSC_S_STOPPING_DRIVER state is not
yet set, the ISR check succeeds and it will unconditionally call queue_work()
with the NULL pdsc->wq:

pdsc_adminq_isr()
    ...
    queue_work(pdsc->wq, &qcq->work);

> +		}
>  
>  		mutex_lock(&pdsc->config_lock);
>  		set_bit(PDSC_S_STOPPING_DRIVER, &pdsc->state);