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

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) [off-list ref]

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

[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