Messages in this thread | | | From | Zheng Hacker <> | Date | Thu, 13 Apr 2023 18:49:48 +0800 | Subject | Re: [PATCH net v2] net: ethernet: fix use after free bug in ns83820_remove_one due to race condition |
| |
Horatiu Vultur <horatiu.vultur@microchip.com> 于2023年4月13日周四 18:01写道: > > The 04/13/2023 15:14, Zheng Wang wrote: > > Hi Zheng, > > > > > In ns83820_init_one, dev->tq_refill was bound with queue_refill. > > > > If irq happens, it will call ns83820_irq->ns83820_do_isr. > > Then it invokes tasklet_schedule(&dev->rx_tasklet) to start > > rx_action function. And rx_action will call ns83820_rx_kick > > and finally start queue_refill function. > > > > If we remove the driver without finishing the work, there > > may be a race condition between ndev, which may cause UAF > > bug. > > > > CPU0 CPU1 > > > > |queue_refill > > ns83820_remove_one | > > free_netdev | > > put_device | > > free ndev | > > |rx_refill > > |//use ndev > > Will you not have the same issue if you remove the driver after you > schedule rx_tasklet? Because rx_action will use also ndev. >
Hello Horatiu,
Thanks for your reply. In ns83820_remove_one, there is an invoking:
free_irq(dev->pci_dev->irq, ndev);
This will prevent the driver from handling more irq, But it couldn't prevent the rx_tasklet from being scheduled. So I think we should add the following code:
tasklet_kill(&dev->rx_tasklet);
after free_irq invoking. Is there anything wrong about my analysis?
Thanks again for pointing the mistake out.
Best regards, Zheng
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > > --- > > v2: > > - cancel the work after unregister_netdev to make sure there > > is no more request suggested by Jakub Kicinski > > --- > > drivers/net/ethernet/natsemi/ns83820.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c > > index 998586872599..2e84b9fcd8e9 100644 > > --- a/drivers/net/ethernet/natsemi/ns83820.c > > +++ b/drivers/net/ethernet/natsemi/ns83820.c > > @@ -2208,8 +2208,13 @@ static void ns83820_remove_one(struct pci_dev *pci_dev) > > > > ns83820_disable_interrupts(dev); /* paranoia */ > > > > + netif_carrier_off(ndev); > > + netif_tx_disable(ndev); > > + > > unregister_netdev(ndev); > > free_irq(dev->pci_dev->irq, ndev); > > + cancel_work_sync(&dev->tq_refill); > > + > > iounmap(dev->base); > > dma_free_coherent(&dev->pci_dev->dev, 4 * DESC_SIZE * NR_TX_DESC, > > dev->tx_descs, dev->tx_phy_descs); > > -- > > 2.25.1 > > > > -- > /Horatiu
| |