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

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

[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 <mailhol@kernel.org>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: stable <stable@kernel.org>
Assisted-by: gkh_clanker_2000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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;
 			}
-- 
2.53.0

Re: [PATCH] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On 23.02.2026 17:51:17, Greg Kroah-Hartman wrote:
> 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 <stable@kernel.org>
> Assisted-by: gkh_clanker_2000
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied to linux-can, with preferred stable format.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

[Greg Kroah-Hartman]

On Mon, Mar 02, 2026 at 11:06:34AM +0100, Marc Kleine-Budde wrote:
> On 23.02.2026 17:51:17, Greg Kroah-Hartman wrote:
> > 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 <stable@kernel.org>
> > Assisted-by: gkh_clanker_2000
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Applied to linux-can, with preferred stable format.

What is your "preferred stable format"?

Re: [PATCH] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1470 bytes --]

On 02.03.2026 07:50:02, Greg Kroah-Hartman wrote:
> On Mon, Mar 02, 2026 at 11:06:34AM +0100, Marc Kleine-Budde wrote:
> > On 23.02.2026 17:51:17, Greg Kroah-Hartman wrote:
> > > 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 <stable@kernel.org>
> > > Assisted-by: gkh_clanker_2000
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >
> > Applied to linux-can, with preferred stable format.
>
> What is your "preferred stable format"?

FTR: Greg commented on this in:

| https://lore.kernel.org/all/2026030234-sheet-countless-fd2f@gregkh/

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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 <gregkh@linuxfoundation.org>

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.

> 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;
> +			}
> +

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.

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

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] can: ems_usb: ems_usb_read_bulk_callback(): check the proper length of a message

[Greg Kroah-Hartman]

On Mon, Mar 02, 2026 at 11:18:58AM +0100, Marc Kleine-Budde wrote:
> 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 <gregkh@linuxfoundation.org>
> 
> 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.
> 
> > 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;
> > +			}
> > +
> 
> 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.

Ah, tricky, gotta love variable length messages...

I'll review this next week when I get back from my conference trips and
make a v2, thanks for letting me know it's output.

greg k-h