[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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] can: kvaser_usb: hydra: out-of-bounds read in
  kvaser_usb_hydra_cmd_size()
- [Medium] can: kvaser_usb: hydra: out-of-bounds read in
  kvaser_usb_hydra_rx_msg_ext()
--

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

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

This commit rejects commands whose length is smaller than sizeof(struct
kvaser_cmd) in kvaser_usb_hydra_wait_cmd() and
kvaser_usb_hydra_read_bulk_callback().

> 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
> @@ -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;

[Severity: Medium]
Can kvaser_usb_hydra_cmd_size() read past the received bytes in buf when
fewer than offsetofend(struct kvaser_cmd_ext, len) (6) bytes remain?

When tmp_cmd->header.cmd_no is CMD_EXTENDED, kvaser_usb_hydra_cmd_size()
reads ((struct kvaser_cmd_ext *)cmd)->len at byte offsets 4..5 before
checking how many bytes are left in the transfer.  If a full
KVASER_USB_RX_BUFFER_SIZE transfer ends with 1..5 trailing bytes and the
first byte is CMD_EXTENDED, this reads up to 4 bytes past the end of buf.

In kvaser_usb_hydra_read_bulk_callback(), a valid extended command split
across two URB transfers with fewer than 6 bytes in the first transfer
also reads unreceived bytes from buf[pos + 4..5] to compute cmd_len,
either tripping the new cmd_len < sizeof(struct kvaser_cmd) check or
saving fewer than 6 bytes into card_data->usb_rx_leftover, where the next
callback calls kvaser_usb_hydra_cmd_size() before the rest of the header
is copied in and without checking cmd_len < sizeof(struct kvaser_cmd).

> @@ -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;

[Severity: Medium]
Does checking cmd_len < sizeof(struct kvaser_cmd) prevent
kvaser_usb_hydra_rx_msg_ext() from reading past the end of cmd and buf?

In struct kvaser_cmd_ext, the fixed fields of rx_can end at byte 32, and
the union holding err_frame_data and kcan_payload[64] starts at byte 32:

drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_rx_msg_ext() {
	...
	if (flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME) {
		kvaser_usb_hydra_error_frame(priv, &cmd->rx_can.err_frame_data,
					     hwtstamp);
		return;
	}
	...
	if (flags & KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME) {
		cf->can_id |= CAN_RTR_FLAG;
	} else {
		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

		stats->rx_bytes += cf->len;
	}
}

A CMD_RX_MESSAGE_FD command with cmd_len == 32 passes this check, after
which kvaser_usb_hydra_rx_msg_ext() reads err_frame_data or copies up to
64 bytes from cmd->rx_can.kcan_payload (depending on the DLC in
kcan_header) past the end of the command and potentially past the end of
buf into the skb.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-kvaser-usb-validate-v2-0-193dc10ca19b@gmail.com?part=1

[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