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

[PATCH 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 the command
header, which otherwise stall the command loops.

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.

---
Joas Antonio dos Santos (2):
      can: kvaser_usb: hydra: reject commands shorter than the header
      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 1/2] can: kvaser_usb: hydra: reject commands shorter than the header

[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 command can never be shorter than its header, so 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..92bfdcb04b01 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_header) ||
+			    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_header)) {
+			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 1/2] can: kvaser_usb: hydra: reject commands shorter than the header

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: kvaser_usb: incomplete extended cmd_len check in hydra
  parsers
--

commit a17f31efe99f81a18058ca235af35d386b0380de
Author: Joas Antonio dos Santos <joasantonio108@gmail.com>

can: kvaser_usb: hydra: reject commands shorter than the header

This commit adds a minimum command length check against
sizeof(struct kvaser_cmd_header) in kvaser_usb_hydra_wait_cmd() and
kvaser_usb_hydra_read_bulk_callback() to prevent infinite loops on
zero-length 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..92bfdcb04b019 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_header) ||
> +			    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_header)) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}
> +
>  		if (pos + cmd_len > len) {
>  			/* We got first part of a command */
>  			int leftover_bytes;

[Severity: Medium]
Can an extended command with cmd_len between 4 and 31 still pass this
check and trigger an out-of-bounds read in kvaser_usb_hydra_handle_cmd_ext()?

kvaser_usb_hydra_cmd_size() only returns a device-supplied length when
cmd->header.cmd_no == CMD_EXTENDED; standard commands always return
sizeof(struct kvaser_cmd) (32 bytes):

drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_cmd_size() {
	if (cmd->header.cmd_no == CMD_EXTENDED)
		ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
	else
		ret = sizeof(struct kvaser_cmd);
}

In struct kvaser_cmd_ext, the extended header itself is 8 bytes (len is at
bytes 4..5 and cmd_no_ext is at byte 6), and the handlers reached via
kvaser_usb_hydra_handle_cmd_ext() (kvaser_usb_hydra_tx_acknowledge() and
kvaser_usb_hydra_rx_msg_ext()) read at least 32 bytes from cmd without
re-checking cmd_len. Checking against sizeof(struct kvaser_cmd_header) (4
bytes) lets a CMD_EXTENDED frame with cmd_len in 4..31 near the end of
buf pass pos + cmd_len <= len and read past the end of the URB buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-kvaser-usb-validate-v1-0-f266b85aa908@gmail.com?part=1

Re: [PATCH 1/2] can: kvaser_usb: hydra: reject commands shorter than the header

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] The patch says it rejects commands shorter than the header.

--

can: kvaser_usb: hydra: reject commands shorter than the header

This commit adds a lower bound on the command length returned by
kvaser_usb_hydra_cmd_size(). The check is added in both
kvaser_usb_hydra_wait_cmd() and kvaser_usb_hydra_read_bulk_callback(), so
a CMD_EXTENDED command with a zero length no longer causes an infinite
parse loop. Lengths shorter than struct kvaser_cmd_header are now treated
as format errors.

> 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..92bfdcb04b019 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_header) ||
> +			    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_header)) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}
> +

[Severity: Medium]
Is struct kvaser_cmd_header the right lower bound here?

kvaser_usb_hydra_cmd_size() returns sizeof(struct kvaser_cmd), which is
32, for every non-extended command. So this check only has an effect for
CMD_EXTENDED.

The fixed header of an extended command is the 8-byte prefix of struct
kvaser_cmd_ext, and the length field is part of that prefix:

struct kvaser_cmd_ext {
	struct kvaser_cmd_header header;
	__le16 len;
	u8 cmd_no_ext;
	u8 reserved;
	...

With a 4-byte bound, a CMD_EXTENDED with len between 4 and 7 still passes
this check, and also the new check in kvaser_usb_hydra_wait_cmd(). In the
bulk callback the command is then handled like this:

kvaser_usb_hydra_read_bulk_callback()
  kvaser_usb_hydra_handle_cmd()
    kvaser_usb_hydra_handle_cmd_ext()
      switch (cmd->cmd_no_ext)
      kvaser_usb_hydra_rx_msg_ext()
        memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

Both cmd_no_ext (offset 6) and rx_can (from offset 8) are outside the
declared command. So the dispatch decision and the CAN frame passed to
netif_rx() come from the next command in the transfer, or from stale URB
buffer bytes. Parsing then resumes 4 to 7 bytes later, in the middle of
what was just treated as payload.

The commit message says "A command can never be shorter than its header".
The leaf fix it cites, 0c73772cd2b8, bounds by CMD_HEADER_LEN, and that
header includes the length field.

Should the CMD_EXTENDED minimum be at least
offsetof(struct kvaser_cmd_ext, rx_can)? For reference,
kvaser_usb_hydra_frame_to_cmd_ext() always builds extended commands of at
least 32 bytes, aligned to 8.

A complete fix would also need per-sub-command minimum sizes, similar to
kvaser_usb_leaf_verify_size(). The extended handlers already read up to 96
bytes based on dlc, whatever cmd_len says. That over-read predates this
patch, and neither bound fixes it.

The later patch in this series, "can: kvaser_usb: reject devices reporting
zero max outstanding tx", only changes probe in kvaser_usb_core.c and does
not touch these parsers.

>  		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/20261110-kvaser-usb-validate-v1-0-f266b85aa908%40gmail.com

[PATCH 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