[PATCH v2 0/2] can: kvaser_usb: validate values received from the device

[PATCH v2 0/2] can: kvaser_usb: validate values received from the device

[Joas Antonio dos Santos via B4 Relay]

Two fixes for values that kvaser_usb takes from the USB device without
checking them.

Patch 1 rejects hydra commands whose length is shorter than a standard
command.  A length of zero otherwise stalls the command loops, and a
short extended command makes the handlers read past the command.

Patch 2 rejects devices that report zero max outstanding tx, which
otherwise leads to a division by zero.

These issues were found with the help of an LLM-based code review tool
(Claude).  I checked the code paths by hand against v7.4-rc3 and
reviewed the fixes myself.  They are build-tested only (W=1, arm64); I
do not have the hardware to test them at runtime.

---
Changes in v2:
- Patch 1: require at least sizeof(struct kvaser_cmd) instead of the
  4-byte header, so that the extended command handlers do not read past
  a short command (sashiko review of v1).  Subject changed, it was
  "can: kvaser_usb: hydra: reject commands shorter than the header".
- Patch 2: unchanged.
- Link to v1: https://lore.kernel.org/r/20261110-kvaser-usb-validate-v1-0-f266b85aa908@gmail.com

---
Joas Antonio dos Santos (2):
      can: kvaser_usb: hydra: reject commands shorter than a standard command
      can: kvaser_usb: reject devices reporting zero max outstanding tx

 drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c  | 6 ++++++
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 8 +++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)
---
base-commit: b3dfbde62cf1981c4638939c159dfd087b7a72e9
change-id: 20261110-kvaser-usb-validate-33c2e34b63bf

Best regards,
-- 
Joas Antonio dos Santos <joasantonio108@gmail.com>

[PATCH v2 1/2] can: kvaser_usb: hydra: reject commands shorter than a standard command

[Joas Antonio dos Santos via B4 Relay]

From: Joas Antonio dos Santos <joasantonio108@gmail.com>

kvaser_usb_hydra_cmd_size() returns the length field supplied by the
device for extended commands.  Both command parsers advance through the
received buffer by that length:

  kvaser_usb_hydra_wait_cmd()            pos += cmd_len;
  kvaser_usb_hydra_read_bulk_callback()  pos += cmd_len;

Neither checks that the length is non-zero.  A device that sends an
extended command (cmd_no == CMD_EXTENDED) with a length of zero keeps
pos unchanged, so the loop never terminates.  In
kvaser_usb_hydra_read_bulk_callback() this happens in the URB completion
handler, which also calls kvaser_usb_hydra_handle_cmd() on the same
command in every iteration, and locks up the CPU.  In
kvaser_usb_hydra_wait_cmd() the loop spins in process context without
ever reaching the timeout check.

A non-zero length that is too small is not valid either.  The parsers
only check that cmd_len bytes are available, but the handlers of the
extended commands read the fixed part of the command without looking at
the length again: cmd_no_ext, the flags and timestamp of
struct kvaser_cmd_ext_tx_ack and the fields of
struct kvaser_cmd_ext_rx_can in front of the payload.  That fixed part
ends at byte 32 in both cases, so a shorter extended command makes the
handlers read bytes that are not part of the command.

Standard commands are sizeof(struct kvaser_cmd), 32 bytes, and the
extended commands that the driver handles or sends are never shorter
than that.  Treat a shorter length as a format error in both parsers.

A similar infinite loop in the leaf command parsers was fixed in
commit 0c73772cd2b8 ("can: kvaser_usb: leaf: Fix potential infinite loop
in command parsers").

Found by LLM-assisted source review.

Fixes: aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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 687911d764de..9adb4eb36e39 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -676,7 +676,8 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
 
 			tmp_cmd = buf + pos;
 			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
-			if (pos + cmd_len > actual_len) {
+			if (cmd_len < sizeof(struct kvaser_cmd) ||
+			    pos + cmd_len > actual_len) {
 				dev_err_ratelimited(&dev->intf->dev,
 						    "Format error\n");
 				break;
@@ -2156,6 +2157,11 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 
 		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
 
+		if (cmd_len < sizeof(struct kvaser_cmd)) {
+			dev_err(&dev->intf->dev, "Format error\n");
+			return;
+		}
+
 		if (pos + cmd_len > len) {
 			/* We got first part of a command */
 			int leftover_bytes;

-- 
2.43.0

Re: [PATCH v2 1/2] can: kvaser_usb: hydra: reject commands shorter than a standard command

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The new 32-byte floor (cmd_len < sizeof(struct kvaser_cmd)) only
  covers the fixed part of an extended command.
- [High] The commit message says 'Treat a shorter length as a format
  error in both parsers', but the usb_rx_leftover reassembly path in…
- [Medium] The patch rejects every CMD_EXTENDED command whose len is
  below 32 bytes, including extended command numbers the driver doesn't…

--

can: kvaser_usb: hydra: reject commands shorter than a standard command

This commit treats any command length below sizeof(struct kvaser_cmd)
(32 bytes) as a format error. The check is added to both
kvaser_usb_hydra_wait_cmd() and kvaser_usb_hydra_read_bulk_callback().
The goal is to prevent an infinite loop on zero-length extended commands
and to stop handlers from reading past short extended commands.

> 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 687911d764ded..9adb4eb36e394 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

[ ... ]

> @@ -2156,6 +2157,11 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  
>  		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
>  
> +		if (cmd_len < sizeof(struct kvaser_cmd)) {

[Severity: High]

Does a 32-byte floor cover the whole extended RX command? The fixed part
of struct kvaser_cmd_ext_rx_can ends at byte 32. kcan_payload[64] and
err_frame_data come after it, and kvaser_usb_hydra_rx_msg_ext() never
checks the payload size against the command's len:

kvaser_usb_hydra_rx_msg_ext() {
    ...
	if (flags & KVASER_USB_HYDRA_CF_FLAG_FDF) {
		cf->len = can_fd_dlc2len(dlc);
    ...
		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
    ...
}

Take a device that sends CMD_EXTENDED / CMD_RX_MESSAGE_FD with len == 32,
KVASER_USB_HYDRA_CF_FLAG_FDF set and DLC 15 in kcan_header. That command
passes the new check, and the memcpy() then reads bytes 32..95 of a
32-byte command.

An error frame reads err_frame_data at bytes 32..39. A classic frame
reads up to 8 bytes past the end.

In this function the buffer is the 3072-byte URB buffer. Suppose the
32-byte extended command ends exactly at byte 3072, for example after 95
standard commands. Can the memcpy() then read up to 64 bytes past the end
of that allocation?

For HCDs without DMA, such as vhci_hcd, hcd_buffer_alloc() uses
kmalloc(). The bytes read end up in a CAN FD frame passed to netif_rx(),
where any CAN socket can read them.

The commit message says:

  "so a shorter extended command makes the handlers read bytes that are
  not part of the command."

That kind of read still looks reachable here. The TX side sets len to
include the payload:

kvaser_usb_hydra_frame_to_cmd_ext() {
    ...
	*cmd_len = ALIGN(sizeof(struct kvaser_cmd_ext) -
			 sizeof(cmd->tx_can.kcan_payload) + nbr_of_bytes,
			 8);
    ...
}

Should the handler, or the parser using cmd_len, check the offset of
kcan_payload plus cf->len (or plus sizeof(err_frame_data)) against
cmd_len?

> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;

[Severity: Medium]

Does this floor also reject well-formed extended commands that the driver
just doesn't handle? The extended format only needs the 8-byte header.
kvaser_usb_hydra_handle_cmd_ext() already has a default branch for
unknown commands, and that branch only reads header bytes:

kvaser_usb_hydra_handle_cmd_ext() {
    ...
	default:
		dev_warn(&dev->intf->dev, "Unhandled extended command (%d)\n",
			 cmd->header.cmd_no);
		break;
    ...
}

Before this patch, an unknown extended command of 8 to 31 bytes was
skipped with pos += cmd_len, and the rest of the transfer was still
parsed. The commit message only shows that the extended commands the
driver handles or sends are at least 32 bytes.

With the return here, everything after such a command in the URB is
dropped, including CMD_TX_ACKNOWLEDGE / CMD_TX_ACKNOWLEDGE_FD and RX
frames. A following command that crosses the URB boundary is not saved
as leftover either.

kvaser_usb_hydra_tx_acknowledge() is the only place that does these
things:

- releases the echo skb
- resets context->echo_index
- decrements active_tx_contexts
- wakes the queue

Could each lost ACK leak a TX context, until the TX queue stays stopped?

It isn't clear that any shipping Kvaser firmware sends extended commands
shorter than 32 bytes, so this may only be theoretical.

Would it be safer to abort only on lengths that can't be skipped (shorter
than the 8-byte header)? The 32-byte requirement could then apply only to
CMD_TX_ACKNOWLEDGE_FD and CMD_RX_MESSAGE_FD.

> +		}
> +

[Severity: High]

The commit message says:

  "Treat a shorter length as a format error in both parsers."

Does that also cover the usb_rx_leftover reassembly path at the top of
kvaser_usb_hydra_read_bulk_callback()? There, cmd_len is computed from
usb_rx_leftover before the remaining bytes are copied in, and it is
never compared with sizeof(struct kvaser_cmd):

kvaser_usb_hydra_read_bulk_callback() {
    ...
	if (usb_rx_leftover_len) {
		...
		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
		...
		if (remaining_bytes + usb_rx_leftover_len == cmd_len) {
			kvaser_usb_hydra_handle_cmd(dev, cmd);
    ...
}

What happens if a transfer ends 1 to 5 bytes into a CMD_EXTENDED header,
before the le16 len field at bytes 4-5 has fully arrived?

In the main loop, the new check reads ext->len from bytes past len. The
URB buffer is reused, so those bytes are stale data from an earlier
transfer. If they give cmd_len >= 32, the partial header is saved as
leftover.

On the next callback, bytes 4-5 of usb_rx_leftover are stale as well.
For example, with 5 bytes saved, usb_rx_leftover[4] == 8 and
usb_rx_leftover[5] == 0 give cmd_len = 8 and remaining_bytes = 3. Since
3 + 5 == cmd_len, kvaser_usb_hydra_handle_cmd() runs on an 8-byte
extended command.

If cmd_no_ext is CMD_RX_MESSAGE_FD or CMD_TX_ACKNOWLEDGE_FD, the handlers
read fixed fields and payload up to byte 96 of usb_rx_leftover. That
stays inside the 128-byte array. The data is stale and not part of the
command, though, so it can produce bogus CAN frames or bogus TX
acknowledgements.

The new main-loop check can also reject a legitimately split command,
because it decides based on bytes that have not been received yet.

Would it make sense to add the minimum length check to the leftover
branch too? In both branches, ext->len could also be ignored until
offsetof(struct kvaser_cmd_ext, cmd_no_ext) bytes have arrived.

>  		if (pos + cmd_len > len) {
>  			/* We got first part of a command */
>  			int leftover_bytes;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-kvaser-usb-validate-v2-0-193dc10ca19b%40gmail.com

[PATCH v2 2/2] can: kvaser_usb: reject devices reporting zero max outstanding tx

[Joas Antonio dos Santos via B4 Relay]

From: Joas Antonio dos Santos <joasantonio108@gmail.com>

The number of tx contexts is taken from the max_outstanding_tx field of
the software info reply sent by the device.  The leaf driver uses the
value as is and the hydra driver only limits it to
KVASER_USB_MAX_TX_URBS, so a device can report zero.

dev->max_tx_urbs is then used as a divisor when a tx acknowledge is
received:

  kvaser_usb_leaf_tx_acknowledge()
	context = &priv->tx_contexts[tid % dev->max_tx_urbs];

  kvaser_usb_hydra_tx_acknowledge()
	context = &priv->tx_contexts[transid % dev->max_tx_urbs];

With max_tx_urbs == 0 a tx acknowledge sent by the device while the
interface is up is a division by zero in the URB completion handler on
architectures that trap on it.  On architectures that do not
trap, the index is not reduced at all and tx_contexts[], which was
allocated with zero elements, is accessed out of bounds.

A device that cannot accept any outstanding tx frame is not usable, so
fail the probe when it reports zero.

Found by LLM-assisted source review.

Fixes: 082d70b6142f ("can: kvaser_usb: Comply with firmware max tx URBs value")
Cc: stable@vger.kernel.org
Signed-off-by: Joas Antonio dos Santos <joasantonio108@gmail.com>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
index d0a2a2a33c1c..a915e529290b 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c
@@ -977,6 +977,12 @@ static int kvaser_usb_probe(struct usb_interface *intf,
 		goto free_devlink;
 	}
 
+	if (!dev->max_tx_urbs) {
+		dev_err(&intf->dev, "Invalid max outstanding tx (0)\n");
+		err = -ENODEV;
+		goto free_devlink;
+	}
+
 	if (ops->dev_get_software_details) {
 		err = ops->dev_get_software_details(dev);
 		if (err) {

-- 
2.43.0