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

[sashiko-bot@kernel.org]

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