Re: [PATCH net 02/19] 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:
- [Critical] This problem wasn't introduced by this patch, but a missing bounds check on `dev->net_count` derived from the USB device response leads to a critical heap buffer overflow.
- [High] This problem wasn't introduced by this patch, but a malicious USB device can cause an infinite loop and CPU hard lockup by sending a message with length 0.
- [High] This problem wasn't introduced by this patch, but a malicious USB device can trigger an out-of-bounds memory read by sending a malformed or truncated message payload.
- [High] This problem wasn't introduced by this patch, but a missing NULL check on partially initialized network interfaces allows a malicious USB device to crash the kernel via NULL pointer dereference.
- [High] This problem wasn't introduced by this patch, but there is a permanent leakage of TX contexts on URB submission failure, leading to a permanent DoS of the interface.
--

--- Patch [2]: [PATCH net 02/19] can: esd_usb: kill anchored URBs before freeing netdevs ---
commit 1a29cc039e833667b5ba86f653b2a7b3b73983e1
Author: Fan Wu <[email protected]>

can: esd_usb: kill anchored URBs before freeing netdevs

esd_usb_disconnect() frees each CAN netdev with free_candev() inside
its per-netdev loop and only calls unlink_all_urbs() afterwards.
The per-netdev private data is embedded in the net_device allocation,
so once free_candev() has run, dev->nets points to freed memory.
Reordering the teardown to match other CAN/USB drivers avoids this.

Link: https://patch.msgid.link/[email protected]

> 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 while reviewing the driver 
code I noticed a potential CPU hard lockup in esd_usb_read_bulk_callback() in 
drivers/net/can/usb/esd_usb.c:

    while (pos < urb->actual_length) {
        ...
        pos += msg->hdr.len * sizeof(u32); 

If a USB device sends a message where len is set to 0, won't this loop run 
infinitely since pos is never advanced?

[Severity: High]
This is a pre-existing issue, but could a USB device trigger an out-of-bounds 
memory read by sending a malformed message payload? 

In esd_usb_read_bulk_callback() in drivers/net/can/usb/esd_usb.c:

    msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);
    switch (msg->hdr.cmd) {
    ...

Are the message fields dereferenced here before verifying if the remaining 
bytes in the transfer buffer are sufficient to back the structure?

[Severity: High]
This problem wasn't introduced by this patch, but could a USB device crash 
the kernel via a NULL pointer dereference? 

In esd_usb_probe() in drivers/net/can/usb/esd_usb.c, we have:

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

If esd_usb_probe_one_net() fails, it returns an error but the driver ignores
the return value, leaving dev->nets[i] as NULL. 

Later, if esd_usb_read_bulk_callback() receives a message targeted at this 
failed index:

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

Is it possible this unconditionally dereferences dev->nets[msg->rx.net] without 
checking if it is NULL first?

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing bounds 
check on dev->net_count in drivers/net/can/usb/esd_usb.c?

In esd_usb_probe():

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

The array upper bound is set based on untrusted USB input with no validation 
against the structural maximum (ESD_USB_MAX_NETS). 

Later in esd_usb_probe_one_net():

    dev->nets[index] = priv;

Could a USB device claim a large number of nets and cause a heap buffer 
overflow by writing past the end of the dev->nets array?

[Severity: High]
This is a pre-existing issue, but does the error handling in 
esd_usb_start_xmit() in drivers/net/can/usb/esd_usb.c leak TX contexts?

    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);
        stats->tx_dropped++;

If the URB submission fails, the path drops the SKB and untracks the job, but 
forgets to reset context->echo_index (e.g. to ESD_USB_MAX_TX_URBS). 

Could this permanently leak the context slot, eventually exhausting all 
available contexts and leading to a denial of service?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2