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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
can: kvaser_usb: validate command format before parsing in hydra receive path

This adds kvaser_usb_hydra_cmd_size_rx() so the hydra receive paths check
that the CMD_EXTENDED 2-byte length field is fully present before reading
it, and rejects a declared length of zero to keep the parse loop making
progress.  The asynchronous path now keeps a partial extended header in the
leftover buffer and completes it from the next transfer.

Link: https://patch.msgid.link/20260826013037.6933-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..efe7b3ef489bf 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;
> +
> +	return 0;
> +}

[Severity: High]
Should this helper also reject a declared length that is smaller than the
fields the extended-command consumers dereference?

Right now the only constraint on the device-supplied length is that it is
non-zero, but struct kvaser_cmd_ext is 96 bytes with cmd_no_ext at offset 6,
rx_can.flags at 8, kcan_header at 20, timestamp at 24 and kcan_payload at
32..95:

	struct kvaser_cmd_ext {
		struct kvaser_cmd_header header;
		__le16 len;
		u8 cmd_no_ext;
		u8 reserved;
		union {
			struct kvaser_cmd_ext_rx_can rx_can;
			...
	} __packed;

kvaser_usb_hydra_handle_cmd_ext() reads cmd->cmd_no_ext and then
kvaser_usb_hydra_rx_msg_ext() reads all the way up to kcan_payload,
without any relation to cmd->len:

	drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_rx_msg_ext() {
		...
		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
		...
	}

The sibling leaf driver already enforces per-command minimums before
dispatch via kvaser_usb_leaf_verify_size() in
kvaser_usb_leaf_handle_command().  Would it make sense to require
cmd_len >= offsetof(struct kvaser_cmd_ext, cmd_no_ext) + 2 here, or better a
per-cmd_no_ext minimum up to sizeof(struct kvaser_cmd_ext), instead of only
rejecting zero?

> @@ -675,8 +693,8 @@ 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_size_rx(tmp_cmd, actual_len - pos, &cmd_len);
> +			if (err || pos + cmd_len > actual_len) {
>  				dev_err_ratelimited(&dev->intf->dev,
>  						    "Format error\n");
>  				break;

[Severity: High]
In kvaser_usb_hydra_wait_cmd(), a CMD_EXTENDED declaring a small positive
length (say 1) still returns 0 from the helper, and cmd_len is then used as
the stride:

			pos += cmd_len;

Does this leave the parser resynchronising from the middle of the malformed
extended header for the rest of the buffer?  The accesses stay inside the
kzalloc'd buffer here, but the interpretation of the following bytes is
attacker-chosen.

> @@ -2120,23 +2138,47 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	spin_lock_irqsave(usb_rx_leftover_lock, irq_flags);
>  	usb_rx_leftover_len = card_data->usb_rx_leftover_len;
>  	if (usb_rx_leftover_len) {
> +		const size_t cmd_size_field_end = offsetof(struct kvaser_cmd_ext, cmd_no_ext);
>  		int remaining_bytes;
> +		int err;
>  
>  		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		if (cmd->header.cmd_no == CMD_EXTENDED &&
> +		    usb_rx_leftover_len < cmd_size_field_end) {
> +			remaining_bytes = min_t(int, len, cmd_size_field_end - usb_rx_leftover_len);
> +
> +			memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf, remaining_bytes);
> +			usb_rx_leftover_len += remaining_bytes;
> +			card_data->usb_rx_leftover_len = usb_rx_leftover_len;
> +			pos += remaining_bytes;
>  
> -		remaining_bytes = min_t(unsigned int, len,
> +			if (usb_rx_leftover_len < cmd_size_field_end) {
> +				spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
> +				return;
> +			}
> +		}
> +
> +		err = kvaser_usb_hydra_cmd_size_rx(cmd, usb_rx_leftover_len, &cmd_len);
> +		if (err || cmd_len < usb_rx_leftover_len) {
> +			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);

[Severity: High]
On the reassembly path, once the top-up above has filled exactly the 6 bytes
up to cmd_size_field_end, a device-declared cmd_len of 6, 7 or 8 passes the
helper and then satisfies the completion test straight away:

		if (remaining_bytes + usb_rx_leftover_len == cmd_len) {
			kvaser_usb_hydra_handle_cmd(dev, cmd);

Does this dispatch an extended command whose cmd_no_ext and payload bytes
come from whatever was previously left in the 128-byte
card_data->usb_rx_leftover array, since only the first 6 bytes have been
written by this transfer?

> @@ -2152,11 +2194,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
>  
>  	while (pos < len) {
> +		int err;
> +
>  		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) {
>  			/* We got first part of a command */
>  			int leftover_bytes;
>  

[Severity: High]
Can a crafted transfer still reach the same class of out-of-bounds read that
this commit describes?

Here err == 0 together with pos + cmd_len <= len is the complete gate before
kvaser_usb_hydra_handle_cmd(), and pos is advanced by the device's own
declared lengths, so pos can be placed anywhere in the 3072-byte URB buffer
allocated in kvaser_usb_setup_rx_urbs():

	buf = usb_alloc_coherent(dev->udev, KVASER_USB_RX_BUFFER_SIZE,
				 GFP_KERNEL, &buf_dma);

For a full 3072-byte transfer holding a CMD_EXTENDED with len 3064, followed
at pos 3064 by a CMD_EXTENDED with len 8 and cmd_no_ext CMD_RX_MESSAGE_FD:

  remaining is 8, which is >= offsetof(struct kvaser_cmd_ext, cmd_no_ext)
  cmd_len is 8, so the zero check passes
  pos + cmd_len == 3072 == len, so the truncation check passes

kvaser_usb_hydra_handle_cmd() -> kvaser_usb_hydra_handle_cmd_ext() ->
kvaser_usb_hydra_rx_msg_ext() then reads rx_can.flags, kcan_header and
timestamp past the end of the transfer buffer and finishes with:

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

with cf->len up to 64 taken from the device-supplied DLC, i.e. up to roughly
88 bytes beyond the 3072-byte allocation.  On host controllers where
hcd_uses_dma() is false (dummy_hcd, vhci) hcd_buffer_alloc() falls back to
kmalloc(3072), so KASAN would report a slab-out-of-bounds read here, and the
bytes read are then handed to netif_rx() inside a CAN FD frame.

Would adding the minimum-length check inside kvaser_usb_hydra_cmd_size_rx()
close this as well, rather than leaving it for a follow-up?