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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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