[PATCH] can: ems_usb: validate received CAN message payloads

[PATCH] can: ems_usb: validate received CAN message payloads

[Guangshuo Li]

ems_usb_read_bulk_callback() validates that the fixed CPC message header
fits in the received USB buffer before dispatching the message to a
handler.

That is not enough for messages with payloads. The CAN frame handler
reads the CAN id, DLC and data bytes from the variable payload area. A
malformed device can place a CPC message header at the end of the
received buffer so that the fixed header check succeeds, but the CAN
payload read by ems_usb_rx_can_msg() extends past the actual USB data.

The post-dispatch length check is too late because the handler has
already read the payload by then. Also, the number of CAN data bytes read
by ems_usb_rx_can_msg() is derived from the CAN DLC field, so it must be
validated against the received CPC payload length as well.

Validate the complete CPC payload before dispatching the message, and
make sure CAN frame messages contain the bytes required by their DLC.

Fixes: 702171adeed3 ("ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB interface")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/net/can/usb/ems_usb.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
index 9b25dda7c183..f72e36fdf788 100644
--- a/drivers/net/can/usb/ems_usb.c
+++ b/drivers/net/can/usb/ems_usb.c
@@ -409,6 +409,28 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
 		netif_rx(skb);
 }
 
+static bool ems_usb_msg_valid(struct ems_cpc_msg *msg)
+{
+	u8 can_len;
+
+	switch (msg->type) {
+	case CPC_MSG_TYPE_CAN_FRAME:
+	case CPC_MSG_TYPE_EXT_CAN_FRAME:
+		if (msg->length < CPC_CAN_MSG_MIN_SIZE)
+			return false;
+
+		can_len = can_cc_dlc2len(msg->msg.can_msg.length & 0xf);
+		return msg->length >= CPC_CAN_MSG_MIN_SIZE + can_len;
+
+	case CPC_MSG_TYPE_RTR_FRAME:
+	case CPC_MSG_TYPE_EXT_RTR_FRAME:
+		return msg->length >= CPC_CAN_MSG_MIN_SIZE;
+
+	default:
+		return true;
+	}
+}
+
 /*
  * callback for bulk IN urb
  */
@@ -452,6 +474,16 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 
 			msg = (struct ems_cpc_msg *)&ibuf[start];
 
+			if (msg->length > urb->actual_length - start -
+			    CPC_MSG_HEADER_LEN) {
+				netdev_err(netdev, "format error\n");
+				break;
+			}
+			if (!ems_usb_msg_valid(msg)) {
+				netdev_err(netdev, "format error\n");
+				break;
+			}
+
 			switch (msg->type) {
 			case CPC_MSG_TYPE_CAN_STATE:
 				/* Process CAN state changes */
-- 
2.43.0

Re: [PATCH] can: ems_usb: validate received CAN message payloads

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver double-frees coherent DMA buffers when the USB device is disconnected after the interface was brought down.
- [High] The patch introduces `ems_usb_msg_valid()` to validate message payload lengths, but fails to validate `CPC_MSG_TYPE_CAN_STATE` and `CPC_MSG_TYPE_CAN_FRAME_ERROR` types. This leaves a slab out-of-bounds read vulnerability where unvalidated payload fields are accessed.
- [High] Failed interface initialization in `ems_usb_start()` causes a severe memory and URB leak because it returns without cleaning up successfully submitted RX URBs and DMA buffers.
- [High] Uninitialized kernel memory is leaked to the USB device during CAN frame transmission in `ems_usb_start_xmit`.
--

--- Patch [1]: [PATCH] can: ems_usb: validate received CAN message payloads ---
commit 22fc83024cb899592c1db5423c95e678cb8a9b23
Author: Guangshuo Li <lgs201920130244@gmail.com>

can: ems_usb: validate received CAN message payloads

This patch adds payload length validation for received CAN messages in the
ems_usb driver to prevent out-of-bounds reads. It introduces a validation
function to check the length of variable-length payloads before processing
them.

> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 9b25dda7c1838..f72e36fdf788c 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c

[ ... ]

> @@ -409,6 +409,28 @@ static void ems_usb_rx_err(struct ems_usb *dev, struct ems_cpc_msg *msg)
> +static bool ems_usb_msg_valid(struct ems_cpc_msg *msg)
> +{
> +	u8 can_len;
> +
> +	switch (msg->type) {
> +	case CPC_MSG_TYPE_CAN_FRAME:
> +	case CPC_MSG_TYPE_EXT_CAN_FRAME:
> +		if (msg->length < CPC_CAN_MSG_MIN_SIZE)
> +			return false;
> +
> +		can_len = can_cc_dlc2len(msg->msg.can_msg.length & 0xf);
> +		return msg->length >= CPC_CAN_MSG_MIN_SIZE + can_len;
> +
> +	case CPC_MSG_TYPE_RTR_FRAME:
> +	case CPC_MSG_TYPE_EXT_RTR_FRAME:
> +		return msg->length >= CPC_CAN_MSG_MIN_SIZE;
> +
> +	default:
> +		return true;

[Severity: High]
This isn't a bug introduced by this patch, but does this leave the driver
vulnerable to out-of-bounds reads for CAN state and error messages?

If a malicious USB device crafts a CPC_MSG_TYPE_CAN_STATE or
CPC_MSG_TYPE_CAN_FRAME_ERROR message with a truncated length positioned at
the end of the 64-byte RX_BUFFER_SIZE transfer buffer, this default case
returns true without validating the length. When the message is dispatched to
ems_usb_rx_err(), it unconditionally reads fields like msg->msg.can_state
or msg->msg.error.cc.regs.sja1000.ecc. Could these reads fall outside the
allocated slab buffer and leak adjacent kernel memory to userspace?

> +	}
> +}

[Severity: High]
This is a pre-existing issue, but does ems_usb_start() leak URB and DMA
memory on error paths?

If submitting the interrupt URB fails or if subsequent control commands fail,
ems_usb_start() returns an error without cleaning up the successfully
submitted MAX_RX_URBS bulk RX URBs or freeing their associated DMA buffers in
dev->rxbuf. Since the calling function ems_usb_open() does not call
unlink_all_urbs() on this failure, do the RX URBs remain active in the
system? If the user attempts to bring the interface up again, would the new
allocations permanently leak the old DMA buffers and leave ghost URBs
running?

[Severity: Critical]
This is a pre-existing issue, but can unlink_all_urbs() cause a double-free
during device disconnect?

When the interface is brought down, ems_usb_close() calls unlink_all_urbs(),
which frees the dev->rxbuf[i] DMA buffers using usb_free_coherent() but does
not set the pointers to NULL. If the device is subsequently disconnected,
ems_usb_disconnect() calls unlink_all_urbs() again. Since the pointers are
not NULL, won't usb_free_coherent() be called again on the same addresses?

[Severity: High]
This is a pre-existing issue, but does ems_usb_start_xmit() leak
uninitialized kernel memory to the USB device?

The function allocates a 28-byte coherent DMA buffer via usb_alloc_coherent()
with GFP_ATOMIC, which does not zero the memory because __GFP_ZERO is
missing. The driver initializes specific payload fields but leaves up to 21
bytes (such as the 4-byte CPC header padding, msgid, ts_sec, ts_nsec, and
unused CAN payload bytes) uninitialized. When the full 28-byte buffer is
transmitted via bulk URB, does this expose uninitialized kernel slab memory
to the USB device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708125949.765815-1-lgs201920130244@gmail.com?part=1