Re: [PATCH net 05/12] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message
From: Paolo Abeni <pabeni@redhat.com>
Date: 2026-03-03 14:20:07
Also in:
linux-can
On 3/2/26 4:16 PM, Marc Kleine-Budde wrote:
quoted hunk ↗ jump to hunk
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <mailhol@kernel.org> Cc: Marc Kleine-Budde <mkl@pengutronix.de> Cc: stable@kernel.org Assisted-by: gkh_clanker_2000 Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Link: https://patch.msgid.link/2026022316-answering-strainer-a5db@gregkh Fixes: 702171adeed3 ("ems_usb: Added support for EMS CPC-USB/ARM7 CAN/USB interface") Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de> --- drivers/net/can/usb/ems_usb.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index 4c219a5b139b..9b25dda7c183 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c@@ -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; + } + msg = (struct ems_cpc_msg *)&ibuf[start]; switch (msg->type) {@@ -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; }
AI noticed the following:
---
Does the check validate enough of the message? The code checks that the
11-byte header (CPC_MSG_HEADER_LEN) fits in the buffer, but
ems_usb_rx_can_msg() and ems_usb_rx_err() both access fields in the
msg->msg union payload starting at offset 11.
For example, ems_usb_rx_can_msg() reads:
- msg->msg.can_msg.id (4 bytes at offset 11)
- msg->msg.can_msg.length (1 byte at offset 15)
- msg->msg.can_msg.msg[i] (up to 8 bytes at offset 16)
Similarly, ems_usb_rx_err() reads:
- msg->msg.can_state (1 byte at offset 11)
- msg->msg.error.cc.regs.sja1000.{ecc,txerr,rxerr} (3 bytes at offset 11+)
A malicious USB device could send a packet where actual_length equals
CPC_HEADER_SIZE + CPC_MSG_HEADER_LEN (15 bytes total), which would pass
this check but provide zero payload bytes. The callees would then read
beyond the received buffer before reaching the second check at the end
of the loop.
Should the validation check that both the header and payload
(msg->length bytes) fit within actual_length before calling the
processing functions?
---
I guess this patch could need a follow-up? Not blocking the PR, as
AFAICS worst case this patch still improves the current situation.
/P