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

[sashiko-bot@kernel.org]

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