Re: [PATCH net v3] 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The patch prevents an OOB read of the `cmd->len` field but fails to enforce a minimum payload length for the extracted `cmd_len`, allowing maliciously truncated commands to bypass buffer bounds checks and cause slab-out-of-bounds reads in downstream handlers.
--

commit 25501dc93a8df5ec40c88edcab68f9d2a1cded89
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

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

The receive-path command parsers call kvaser_usb_hydra_cmd_size() without
verifying that enough buffer remains. This patch adds
kvaser_usb_hydra_cmd_size_rx() to check whether the size field is complete
before reading it and rejects zero command lengths.

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..eddd207ee8daa 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,24 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
>  	return ret;
>  }
>  
> +/* -EAGAIN means incomplete; -EINVAL rejects zero to ensure progress. */
> +static int kvaser_usb_hydra_cmd_size_rx(struct kvaser_cmd *cmd,
> +					size_t remaining, size_t *cmd_len)
> +{
> +	if (remaining < sizeof(cmd->header.cmd_no))
> +		return -EAGAIN;
> +
> +	if (cmd->header.cmd_no == CMD_EXTENDED &&
> +	    remaining < offsetof(struct kvaser_cmd_ext, cmd_no_ext))
> +		return -EAGAIN;
> +
> +	*cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +	if (!*cmd_len)
> +		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does kvaser_usb_hydra_cmd_size_rx() need to
enforce a minimum payload length for the extracted cmd_len based on the
command type?

If a malicious USB device sends a CMD_EXTENDED packet with an artificially
small length (for example, 8), this function will extract that small length
without validating it against the actual structure size.

[ ... ]

> @@ -2154,9 +2204,13 @@ 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);
> +		err = kvaser_usb_hydra_cmd_size_rx(cmd, len - pos, &cmd_len);
> +		if (err && err != -EAGAIN) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}
>  
> -		if (pos + cmd_len > len) {
> +		if (err == -EAGAIN || pos + cmd_len > len) {

[Severity: High]
This is a pre-existing issue, but because the extracted cmd_len is
artificially small, the buffer bounds check in
kvaser_usb_hydra_read_bulk_callback():

    if (err == -EAGAIN || pos + cmd_len > len) {

can pass when the packet is placed at the end of the URB buffer.

Downstream handlers like kvaser_usb_hydra_rx_msg_ext() will then read fields
at fixed offsets far exceeding this length, for instance:

    kcan_header = le32_to_cpu(cmd->rx_can.kcan_header);

at offset 20, or:

    memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

at offset 32. Can this cause a slab-out-of-bounds read past the end of the USB
URB buffer?

>  			/* We got first part of a command */
>  			int leftover_bytes;

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