Re: [PATCH] can: esd_usb: validate receive message extents

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on `dev->net_count` received from the USB device, leading to an out-of-bounds array write in `esd_usb_probe_one_net`.
- [High] Missing NULL check for `dev->nets[msg->rx.net]` before dereferencing it when processing RX or TX_DONE messages.
- [Medium] Permanent leak of TX contexts when `usb_submit_urb` fails in `esd_usb_start_xmit`.
- [Medium] Information leak of up to 8 bytes of uninitialized kernel heap memory in `esd_usb_3_set_bittiming`.
--

commit 80cc711006aa828bbd745bab396f97f761d0c5dd
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: esd_usb: validate receive message extents

The bulk callback reads the message command before proving that the current
URB remainder contains a header and dispatches CAN messages before checking
the declared message span. Short records can therefore be consumed before
the later position check.

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

[ ... ]

> @@ -507,8 +534,20 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>  
>  	while (pos < urb->actual_length) {
>  		union esd_usb_msg *msg;
> +		size_t msg_len;
> +		int remaining = urb->actual_length - pos;
>  
> +		if (remaining < sizeof(msg->hdr)) {
> +			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);
> +		if (msg_len < sizeof(msg->hdr) || msg_len > remaining ||
> +		    !esd_usb_rx_msg_valid(msg, msg_len)) {
> +			dev_err(dev->udev->dev.parent, "format error\n");
> +			break;
> +		}
>  
>  		switch (msg->hdr.cmd) {
>  		case ESD_USB_CMD_CAN_RX:

[Severity: High]
This isn't a bug introduced by this patch, but it looks like there might be a
NULL pointer dereference when processing RX or TX_DONE messages.

If esd_usb_probe_one_net() fails to register a CAN interface (e.g. due to
memory allocation failure), it returns an error but leaves dev->nets[i] as
NULL. The probe function ignores this return value:

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

Later, in esd_usb_read_bulk_callback(), the code validates the net index
but does not check if the array element is NULL before passing it to
esd_usb_rx_can_msg() or esd_usb_tx_done_msg():

	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);

Can a misbehaving or malicious USB device send a CAN_RX or CAN_TX command
targeting a failed network index and cause a kernel panic?

[ ... ]

While reviewing this code, I also noticed a few other pre-existing issues in
the driver.

[Severity: High]
This is a pre-existing issue, but I noticed a potential out-of-bounds array
write when probing the network interfaces.

In esd_usb_probe(), the driver trusts the number of interfaces returned by
the USB device:

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

It then iterates up to this value without bounds checking:

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

In esd_usb_probe_one_net(), the index is used to write to the statically sized
array of length 2 (ESD_USB_MAX_NETS):

	dev->nets[index] = priv;

Could a malicious USB device return a nets value greater than ESD_USB_MAX_NETS
and cause a heap memory corruption here?


[Severity: Medium]
This is a pre-existing issue, but there appears to be a permanent leak of
TX contexts when transmission fails.

In esd_usb_start_xmit(), a transmission context is reserved by setting
context->echo_index = i. If the subsequent call to usb_submit_urb() fails,
the cleanup path unwinds the active_tx_jobs and the echo SKB:

	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);

However, it omits the required context->echo_index = ESD_USB_MAX_TX_URBS
reset. Does this leave the context incorrectly marked as in-use, potentially
leading to a denial of service where the driver perpetually returns
NETDEV_TX_BUSY?


[Severity: Medium]
This is a pre-existing issue, but I noticed a potential information leak of
uninitialized kernel heap memory.

In esd_usb_3_set_bittiming(), a message buffer is allocated via kmalloc_obj()
which does not zero the memory:

	msg = kmalloc_obj(*msg);

The code populates the fields of the esd_usb_3_set_baudrate_msg_x structure,
but the 8-byte baud_x->data structure is only initialized if CAN FD mode is
enabled:

	if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
		...
	}

The entire 28-byte structure is then sent over USB:

	err = esd_usb_send_msg(priv->usb, msg);

If CAN FD mode is not enabled, does this leak up to 8 bytes of uninitialized
kernel heap data to the USB device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830125640.98787-1-pengpeng@iscas.ac.cn?part=1