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

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 <[email protected]>

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/[email protected]?part=1