Re: [PATCH net v2] can: kvaser_usb: validate command format before parsing in hydra receive path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The patch introduces a functional regression where valid fragmented USB streams are treated as format errors and dropped, permanently breaking stream reassembly.

Pre-existing issues:
- [High] The patch fails to validate that the length of a `CMD_EXTENDED` command is large enough for its payload, allowing out-of-bounds reads to persist in the handlers.
--

--- Patch [1]: [PATCH net v2] can: kvaser_usb: validate command format before parsing in hydra receive path ---
commit 1e011698d1b2d6b5a15d58005d752a57039bcf61
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

can: kvaser_usb: validate command format before parsing in hydra receive path

This patch adds kvaser_usb_hydra_cmd_size_rx() to validate buffer bounds
before reading the command size. It attempts to fix a slab-out-of-bounds
read and an infinite loop when parsing malformed or zero-length CMD_EXTENDED
commands in the receive path.

Link: https://lore.kernel.org/all/20260819145658.29872-1-blbllhy@gmail.com

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> index efbb7bed34c9d..534bd26830125 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> @@ -536,6 +536,20 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
>  	return ret;
>  }
>  
> +/* Receive-path wrapper: validate buffer bounds before reading cmd_size. */
> +static size_t kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
> +					   size_t remaining)
> +{
> +	if (remaining < sizeof(struct kvaser_cmd_header))
> +		return 0;
> +
> +	if (cmd->header.cmd_no == CMD_EXTENDED &&
> +	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
> +		return 0;
> +
> +	return kvaser_usb_hydra_cmd_size(cmd);
> +}

[Severity: High]
This is a pre-existing issue, but does this patch completely fix the
out-of-bounds reads for extended commands?

If a malicious USB device sends a CMD_EXTENDED command placed at the end of a
bulk transfer (e.g., 6 bytes remaining at the end of a 3072-byte buffer) and
sets the len field to 6, kvaser_usb_hydra_cmd_size_rx() will pass the bounds
checks and return the device-controlled length of 6.

In kvaser_usb_hydra_read_bulk_callback(), the bounds check:

    if (pos + cmd_len > len)

will pass (3066 + 6 > 3072 is false). The command is then passed to
kvaser_usb_hydra_handle_cmd_ext(), which reads cmd->cmd_no_ext at offset 6
(out of bounds). 

If the command maps to CMD_RX_MESSAGE_FD, kvaser_usb_hydra_rx_msg_ext()
accesses cmd->rx_can.kcan_header at offset 20, causing a slab out-of-bounds
read that can be copied into a CAN frame payload and leaked to userspace.

Should there be an additional check to enforce that the returned cmd_len is
large enough to contain the extended struct payload?

> [ ... ]
> @@ -2154,7 +2178,11 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	while (pos < len) {
>  		cmd = buf + pos;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		cmd_len = kvaser_usb_hydra_cmd_size_rx(cmd, len - pos);
> +		if (!cmd_len) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}

[Severity: High]
Does this change break USB stream reassembly for valid fragmented commands?

Before this patch, if a standard command had only 2 bytes remaining at the end
of a URB, kvaser_usb_hydra_cmd_size() would determine it was not CMD_EXTENDED
and return sizeof(struct kvaser_cmd).

Those 2 bytes were then safely buffered in usb_rx_leftover and successfully
reassembled when the next URB arrived.

With this patch, kvaser_usb_hydra_cmd_size_rx() returns 0 if remaining < 4.
This causes the main loop in kvaser_usb_hydra_read_bulk_callback() to log
"Format error" and return immediately, discarding the fragment and permanently
desynchronizing the command stream.

>  
>  		if (pos + cmd_len > len) {
>  			/* We got first part of a command */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260824214058.44948-1-blbllhy@gmail.com?part=1