can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

When looking at the data in a USB urb, the actual_length is the size of the buffer passed to the driver, not the transfer_buffer_length which is set by the driver as the max size of the buffer.

When parsing the messages in ems_usb_read_bulk_callback() properly check the size both at the beginning of parsing the message to make sure it is big enough for the expected structure, and at the end of the message to make sure we don't overflow past the end of the buffer for the next message.

Cc Vincent Mailhol <[email protected]>

Cc Marc Kleine-Budde <[email protected]>

Cc stable <[email protected]>

Assisted-by gkh_clanker_2000

Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 10 lines · Greg Kroah-Hartman

When looking at the data in a USB urb, the actual_length is the size of the buffer passed to the driver, not the transfer_buffer_length which is set by the driver as the max size of the buffer.

When parsing the messages in ems_usb_read_bulk_callback() properly check the size both at the beginning of parsing the message to make sure it is big enough for the expected structure, and at the end of the message to make sure we don't overflow past the end of the buffer for the next message.

Quoted 5 lines

Cc Vincent Mailhol <[email protected]>

Cc Marc Kleine-Budde <[email protected]>

Cc stable <[email protected]>

Assisted-by gkh_clanker_2000

Signed-off-by Greg Kroah-Hartman <[email protected]>

Applied to linux-can, with preferred stable format.

Quoted 17 lines · Marc Kleine-Budde

Quoted 15 lines · Greg Kroah-Hartman

When looking at the data in a USB urb, the actual_length is the size of the buffer passed to the driver, not the transfer_buffer_length which is set by the driver as the max size of the buffer.

When parsing the messages in ems_usb_read_bulk_callback() properly check the size both at the beginning of parsing the message to make sure it is big enough for the expected structure, and at the end of the message to make sure we don't overflow past the end of the buffer for the next message.

Cc Vincent Mailhol <[email protected]>

Cc Marc Kleine-Budde <[email protected]>

Cc stable <[email protected]>

Assisted-by gkh_clanker_2000

Signed-off-by Greg Kroah-Hartman <[email protected]>

Quoted 1 line

Applied to linux-can, with preferred stable format.

What is your "preferred stable format"?

drivers/net/can/usb/ems_usb.c +0 -0

 			msg = (struct ems_cpc_msg *)&ibuf[start];
 
 			switch (msg->type) {

drivers/net/can/usb/ems_usb.c +1 -1

@@ -474,7 +479,7 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 			start += CPC_MSG_HEADER_LEN + msg->length;
 			msg_count--;
 
-			if (start > urb->transfer_buffer_length) {
+			if (start > urb->actual_length) {
 				netdev_err(netdev, "format error\n");
 				break;
 			}

Hello,

I've applied the patch, but the nipa/ai-review bot found some more problem:

| https://netdev-ai.bots.linux.dev/ai-review.html?id=8aebbd0a-af1c-4caf-8659-03fcf827f060

commit 0b9cf350a2d2eab647de28210e8eaf2a56d5d624
Author: Greg Kroah-Hartman <[email protected]>

can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

When looking at the data in a USB urb, the actual_length is the size of the buffer passed to the driver, not the transfer_buffer_length which is set by the driver as the max size of the buffer. This commit adds validation at the beginning of message parsing and corrects the validation after incrementing the start pointer to use actual_length instead of transfer_buffer_length.

drivers/net/can/usb/ems_usb.c static void ems_usb_read_bulk_callback(struct urb *urb) · line 452

Quoted 13 lines

drivers/net/can/usb/ems_usb.c +5 -0

index 4c219a5b139b..9b25dda7c183 100644
@@ -445,6 +445,11 @@ static void ems_usb_read_bulk_callback(struct urb *urb)
 		start = CPC_HEADER_SIZE;
 
 		while (msg_count) {
+			if (start + CPC_MSG_HEADER_LEN > urb->actual_length) {
+				netdev_err(netdev, "format error\n");
+				break;
+			}
+

The new bounds check here validates that CPC_MSG_HEADER_LEN (11 bytes) fits within the buffer, but does this protect against reading beyond the buffer when accessing the message payload?

Looking at the structure usage:

    msg = (struct ems_cpc_msg *)&ibuf[start];

The ems_cpc_msg structure has a variable-length payload indicated by msg->length. After this line, the code calls ems_usb_rx_can_msg() or ems_usb_rx_err() which access fields in the payload region.

In ems_usb_rx_can_msg(), the code reads:

    msg->msg.can_msg.msg[i]

for up to cf->len bytes from the payload.

In ems_usb_rx_err(), the code reads:

    msg->msg.error.cc.regs.sja1000.ecc

and other union members from the payload.

Since only the header length is validated here, can a malicious USB device send a packet with a large msg->length value that causes out-of-bounds reads when these functions access the payload data?

Would it be more complete to validate the full message size before the switch statement:

    if (start + CPC_MSG_HEADER_LEN + msg->length > urb->actual_length)

The commit message mentions checking "the size both at the beginning of parsing the message to make sure it is big enough for the expected structure," but the current check only validates the header size, not the complete structure including the payload that will be accessed by the processing functions.