[PATCH net] can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

[PATCH net] can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

[Xiang Mei]

Each channel's esd_usb_net_priv is the netdev private area (netdev_priv()),
so it lives inside the net_device that free_candev() frees. Its TX URBs are
anchored in priv->tx_submitted, i.e. in that same allocation.

esd_usb_disconnect() frees the channels first and only then calls
unlink_all_urbs() to kill the URBs:

	for (i = 0; i < dev->net_count; i++) {
		if (dev->nets[i]) {
			...
			free_candev(netdev);		/* frees priv */
		}
	}
	unlink_all_urbs(dev);			/* reads priv again */

unlink_all_urbs() re-walks dev->nets[i] and does
usb_kill_anchored_urbs(&priv->tx_submitted) on each priv, but priv was just
freed by free_candev(), so this reads and writes through freed memory. KASAN
reports a slab-use-after-free write on the anchor lock, and the fault on the
corrupted list then panics the kernel.

The shared RX URBs (anchored in dev->rx_submitted) have the same problem: their
completion handler esd_usb_read_bulk_callback() dereferences
dev->nets[msg->rx.net], which must still be alive when they are killed.

Fix by killing the URBs before freeing the channels. unlink_all_urbs() only
touches dev, dev->udev, dev->rxbuf[] and the still-live dev->nets[i], so it is
safe to run first.

  BUG: KASAN: slab-use-after-free in _raw_spin_lock_irq (include/linux/instrumented.h:112 ...)
  Write of size 4 at addr ffff888010568ca0 by task kworker/1:0/26
  Call Trace:
   _raw_spin_lock_irq (include/linux/instrumented.h:112 ...)
   usb_kill_anchored_urbs (include/linux/spinlock.h:372 drivers/usb/core/urb.c:818)
   esd_usb_disconnect (drivers/net/can/usb/esd_usb.c:786 drivers/net/can/usb/esd_usb.c:1396)
   usb_unbind_interface (drivers/usb/core/driver.c:458)
   device_release_driver_internal (drivers/base/dd.c:1349 drivers/base/dd.c:1372)
   bus_remove_device (drivers/base/bus.c:664)
   device_del (drivers/base/core.c:3961)
   usb_disable_device (drivers/usb/core/message.c:1478)
   usb_disconnect (drivers/usb/core/hub.c:2345)
   hub_event (drivers/usb/core/hub.c:5407 ...)
   process_one_work (kernel/workqueue.c:3322)
   worker_thread (kernel/workqueue.c:3405 kernel/workqueue.c:3486)
   kthread (kernel/kthread.c:436)
   ret_from_fork (kernel/process.c:158)
   ret_from_fork_asm (arch/x86/entry/entry_64.S:245)

  BUG: unable to handle page fault for address: ffffffffffffffd8
  Oops: Oops: 0002 [#1] SMP KASAN NOPTI
  RIP: 0010:usb_get_urb.part.0 (./include/asm/atomic.h:93 ...)
  Call Trace:
   usb_kill_anchored_urbs (drivers/usb/core/urb.c:819 drivers/usb/core/urb.c:823)
   esd_usb_disconnect (drivers/net/can/usb/esd_usb.c:786 drivers/net/can/usb/esd_usb.c:1396)
   ...
   hub_event (drivers/usb/core/hub.c:5407 ...)
   process_one_work (kernel/workqueue.c:3322)
   worker_thread (kernel/workqueue.c:3405 kernel/workqueue.c:3486)
   kthread (kernel/kthread.c:436)
   ret_from_fork (kernel/process.c:158)
   ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
  Kernel panic - not syncing: Fatal exception

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Reported-by: Weiming Shi <bestswngs@gmail.com>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Xiang Mei <xmei5@asu.edu>
---
 drivers/net/can/usb/esd_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index d257440fa01f..34c0ca187283 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -1385,6 +1385,7 @@ static void esd_usb_disconnect(struct usb_interface *intf)
 
 	if (dev) {
 		dev->in_usb_disconnect = 1;
+		unlink_all_urbs(dev);
 		for (i = 0; i < dev->net_count; i++) {
 			if (dev->nets[i]) {
 				netdev = dev->nets[i]->netdev;
@@ -1393,7 +1394,6 @@ static void esd_usb_disconnect(struct usb_interface *intf)
 				free_candev(netdev);
 			}
 		}
-		unlink_all_urbs(dev);
 		kfree(dev);
 	}
 }
-- 
2.43.0

Re: [PATCH net] can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Moving `unlink_all_urbs()` before `unregister_netdev()` introduces a memory leak race with `esd_usb_open()` on the RX path.
--

commit f21a9588c02c39e2094757bcdada4fb7aca593c8
Author: Xiang Mei <xmei5@asu.edu>

can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

This commit fixes a slab-use-after-free issue by killing URBs before freeing
the CAN devices.

> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d257440fa01ff..34c0ca1872839 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1385,6 +1385,7 @@ static void esd_usb_disconnect(struct usb_interface *intf)
>  
>  	if (dev) {
>  		dev->in_usb_disconnect = 1;
> +		unlink_all_urbs(dev);

[Severity: Medium]
Does moving unlink_all_urbs() before unregister_netdev() introduce a memory
leak regression by racing with esd_usb_open() on the RX path?

Previously, unregister_netdev() ran first, providing rtnl_lock 
synchronization. Now that unlink_all_urbs() runs before unregister_netdev(),
esd_usb_open() could run concurrently if the interface is brought up at the
exact same time the device is disconnected.

If this race occurs, unlink_all_urbs() could scan the buffers while they are
still NULL:

drivers/net/can/usb/esd_usb.c:unlink_all_urbs() {
        ...
        for (i = 0; i < ESD_USB_MAX_RX_URBS; ++i)
                usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE,
                                  dev->rxbuf[i], dev->rxbuf_dma[i]);
        ...
}

And concurrently, esd_usb_setup_rx_urbs() could allocate them immediately
after the check:

drivers/net/can/usb/esd_usb.c:esd_usb_setup_rx_urbs() {
        ...
        dev->rxbuf[i] = buf;
        dev->rxbuf_dma[i] = buf_dma;
        ...
}

Could these newly allocated DMA buffers be permanently leaked since
unlink_all_urbs() has already completed its cleanup pass?

>  		for (i = 0; i < dev->net_count; i++) {
>  			if (dev->nets[i]) {
>  				netdev = dev->nets[i]->netdev;
>  				netdev_info(netdev, "unregister\n");
>  				unregister_netdev(netdev);
>  				free_candev(netdev);
>  			}
>  		}
> -		unlink_all_urbs(dev);
>  		kfree(dev);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705010005.1169943-1-xmei5@asu.edu?part=1