Re: [PATCH v2] net: natsemi: ns83820: Fix use-after-free in ns83820_remove_one due to race condition

[Andrew Lunn]

On Tue, Aug 11, 2026 at 10:51:47AM +0800, Pei Xiao wrote:
> In ns83820_init_one, &dev->tq_refill is bound with queue_refill, and
> ns83820_rx_kick can schedule this work on system_wq when it is called
> from the IRQ handler ns83820_irq (via ns83820_do_isr) or from the rx
> tasklet rx_action.
> 
> If we remove the device, ns83820_remove_one makes cleanup and the
> memory allocated for dev with netdev_priv() is released by
> free_netdev(), while the work mentioned above may still be pending or
> running. The sequence of operations that may lead to a UAF bug is as
> follows:
> 
> CPU0                                      CPU1
> 
>                                           | ns83820_irq
>                                           | ns83820_do_isr
>                                           | ns83820_rx_kick
>                                           | schedule_work(&dev->tq_refill)
> ns83820_remove_one                        |
> ns83820_disable_interrupts(dev)           |
> unregister_netdev(ndev)                   |
> free_irq(dev->pci_dev->irq, ndev)         |
> iounmap(dev->base)                        |
> dma_free_coherent(...)                    |
> free_netdev(ndev)                         |
> // dev is freed                           |
>                                           | queue_refill
>                                           | // use dev (use-after-free)
> 
> free_irq() only prevents the IRQ handler from running again. An rx
> tasklet that was already scheduled by a previous interrupt can still
> run afterwards, and rx_action calls ns83820_rx_kick, which can
> re-schedule tq_refill on system_wq. This leaves a window where
> queue_refill can still run after free_netdev() has freed dev.
> 
> Fix it by stopping the sources that can schedule the work, in order:
> unregister_netdev() and free_irq() stop the IRQ handler, tasklet_kill()
> waits for the rx tasklet to finish, and cancel_work_sync() then drains
> any work that was queued before proceeding with the remaining cleanup
> in ns83820_remove_one.

Is this change tested in any way?

   Andrew