Re: [PATCH net 12/14] 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 prevents a specific out-of-bounds read of `cmd->len` by verifying at least 6 bytes are available, but it fails to enforce a safe minimum structural length for extended commands. A malicious USB device can provide a trivially small `cmd->len` (e.g., 8), bypassing the buffer boundary checks in the parser and leading to severe slab-out-of-bounds reads in downstream command handlers.

Pre-existing issues:
- [Critical] Missing validation of the `echo_index` sentinel value allows a malicious USB device to trigger a slab-out-of-bounds read and an arbitrary kernel pointer write, leading to potential privilege escalation.
--

commit 36da6d5bcb0fd8b842d165066e47c1ff8b40c308
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

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

This commit adds bounds checking for command formats in the kvaser_usb hydra
receive path. It aims to prevent a slab-out-of-bounds read caused by malicious
USB devices sending truncated headers or zero-length commands.

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;

[Severity: High]
Does this length validation allow maliciously small extended commands?

While this prevents cmd_len from being 0, it allows trivially small values
(e.g., 8 bytes). If a malicious device places an 8-byte extended command at
the very end of a 3072-byte USB bulk transfer, kvaser_usb_hydra_cmd_size_rx()
will return 0 and set cmd_len to 8. Because pos + 8 == len, the overarching
boundary check in the parser will pass, handing the 8-byte command to
kvaser_usb_hydra_handle_cmd().

Downstream handlers like kvaser_usb_hydra_rx_msg_ext() then blindly access
payload fields extending far beyond 8 bytes (such as cmd->rx_can.kcan_header
at offset 20). Does this code read out-of-bounds of the 3072-byte heap
allocation?

> +
> +	return 0;
> +}
> +
>  static struct kvaser_usb_net_priv *
>  kvaser_usb_hydra_net_priv_from_cmd(const struct kvaser_usb *dev,
>  				   const struct kvaser_cmd *cmd)

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but while reviewing this file I
noticed a pre-existing issue where an unvalidated echo_index can lead to
an out-of-bounds read and arbitrary memory corruption.

In kvaser_usb_hydra_tx_acknowledge():

	u16 transid = kvaser_usb_hydra_get_cmd_transid(cmd);
	...
	context = &priv->tx_contexts[transid % dev->max_tx_urbs];
	...
	skb = priv->can.echo_skb[context->echo_index];
	if (skb)
		skb_hwtstamps(skb)->hwtstamp = kvaser_usb_hydra_ktime_from_cmd(dev->cfg, cmd);

When a TX context is free (or newly initialized), context->echo_index is
set to the sentinel value dev->max_tx_urbs:

	context->echo_index = dev->max_tx_urbs;

If a malicious USB device sends a spoofed CMD_TX_ACKNOWLEDGE with a transid
matching a free TX context, this code directly indexes priv->can.echo_skb
using dev->max_tx_urbs.

Since echo_skb is placed exactly at the end of the net_device memory block
(via alloc_candev_mqs), an index of max_tx_urbs reads 8 bytes of garbage
from the subsequent kernel slab object.

If the read value is non-NULL, the driver dereferences it and writes a
64-bit timestamp derived from the attacker-controlled USB packet into memory.
Can this be triggered by a malicious USB device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260826121036.2706424-1-mkl@pengutronix.de?part=12