[PATCH] can: esd_usb: validate receive message extents

[PATCH] can: esd_usb: validate receive message extents

[Pengpeng Hou]

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.

Validate the common header, declared record extent, and command-specific
payload before dispatch.

Fixes: 96d8e90382dc ("can: Add driver for esd CAN-USB/2 device")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/net/can/usb/esd_usb.c | 46 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 40 insertions(+), 6 deletions(-)

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
@@ -482,6 +482,33 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
 	netif_wake_queue(netdev);
 }
 
+static bool esd_usb_rx_msg_valid(const union esd_usb_msg *msg, size_t len)
+{
+	size_t data_offset = offsetof(struct esd_usb_rx_msg, data);
+	u32 id;
+	u8 data_len;
+
+	if (msg->hdr.cmd == ESD_USB_CMD_CAN_TX)
+		return len >= sizeof(struct esd_usb_tx_done_msg);
+	if (msg->hdr.cmd != ESD_USB_CMD_CAN_RX)
+		return true;
+	if (len < data_offset)
+		return false;
+
+	id = le32_to_cpu(msg->rx.id);
+	if (id & ESD_USB_EVENT)
+		return len >= data_offset + sizeof(msg->rx.ev_can_err_ext);
+
+	if (msg->rx.dlc & ESD_USB_FD)
+		data_len = can_fd_dlc2len(msg->rx.dlc);
+	else if (msg->rx.dlc & ESD_USB_RTR)
+		data_len = 0;
+	else
+		data_len = can_cc_dlc2len(msg->rx.dlc);
+
+	return len >= data_offset + data_len;
+}
+
 static void esd_usb_read_bulk_callback(struct urb *urb)
 {
 	struct esd_usb *dev = urb->context;
@@ -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:
@@ -531,12 +570,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 			break;
 		}
 
-		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;
-		}
+		pos += msg_len;
 	}
 
 resubmit_urb:

base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
-- 
2.50.1

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

[sashiko-bot]

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