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

[Marc Kleine-Budde]

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@xxxxxxxxxxxxxxxxxxx>

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: signature.asc
Description: PGP signature