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

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

[Joas Antonio dos Santos via B4 Relay]

Four 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.

Patch 3 stops the hydra command parsers from reading the length of an
extended command before it has been received.

Patch 4 checks the error frame data and the payload of extended rx
commands against the command length.

These issues were found with the help of an LLM-based code review tool
(Claude); patches 3 and 4 address the points of the Sashiko review of
v2.  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.  In particular patch 4 assumes
that the firmware includes the error frame data and the payload in the
command length, which follows from how the parsers advance through the
buffer but which I could not confirm on a device.

---
Changes in v3:
- New patch 3: do not read the extended command length before bytes 4
  and 5 are received, in kvaser_usb_hydra_wait_cmd() and
  kvaser_usb_hydra_read_bulk_callback() (sashiko review of v2).
- New patch 4: check err_frame_data and kcan_payload against the command
  length in kvaser_usb_hydra_rx_msg_ext() (sashiko review of v2).
- Patches 1 and 2: unchanged.
- Link to v2: https://lore.kernel.org/r/20261111-kvaser-usb-validate-v2-0-193dc10ca19b@gmail.com

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 (4):
      can: kvaser_usb: hydra: reject commands shorter than a standard command
      can: kvaser_usb: reject devices reporting zero max outstanding tx
      can: kvaser_usb: hydra: do not read the command length before it is received
      can: kvaser_usb: hydra: check the payload of extended rx commands

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

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

[PATCH v3 1/4] 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

[PATCH v3 2/4] 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

[PATCH v3 3/4] can: kvaser_usb: hydra: do not read the command length before it is received

[Joas Antonio dos Santos via B4 Relay]

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

kvaser_usb_hydra_cmd_size() reads the 16 bit length of an extended
command from bytes 4 and 5 of the command.  The two command parsers call
it without checking that those bytes have been received:

  - kvaser_usb_hydra_wait_cmd() only requires pos < actual_len.  If the
    transfer fills the whole KVASER_USB_RX_BUFFER_SIZE buffer and the
    last command starts in its last five bytes, the length is read from
    beyond the end of the kzalloc()ed buffer.

  - kvaser_usb_hydra_read_bulk_callback() has the same problem at the
    end of the receive buffer.  In addition, an extended command that is
    split so that fewer than six bytes arrive in the first transfer is
    parsed with a length taken from stale buffer content.  That either
    fails the length check or stores the fragment in usb_rx_leftover,
    where the next callback computes the length again before the rest of
    the header has been copied in, and without checking it against
    sizeof(struct kvaser_cmd).

Every command is at least sizeof(struct kvaser_cmd) long and
kvaser_usb_hydra_wait_cmd() does not handle partial commands, so require
that many bytes there before looking at the command.

In kvaser_usb_hydra_read_bulk_callback(), buffer the fragment when the
length field is not complete, and complete the length field in
usb_rx_leftover before using it.  Apply the minimum length check to the
buffered command too.  Drop the buffered fragment when that check or
the existing check against the size of usb_rx_leftover fails; the
latter kept the fragment, so that the same error was hit again on the
following transfers.

Found by the Sashiko AI review of the previous version of this series.

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 | 50 +++++++++++++++++++----
 1 file changed, 43 insertions(+), 7 deletions(-)

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 9adb4eb36e39..6b68e0714ae3 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -675,6 +675,12 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
 			size_t cmd_len;
 
 			tmp_cmd = buf + pos;
+			if (actual_len - pos < sizeof(struct kvaser_cmd)) {
+				dev_err_ratelimited(&dev->intf->dev,
+						    "Format error\n");
+				break;
+			}
+
 			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
 			if (cmd_len < sizeof(struct kvaser_cmd) ||
 			    pos + cmd_len > actual_len) {
@@ -2117,6 +2123,8 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 							&dev->card_data.hydra;
 	int usb_rx_leftover_len;
 	spinlock_t *usb_rx_leftover_lock = &card_data->usb_rx_leftover_lock;
+	/* Number of bytes needed to tell the length of any command */
+	const int len_end = offsetofend(struct kvaser_cmd_ext, len);
 
 	spin_lock_irqsave(usb_rx_leftover_lock, irq_flags);
 	usb_rx_leftover_len = card_data->usb_rx_leftover_len;
@@ -2125,20 +2133,43 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 
 		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
 
+		if (usb_rx_leftover_len < len_end) {
+			/* Get the rest of the length field first */
+			remaining_bytes = min(len,
+					      len_end - usb_rx_leftover_len);
+			memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len,
+			       buf, remaining_bytes);
+			pos += remaining_bytes;
+			usb_rx_leftover_len += remaining_bytes;
+			card_data->usb_rx_leftover_len = usb_rx_leftover_len;
+			if (usb_rx_leftover_len < len_end) {
+				spin_unlock_irqrestore(usb_rx_leftover_lock,
+						       irq_flags);
+				return;
+			}
+		}
+
 		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		if (cmd_len < sizeof(struct kvaser_cmd)) {
+			dev_err(&dev->intf->dev, "Format error\n");
+			card_data->usb_rx_leftover_len = 0;
+			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
+			return;
+		}
 
-		remaining_bytes = min_t(unsigned int, len,
+		remaining_bytes = min_t(unsigned int, len - pos,
 					cmd_len - usb_rx_leftover_len);
 		/* Make sure we do not overflow usb_rx_leftover */
 		if (remaining_bytes + usb_rx_leftover_len >
 						KVASER_USB_HYDRA_MAX_CMD_LEN) {
 			dev_err(&dev->intf->dev, "Format error\n");
+			card_data->usb_rx_leftover_len = 0;
 			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
 			return;
 		}
 
-		memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf,
-		       remaining_bytes);
+		memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len,
+		       buf + pos, remaining_bytes);
 		pos += remaining_bytes;
 
 		if (remaining_bytes + usb_rx_leftover_len == cmd_len) {
@@ -2155,11 +2186,16 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 	while (pos < len) {
 		cmd = buf + pos;
 
-		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		if (len - pos < len_end) {
+			/* The length is not known yet, buffer what we got */
+			cmd_len = sizeof(struct kvaser_cmd);
+		} else {
+			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 (cmd_len < sizeof(struct kvaser_cmd)) {
+				dev_err(&dev->intf->dev, "Format error\n");
+				return;
+			}
 		}
 
 		if (pos + cmd_len > len) {

-- 
2.43.0

[PATCH v3 4/4] can: kvaser_usb: hydra: check the payload of extended rx commands

[Joas Antonio dos Santos via B4 Relay]

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

kvaser_usb_hydra_rx_msg_ext() reads the error frame data or copies up
to 64 bytes of CAN FD payload from an extended command.  Both start at
byte 32 of the command, and the number of payload bytes is derived from
the DLC in kcan_header.  Neither is checked against the length of the
command, which the parsers only guarantee to be at least
sizeof(struct kvaser_cmd), 32 bytes.

A device can therefore send a short CMD_RX_MESSAGE_FD command with a
large DLC and make the driver copy up to 64 bytes that follow the
command into the CAN frame that is passed to user space: stale data of
earlier transfers in the receive buffer or in usb_rx_leftover, or data
beyond the 3072 bytes of the receive buffer if the command is the last
one in a full transfer.

Check that the command contains the error frame data and the payload
before using them, and count the command as an rx error otherwise.

Found by the Sashiko AI review of the previous version of this series.

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 | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

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 6b68e0714ae3..387a522c60fa 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -1308,6 +1308,7 @@ static void kvaser_usb_hydra_rx_msg_ext(const struct kvaser_usb *dev,
 	struct sk_buff *skb;
 	struct skb_shared_hwtstamps *shhwtstamps;
 	struct net_device_stats *stats;
+	size_t cmd_len = le16_to_cpu(cmd->len);
 	u32 flags;
 	u8 dlc;
 	u32 kcan_header;
@@ -1327,6 +1328,12 @@ static void kvaser_usb_hydra_rx_msg_ext(const struct kvaser_usb *dev,
 	hwtstamp = kvaser_usb_hydra_ktime_from_cmd(dev->cfg, std_cmd);
 
 	if (flags & KVASER_USB_HYDRA_CF_FLAG_ERROR_FRAME) {
+		if (cmd_len < offsetofend(struct kvaser_cmd_ext,
+					  rx_can.err_frame_data)) {
+			stats->rx_errors++;
+			return;
+		}
+
 		kvaser_usb_hydra_error_frame(priv, &cmd->rx_can.err_frame_data,
 					     hwtstamp);
 		return;
@@ -1370,6 +1377,14 @@ static void kvaser_usb_hydra_rx_msg_ext(const struct kvaser_usb *dev,
 	if (flags & KVASER_USB_HYDRA_CF_FLAG_REMOTE_FRAME) {
 		cf->can_id |= CAN_RTR_FLAG;
 	} else {
+		/* The payload must be part of the received command */
+		if (cmd_len < offsetof(struct kvaser_cmd_ext,
+				       rx_can.kcan_payload) + cf->len) {
+			stats->rx_errors++;
+			kfree_skb(skb);
+			return;
+		}
+
 		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
 
 		stats->rx_bytes += cf->len;

-- 
2.43.0