Re: [PATCH] can: kvaser_usb: hydra: validate command prefixes before parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect command length check in `kvaser_usb_hydra_wait_cmd` incorrectly drops valid extended commands.
- [High] Incorrect memory copy source pointer corrupts fragmented command assembly.
--

commit 1cbbe99543052600da4f97d992d3e51a35e561aa
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: kvaser_usb: hydra: validate command prefixes before parsing

The Hydra receive paths derive a command length from the current buffer
before checking that a complete command header is available. This commit
validates the header and extended-length prefix before decoding a command
length.

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

[ ... ]

> @@ -675,8 +699,10 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
>  			size_t cmd_len;
>  
>  			tmp_cmd = buf + pos;
> -			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
> -			if (pos + cmd_len > actual_len) {
> +			err = kvaser_usb_hydra_cmd_len(tmp_cmd, actual_len - pos,
> +						       &cmd_len);
> +			if (err || cmd_len > actual_len - pos ||
> +			    cmd_len > sizeof(*cmd)) {

[Severity: High]
Does this cmd_len > sizeof(*cmd) check unintentionally reject valid
extended commands?

Since kvaser_usb_hydra_cmd_len() correctly calculates the length of extended
commands (which can be larger than sizeof(struct kvaser_cmd)), it seems this
check will evaluate to true and trigger a format error when an extended
command arrives, terminating buffer processing prematurely.

>  				dev_err_ratelimited(&dev->intf->dev,
>  						    "Format error\n");
>  				break;

[ ... ]

> @@ -2121,12 +2147,44 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,

[ ... ]

> +		if (err) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			card_data->usb_rx_leftover_len = 0;
> +			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
> +			return;
> +		}
> +
> +		remaining_bytes = min_t(unsigned int, len - pos,
>  					cmd_len - usb_rx_leftover_len);
>  		/* Make sure we do not overflow usb_rx_leftover */
>  		if (remaining_bytes + usb_rx_leftover_len >

[Severity: High]
Does the leftover command assembly copy from the wrong source pointer?

The new while loop added just above this snippet can advance pos as it
consumes bytes to complete the command header. However, looking slightly
further down in kvaser_usb_hydra_read_bulk_callback(), the memcpy() copying
the remaining payload still reads directly from buf rather than buf + pos:

    memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf,
           remaining_bytes);
    pos += remaining_bytes;

If pos was advanced by the header completion loop, won't this copy duplicate
header bytes from the beginning of the USB transfer instead of the actual
remainder of the command payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830125322.98199-1-pengpeng@iscas.ac.cn?part=1