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

[sashiko-bot@kernel.org]

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