[PATCH] can: kvaser_usb: hydra: validate command prefixes before parsing

[PATCH] can: kvaser_usb: hydra: validate command prefixes before parsing

[Pengpeng Hou]

The Hydra receive paths derive a command length from the current buffer
before
checking that a complete command header is available. For an extended
command
this also reads the extended length field before proving that field is in
the
current USB transfer or saved fragment.

Validate the header and extended-length prefix before decoding a command
length. Keep partial prefixes in the existing leftover buffer until enough
bytes arrive to establish the command extent, and reject invalid extents.

Fixes: aec5fb2268b7 ("can: kvaser_usb: Add support for Kvaser USB hydra family")
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c | 76 +++++++++++++++++++++--
 1 file changed, 70 insertions(+), 6 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 efbb7bed34c9d..92c78fa690281 100644
--- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
+++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
@@ -536,6 +536,30 @@ static size_t kvaser_usb_hydra_cmd_size(struct kvaser_cmd *cmd)
 	return ret;
 }
 
+static int kvaser_usb_hydra_cmd_len(const void *buf, size_t len,
+				    size_t *cmd_len)
+{
+	const struct kvaser_cmd *cmd = buf;
+
+	if (len < sizeof(cmd->header))
+		return -EMSGSIZE;
+
+	if (cmd->header.cmd_no != CMD_EXTENDED) {
+		*cmd_len = sizeof(*cmd);
+		return 0;
+	}
+
+	if (len < offsetofend(struct kvaser_cmd_ext, len))
+		return -EMSGSIZE;
+
+	*cmd_len = le16_to_cpu(((const struct kvaser_cmd_ext *)cmd)->len);
+	if (*cmd_len < offsetofend(struct kvaser_cmd_ext, cmd_no_ext) ||
+	    *cmd_len > KVASER_USB_HYDRA_MAX_CMD_LEN)
+		return -EINVAL;
+
+	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)
@@ -675,8 +699,10 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
 			size_t cmd_len;
 
 			tmp_cmd = buf + pos;
-			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
-			if (pos + cmd_len > actual_len) {
+			err = kvaser_usb_hydra_cmd_len(tmp_cmd, actual_len - pos,
+						       &cmd_len);
+			if (err || cmd_len > actual_len - pos ||
+			    cmd_len > sizeof(*cmd)) {
 				dev_err_ratelimited(&dev->intf->dev,
 						    "Format error\n");
 				break;
@@ -2121,12 +2147,44 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 	usb_rx_leftover_len = card_data->usb_rx_leftover_len;
 	if (usb_rx_leftover_len) {
 		int remaining_bytes;
+		int err;
+		size_t needed;
 
 		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
 
-		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		while (true) {
+			err = kvaser_usb_hydra_cmd_len(cmd, usb_rx_leftover_len,
+						       &cmd_len);
+			if (err != -EMSGSIZE)
+				break;
+
+			needed = sizeof(cmd->header);
+			if (usb_rx_leftover_len >= needed &&
+			    cmd->header.cmd_no == CMD_EXTENDED)
+				needed = offsetofend(struct kvaser_cmd_ext, len);
+
+			remaining_bytes = min_t(unsigned int, len - pos,
+						needed - usb_rx_leftover_len);
+			memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len,
+			       buf + pos, remaining_bytes);
+			usb_rx_leftover_len += remaining_bytes;
+			pos += remaining_bytes;
+
+			if (remaining_bytes == 0) {
+				card_data->usb_rx_leftover_len = usb_rx_leftover_len;
+				spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
+				return;
+			}
+		}
 
-		remaining_bytes = min_t(unsigned int, len,
+		if (err) {
+			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 - pos,
 					cmd_len - usb_rx_leftover_len);
 		/* Make sure we do not overflow usb_rx_leftover */
 		if (remaining_bytes + usb_rx_leftover_len >
@@ -2152,11 +2210,17 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
 	spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
 
 	while (pos < len) {
+		int err;
+
 		cmd = buf + pos;
 
-		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
+		err = kvaser_usb_hydra_cmd_len(cmd, len - pos, &cmd_len);
+		if (err == -EINVAL) {
+			dev_err(&dev->intf->dev, "Format error\n");
+			return;
+		}
 
-		if (pos + cmd_len > len) {
+		if (err == -EMSGSIZE || cmd_len > len - pos) {
 			/* We got first part of a command */
 			int leftover_bytes;

base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
-- 
2.50.1

Re: [PATCH] can: kvaser_usb: hydra: validate command prefixes before parsing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Incorrect command length check in `kvaser_usb_hydra_wait_cmd` incorrectly drops valid extended commands.
- [High] Incorrect memory copy source pointer corrupts fragmented command assembly.
--

commit 1cbbe99543052600da4f97d992d3e51a35e561aa
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: kvaser_usb: hydra: validate command prefixes before parsing

The Hydra receive paths derive a command length from the current buffer
before checking that a complete command header is available. This commit
validates the header and extended-length prefix before decoding a command
length.

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

[ ... ]

> @@ -675,8 +699,10 @@ static int kvaser_usb_hydra_wait_cmd(const struct kvaser_usb *dev, u8 cmd_no,
>  			size_t cmd_len;
>  
>  			tmp_cmd = buf + pos;
> -			cmd_len = kvaser_usb_hydra_cmd_size(tmp_cmd);
> -			if (pos + cmd_len > actual_len) {
> +			err = kvaser_usb_hydra_cmd_len(tmp_cmd, actual_len - pos,
> +						       &cmd_len);
> +			if (err || cmd_len > actual_len - pos ||
> +			    cmd_len > sizeof(*cmd)) {

[Severity: High]
Does this cmd_len > sizeof(*cmd) check unintentionally reject valid
extended commands?

Since kvaser_usb_hydra_cmd_len() correctly calculates the length of extended
commands (which can be larger than sizeof(struct kvaser_cmd)), it seems this
check will evaluate to true and trigger a format error when an extended
command arrives, terminating buffer processing prematurely.

>  				dev_err_ratelimited(&dev->intf->dev,
>  						    "Format error\n");
>  				break;

[ ... ]

> @@ -2121,12 +2147,44 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,

[ ... ]

> +		if (err) {
> +			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 - pos,
>  					cmd_len - usb_rx_leftover_len);
>  		/* Make sure we do not overflow usb_rx_leftover */
>  		if (remaining_bytes + usb_rx_leftover_len >

[Severity: High]
Does the leftover command assembly copy from the wrong source pointer?

The new while loop added just above this snippet can advance pos as it
consumes bytes to complete the command header. However, looking slightly
further down in kvaser_usb_hydra_read_bulk_callback(), the memcpy() copying
the remaining payload still reads directly from buf rather than buf + pos:

    memcpy(card_data->usb_rx_leftover + usb_rx_leftover_len, buf,
           remaining_bytes);
    pos += remaining_bytes;

If pos was advanced by the header completion loop, won't this copy duplicate
header bytes from the beginning of the USB transfer instead of the actual
remainder of the command payload?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830125322.98199-1-pengpeng@iscas.ac.cn?part=1