diff mbox series

[net,v3] net: ethernet: fix use after free bug in ns83820_remove_one due to race condition

Message ID 20230417013107.360888-1-zyytlz.wz@163.com
State New
Headers
Series [net,v3] net: ethernet: fix use after free bug in ns83820_remove_one due to race condition |

Commit Message

Zheng Wang April 17, 2023, 1:31 a.m. UTC 
  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

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Zheng Wang <zyytlz.wz@163.com>
---
v3:
- add tasklet_kill to stop more task scheduling suggested by
Horatiu Vultur
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 | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Horatiu Vultur April 18, 2023, 11:44 a.m. UTC | #1 
The 04/17/2023 09:31, Zheng Wang wrote:
> 
> 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
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Zheng Wang <zyytlz.wz@163.com>

I think it looks OK:
Reviewed-by: Horatiu Vultur <horatiu.vultur@microchip.com>

> ---
> v3:
> - add tasklet_kill to stop more task scheduling suggested by
> Horatiu Vultur
> 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
> index 998586872599..af597719795d 100644
> --- a/drivers/net/ethernet/natsemi/ns83820.c
> +++ b/drivers/net/ethernet/natsemi/ns83820.c
> @@ -2208,8 +2208,14 @@ 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);
> +       tasklet_kill(&dev->rx_tasklet);
> +       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
>
Jakub Kicinski April 19, 2023, 4:33 a.m. UTC | #2 
On Mon, 17 Apr 2023 09:31:07 +0800 Zheng Wang wrote:
> +	netif_carrier_off(ndev);
> +	netif_tx_disable(ndev);
> +
>  	unregister_netdev(ndev

It's not immediately obvious to me why disabling carrier and tx
are supposed to help. Please elaborate in the commit message if
you're confident that this is right.
Zheng Hacker April 19, 2023, 5:30 a.m. UTC | #3 
Jakub Kicinski <kuba@kernel.org> 于2023年4月19日周三 12:33写道：
>
> On Mon, 17 Apr 2023 09:31:07 +0800 Zheng Wang wrote:
> > +     netif_carrier_off(ndev);
> > +     netif_tx_disable(ndev);
> > +
> >       unregister_netdev(ndev
>
> It's not immediately obvious to me why disabling carrier and tx
> are supposed to help. Please elaborate in the commit message if
> you're confident that this is right.
>

Hi Jakub,

Thanks for your reply. I'll figure it out to see if it's really necessary.

Best regards,
Zheng

> --
> pw-bot: cr
diff mbox series

Patch

diff --git a/drivers/net/ethernet/natsemi/ns83820.c b/drivers/net/ethernet/natsemi/ns83820.c
index 998586872599..af597719795d 100644
--- a/drivers/net/ethernet/natsemi/ns83820.c
+++ b/drivers/net/ethernet/natsemi/ns83820.c
@@ -2208,8 +2208,14 @@  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);
+	tasklet_kill(&dev->rx_tasklet);
+	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);