[PATCH net] can: esd_usb: kill anchored URBs before freeing netdevs

[PATCH net] can: esd_usb: kill anchored URBs before freeing netdevs

[Fan Wu]

esd_usb_disconnect() frees each CAN netdev with free_candev() inside
its per-netdev loop and only calls unlink_all_urbs(dev) afterwards.
The per-netdev private data (struct esd_usb_net_priv) is embedded in
the net_device allocation returned by alloc_candev(), so once
free_candev() has run, dev->nets[i] points to freed memory.
unlink_all_urbs() then dereferences the freed dev->nets[i] to kill the
per-netdev TX anchor (usb_kill_anchored_urbs(&priv->tx_submitted)),
clear active_tx_jobs, and reset priv->tx_contexts[].

Reorder the teardown so the anchored URBs are killed before the netdevs
are freed, matching other CAN/USB drivers in the same directory such as
ems_usb, usb_8dev and mcba_usb, which unregister, then unlink, then
free: unregister the netdevs first (which stops their TX queues), call
unlink_all_urbs(dev) once, then free the netdevs.

This issue was found by an in-house static analysis tool.

Fixes: 96d8e90382dc336b5de401164597edfdc2e8d9f1 ("can: Add driver for esd CAN-USB/2 device")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/net/can/usb/esd_usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index d257440fa..f41d4a0d1 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -1390,10 +1390,13 @@ static void esd_usb_disconnect(struct usb_interface *intf)
 				netdev = dev->nets[i]->netdev;
 				netdev_info(netdev, "unregister\n");
 				unregister_netdev(netdev);
-				free_candev(netdev);
 			}
 		}
 		unlink_all_urbs(dev);
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i])
+				free_candev(dev->nets[i]->netdev);
+		}
 		kfree(dev);
 	}
 }
-- 
2.34.1

RE: [PATCH net] can: esd_usb: kill anchored URBs before freeing netdevs

[Jagielski, Jedrzej]

From: Fan Wu <fanwu01@zju.edu.cn> 
Sent: Thursday, July 9, 2026 12:46 PM

>esd_usb_disconnect() frees each CAN netdev with free_candev() inside
>its per-netdev loop and only calls unlink_all_urbs(dev) afterwards.
>The per-netdev private data (struct esd_usb_net_priv) is embedded in
>the net_device allocation returned by alloc_candev(), so once
>free_candev() has run, dev->nets[i] points to freed memory.
>unlink_all_urbs() then dereferences the freed dev->nets[i] to kill the
>per-netdev TX anchor (usb_kill_anchored_urbs(&priv->tx_submitted)),
>clear active_tx_jobs, and reset priv->tx_contexts[].
>
>Reorder the teardown so the anchored URBs are killed before the netdevs
>are freed, matching other CAN/USB drivers in the same directory such as
>ems_usb, usb_8dev and mcba_usb, which unregister, then unlink, then
>free: unregister the netdevs first (which stops their TX queues), call
>unlink_all_urbs(dev) once, then free the netdevs.
>
>This issue was found by an in-house static analysis tool.
>
>Fixes: 96d8e90382dc336b5de401164597edfdc2e8d9f1 ("can: Add driver for esd CAN-USB/2 device")

Hi Fan,

no need to put whole SHA, 12 first chars is enough

[PATCH net v2] can: esd_usb: kill anchored URBs before freeing netdevs

[Fan Wu]

esd_usb_disconnect() frees each CAN netdev with free_candev() inside
its per-netdev loop and only calls unlink_all_urbs(dev) afterwards.
The per-netdev private data (struct esd_usb_net_priv) is embedded in
the net_device allocation returned by alloc_candev(), so once
free_candev() has run, dev->nets[i] points to freed memory.
unlink_all_urbs() then dereferences the freed dev->nets[i] to kill the
per-netdev TX anchor (usb_kill_anchored_urbs(&priv->tx_submitted)),
clear active_tx_jobs, and reset priv->tx_contexts[].

Reorder the teardown so the anchored URBs are killed before the netdevs
are freed, matching other CAN/USB drivers in the same directory such as
ems_usb, usb_8dev and mcba_usb, which unregister, then unlink, then
free: unregister the netdevs first (which stops their TX queues), call
unlink_all_urbs(dev) once, then free the netdevs.

This issue was found by an in-house static analysis tool.

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Cc: stable@vger.kernel.org
Assisted-by: Codex:gpt-5.5
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
Changes in v2: shorten the Fixes: tag to the abbreviated 12-char SHA
per Jedrzej Jagielski ("no need to put whole SHA, 12 first chars is
enough").

v1: https://lore.kernel.org/netdev/20260709104620.133765-1-fanwu01@zju.edu.cn/
 drivers/net/can/usb/esd_usb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index d257440fa..f41d4a0d1 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -1390,10 +1390,13 @@ static void esd_usb_disconnect(struct usb_interface *intf)
 				netdev = dev->nets[i]->netdev;
 				netdev_info(netdev, "unregister\n");
 				unregister_netdev(netdev);
-				free_candev(netdev);
 			}
 		}
 		unlink_all_urbs(dev);
+		for (i = 0; i < dev->net_count; i++) {
+			if (dev->nets[i])
+				free_candev(dev->nets[i]->netdev);
+		}
 		kfree(dev);
 	}
 }
-- 
2.34.1

Re: [PATCH net] can: esd_usb: kill anchored URBs before freeing netdevs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Malicious USB devices or normal operations can trigger a NULL pointer dereference if `esd_usb_probe_one_net()` fails for a network interface.
- [High] A zero-length message from the USB device causes an infinite loop in the RX completion handler, leading to a softirq CPU lockup.
- [High] The driver parses and copies USB message payload into CAN frames before validating that the message length fits within the received USB data or allocated buffer.
- [High] Double-free and Use-After-Free of echo_skb during device teardown due to missing URB cancellation in `ndo_stop`.
- [Medium] If `usb_submit_urb()` fails in `esd_usb_start_xmit()`, the allocated TX context is never released, leading to a permanent depletion of TX contexts.
--

--- Patch [1]: [PATCH net] can: esd_usb: kill anchored URBs before freeing netdevs ---
commit f2616c0b84f4389121d1ebf004051cb8242711c9
Author: Fan Wu <fanwu01@zju.edu.cn>

can: esd_usb: kill anchored URBs before freeing netdevs

This commit reorders the teardown sequence in esd_usb_disconnect so that
anchored URBs are killed before the CAN netdevs are freed. This prevents a
use-after-free where unlink_all_urbs would otherwise dereference a
previously freed network device structure to cancel active TX jobs.

> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d257440fa01ff..f41d4a0d140f7 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1390,10 +1390,13 @@ static void esd_usb_disconnect(struct usb_interface *intf)
>  				netdev = dev->nets[i]->netdev;
>  				netdev_info(netdev, "unregister\n");
>  				unregister_netdev(netdev);
> -				free_candev(netdev);
>  			}
>  		}
>  		unlink_all_urbs(dev);
> +		for (i = 0; i < dev->net_count; i++) {
> +			if (dev->nets[i])
> +				free_candev(dev->nets[i]->netdev);
> +		}
>  		kfree(dev);
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but if esd_usb_probe_one_net
fails during initialization, could it lead to a NULL pointer dereference
later?

In esd_usb_probe:
	for (i = 0; i < dev->net_count; i++)
		esd_usb_probe_one_net(intf, i);

If the probe fails for a specific network interface, dev->nets[i] remains
NULL because the error is ignored. If a USB device sends a message for that
uninitialized network index, is it safe to pass it directly to
esd_usb_rx_can_msg?

In esd_usb_read_bulk_callback:
		case ESD_USB_CMD_CAN_RX:
			if (msg->rx.net >= dev->net_count) {
				dev_err(dev->udev->dev.parent, "format error\n");
				break;
			}

			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);

[Severity: High]
This is a pre-existing issue, but does a zero-length message from the USB
device cause an infinite loop in the RX completion handler?

In esd_usb_read_bulk_callback:
		pos += msg->hdr.len * sizeof(u32); /* convert to # of bytes */

		if (pos > urb->actual_length) {
			dev_err(dev->udev->dev.parent, "format error\n");
			break;
		}

If a local USB device sets msg->hdr.len = 0, pos will not advance,
potentially causing the loop condition pos < urb->actual_length to run
infinitely and lock up the CPU in softirq context.

[Severity: High]
This is a pre-existing issue, but does the driver parse the USB message
payload before ensuring it fits within the received buffer?

In esd_usb_read_bulk_callback:
	while (pos < urb->actual_length) {
		union esd_usb_msg *msg;

		msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);

		switch (msg->hdr.cmd) {
		case ESD_USB_CMD_CAN_RX:
// ...
			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);

The message fields seem to be parsed and passed to handlers like
esd_usb_rx_can_msg which read from the buffer, but the boundary check
evaluating pos > urb->actual_length happens at the end of the loop. Can this
lead to an out-of-bounds read if a message is positioned near the end of the
transfer buffer?

[Severity: High]
This is a pre-existing issue, but is there a potential double-free of
echo_skb during device teardown?

In esd_usb_close:
	priv->can.state = CAN_STATE_STOPPED;

	netif_stop_queue(netdev);

	close_candev(netdev);

	return err;

Since close_candev flushes the echo_skb array without locking, and
esd_usb_close does not appear to kill active URBs before this point, could
a concurrent TX URB completion fire in softirq?

In esd_usb_tx_done_msg:
		stats->tx_errors++;
		can_free_echo_skb(netdev, context->echo_index, NULL);

If both paths access and free the exact same echo_skb concurrently, this
might lead to memory corruption.

[Severity: Medium]
This is a pre-existing issue, but if usb_submit_urb fails in
esd_usb_start_xmit, does it permanently leak a TX context?

In esd_usb_start_xmit:
	err = usb_submit_urb(urb, GFP_ATOMIC);
	if (err) {
		can_free_echo_skb(netdev, context->echo_index, NULL);

		atomic_dec(&priv->active_tx_jobs);
		usb_unanchor_urb(urb);

The error path frees the echo_skb, but it appears to miss resetting
context->echo_index back to ESD_USB_MAX_TX_URBS. If this happens repeatedly
due to transient errors, could it permanently exhaust all 16 contexts and
stall network transmission?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709104620.133765-1-fanwu01@zju.edu.cn?part=1

Re: [PATCH net v2] can: esd_usb: kill anchored URBs before freeing netdevs

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing bounds check on `dev->net_count` derived from untrusted USB device input leads to an out-of-bounds array write.
- [High] A zero-length message from the USB device causes an infinite loop in the RX completion handler.
- [High] Missing NULL check for `dev->nets[i]` array elements allows a malicious USB device to trigger a NULL pointer dereference.
- [High] Missing bounds validation on the URB buffer length allows an attacker-controlled USB device to trigger out-of-bounds heap memory reads.
--

--- Patch [1]: [PATCH net v2] can: esd_usb: kill anchored URBs before freeing netdevs ---
commit 61e5cb7cf9d6a05752ca57498b34f59417480dc7
Author: Fan Wu <fanwu01@zju.edu.cn>

can: esd_usb: kill anchored URBs before freeing netdevs

This patch reorders the teardown sequence in esd_usb_disconnect() to unregister
netdevs, unlink all URBs, and then free the netdevs. This resolves an issue
where the URB unlinking process accesses a freed netdev structure.

> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d257440fa01ff..f41d4a0d140f7 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1390,10 +1390,13 @@ static void esd_usb_disconnect(struct usb_interface *intf)
>  				netdev = dev->nets[i]->netdev;
>  				netdev_info(netdev, "unregister\n");
>  				unregister_netdev(netdev);
> -				free_candev(netdev);
>  			}
>  		}
>  		unlink_all_urbs(dev);
> +		for (i = 0; i < dev->net_count; i++) {
> +			if (dev->nets[i])
> +				free_candev(dev->nets[i]->netdev);
> +		}
>  		kfree(dev);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does this code in esd_usb_probe() allow a
regression where a USB device triggers an out-of-bounds heap write?

In esd_usb_probe():

	err = esd_usb_wait_msg(dev, msg);
	if (err < 0) {
		dev_err(&intf->dev, "no version message answer\n");
		goto free_msg;
	}

	dev->net_count = (int)msg->version_reply.nets;

[ ... ]

	/* do per device probing */
	for (i = 0; i < dev->net_count; i++)
		esd_usb_probe_one_net(intf, i);

If a USB device provides a version_reply.nets value up to 255, the driver
assigns it to dev->net_count without checking it against ESD_USB_MAX_NETS (2).

The initialization loop then iterates dev->net_count times, calling
esd_usb_probe_one_net():

	dev->nets[index] = priv;

Could this write past the bounds of the dev->nets array into adjacent memory?

[Severity: High]
This isn't a bug introduced by this patch, but does the RX completion handler
contain a regression leading to an infinite loop if a zero-length message is
received?

In esd_usb_read_bulk_callback():

	while (pos < urb->actual_length) {
		union esd_usb_msg *msg;

		msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);

[ ... ]

		pos += msg->hdr.len * sizeof(u32); /* convert to # of bytes */

If a USB device provides a message with msg->hdr.len == 0, pos does not
increase.

The loop condition pos < urb->actual_length would remain true forever, causing
the loop to spin indefinitely.

[Severity: High]
This is a pre-existing issue, but does esd_usb_read_bulk_callback() dereference
a NULL pointer if a previous interface registration failed?

In esd_usb_probe(), if registering a netdev fails in esd_usb_probe_one_net()
due to memory exhaustion, it returns an error:

	for (i = 0; i < dev->net_count; i++)
		esd_usb_probe_one_net(intf, i);

The error is ignored and dev->nets[i] remains NULL.

Later in esd_usb_read_bulk_callback():

		case ESD_USB_CMD_CAN_RX:
			if (msg->rx.net >= dev->net_count) {
				dev_err(dev->udev->dev.parent, "format error\n");
				break;
			}

			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);
			break;

If a USB device sends an ESD_USB_CMD_CAN_RX packet specifying the failed
interface's index, it passes the dev->net_count check.

Does the lack of a NULL check before passing the pointer to esd_usb_rx_can_msg()
cause a crash here?

[Severity: High]
This isn't a bug introduced by this patch, but does esd_usb_read_bulk_callback()
read past the URB buffer bounds?

In esd_usb_read_bulk_callback():

	while (pos < urb->actual_length) {
		union esd_usb_msg *msg;

		msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);

		switch (msg->hdr.cmd) {

The code accesses the message fields before checking if the message size fits
within urb->actual_length.

Additionally, if the message length validation is missing, could this lead to an
out-of-bounds heap memory read in esd_usb_rx_can_msg()?

	memcpy(cfd->data, msg->rx.data_fd, len);

If msg->rx.data_fd falls near the end of the urb->transfer_buffer, this could
copy adjacent heap memory into the CAN frame, leaking memory contents.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709164159.497640-1-fanwu01@zju.edu.cn?part=1