Re: [PATCH 1/2] can: esd_usb: validate received message length before use

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

Pre-existing issues:
- [High] Unvalidated interface count from device causes heap buffer overflow during probe.
- [High] Dereferencing uninitialized network interfaces from USB messages causes kernel panic.
- [High] Spurious TX_DONE messages decrement inactive contexts causing active_tx_jobs underflow.
--

--- Patch [1]: [PATCH 1/2] can: esd_usb: validate received message length before use ---
commit 38efd7f38912ccfd78688896fcdf77da350528fd
Author: Yiran Qiu <[email protected]>
Date:   Fri Aug 14 18:05:46 2026 +0000

    can: esd_usb: validate received message length before use
    
    esd_usb_read_bulk_callback() walks a sequence of variable-length
    messages out of the RX URB buffer. The only length check,
    "pos > urb->actual_length", runs *after* the current message has been
    dispatched and after @pos has been advanced, so it can neither protect
    the message being processed nor stop the loop:
    
    Validate the header and the declared message length before dispatch:
    reject a message whose header is not fully present, whose length is
    zero, or which extends past the received data, and advance @pos by the
    validated length. Pass the validated length to the message handlers so
    they can confirm that the fields they read (and the payload they copy)
    were actually received.

> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index f41d4a0d140f7..13356e68f3f60 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c

[ ... ]

> @@ -455,7 +479,7 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
>  }
>  
>  static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
> -				union esd_usb_msg *msg)
> +				union esd_usb_msg *msg, unsigned int msg_len)
>  {
>  	struct net_device_stats *stats = &priv->netdev->stats;
>  	struct net_device *netdev = priv->netdev;
> @@ -464,6 +488,9 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
>  	if (!netif_device_present(netdev))
>  		return;
>  
> +	if (msg_len < offsetofend(struct esd_usb_tx_done_msg, hnd))
> +		return;
> +
>  	context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
>  
>  	if (!msg->txdone.status) {

[Severity: High]
This is a pre-existing issue, but can spurious TX_DONE messages decrement
inactive contexts and cause the active_tx_jobs counter to underflow?

In esd_usb_tx_done_msg(), the context identified by msg->txdone.hnd is accessed
without checking if it is actually active (e.g. echo_index !=
ESD_USB_MAX_TX_URBS). If a malicious device sends spurious TX_DONE messages,
this will unconditionally execute atomic_dec(&priv->active_tx_jobs). 

When the queue eventually fills up with real packets, esd_usb_start_xmit()
won't stop the queue because active_tx_jobs is negative, returning
NETDEV_TX_BUSY. Returning NETDEV_TX_BUSY without stopping the queue causes the
network stack to infinitely retry, softlocking the CPU.

[ ... ]

> @@ -507,8 +534,27 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>  
>  	while (pos < urb->actual_length) {
>  		union esd_usb_msg *msg;
> +		unsigned int msg_len;
> +
> +		/* The header must be fully present before hdr.len / hdr.cmd
> +		 * (and the net index below) are read.
> +		 */
> +		if (pos + sizeof(struct esd_usb_header_msg) > urb->actual_length) {
> +			dev_err(dev->udev->dev.parent, "format error\n");
> +			break;
> +		}
>  
>  		msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);
> +		msg_len = msg->hdr.len * sizeof(u32); /* convert to # of bytes */
> +
> +		/* A zero-length message would never advance @pos and would
> +		 * spin this URB-completion softirq forever; a message must
> +		 * also fit within the received data.
> +		 */
> +		if (msg->hdr.len == 0 || msg_len > urb->actual_length - pos) {
> +			dev_err(dev->udev->dev.parent, "format error\n");
> +			break;
> +		}
>  
>  		switch (msg->hdr.cmd) {
>  		case ESD_USB_CMD_CAN_RX:
> @@ -517,7 +563,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>  				break;
>  			}
>  
> -			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);
> +			esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg, msg_len);

[Severity: High]
This isn't a bug introduced by this patch, but does this loop properly handle
uninitialized network interfaces?

If esd_usb_probe_one_net() fails for one of the interfaces during
esd_usb_probe(), it leaves dev->nets[index] as NULL. A malicious USB device can
then send ESD_USB_CMD_CAN_RX messages with the index of the failed interface,
which is passed unconditionally to esd_usb_rx_can_msg() resulting in a NULL
pointer dereference.

[Severity: High]
This is also a pre-existing issue, but does msg->rx.net >= dev->net_count
properly protect against out-of-bounds array access for dev->nets[]?

A malicious USB device can return an arbitrarily large number of interfaces in
the version reply message. In esd_usb_probe(), this is directly assigned to
dev->net_count without validating it against ESD_USB_MAX_NETS (2), which can
corrupt adjacent heap memory when writing to dev->nets[index]. Furthermore,
since dev->net_count can be larger than ESD_USB_MAX_NETS, the check
msg->rx.net >= dev->net_count above does not prevent dev->nets[msg->rx.net]
from going out of bounds here.

>  			break;
>  
>  		case ESD_USB_CMD_CAN_TX:
> @@ -527,16 +573,11 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>  			}
>  
>  			esd_usb_tx_done_msg(dev->nets[msg->txdone.net],
> -					    msg);
> +					    msg, msg_len);
>  			break;
>  		}

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