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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch says it rejects commands shorter than the header.

--

can: kvaser_usb: hydra: reject commands shorter than the header

This commit adds a lower bound on the command length returned by
kvaser_usb_hydra_cmd_size(). The check is added in both
kvaser_usb_hydra_wait_cmd() and kvaser_usb_hydra_read_bulk_callback(), so
a CMD_EXTENDED command with a zero length no longer causes an infinite
parse loop. Lengths shorter than struct kvaser_cmd_header are now treated
as format errors.

> 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;
> +		}
> +

[Severity: Medium]
Is struct kvaser_cmd_header the right lower bound here?

kvaser_usb_hydra_cmd_size() returns sizeof(struct kvaser_cmd), which is
32, for every non-extended command. So this check only has an effect for
CMD_EXTENDED.

The fixed header of an extended command is the 8-byte prefix of struct
kvaser_cmd_ext, and the length field is part of that prefix:

struct kvaser_cmd_ext {
	struct kvaser_cmd_header header;
	__le16 len;
	u8 cmd_no_ext;
	u8 reserved;
	...

With a 4-byte bound, a CMD_EXTENDED with len between 4 and 7 still passes
this check, and also the new check in kvaser_usb_hydra_wait_cmd(). In the
bulk callback the command is then handled like this:

kvaser_usb_hydra_read_bulk_callback()
  kvaser_usb_hydra_handle_cmd()
    kvaser_usb_hydra_handle_cmd_ext()
      switch (cmd->cmd_no_ext)
      kvaser_usb_hydra_rx_msg_ext()
        memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

Both cmd_no_ext (offset 6) and rx_can (from offset 8) are outside the
declared command. So the dispatch decision and the CAN frame passed to
netif_rx() come from the next command in the transfer, or from stale URB
buffer bytes. Parsing then resumes 4 to 7 bytes later, in the middle of
what was just treated as payload.

The commit message says "A command can never be shorter than its header".
The leaf fix it cites, 0c73772cd2b8, bounds by CMD_HEADER_LEN, and that
header includes the length field.

Should the CMD_EXTENDED minimum be at least
offsetof(struct kvaser_cmd_ext, rx_can)? For reference,
kvaser_usb_hydra_frame_to_cmd_ext() always builds extended commands of at
least 32 bytes, aligned to 8.

A complete fix would also need per-sub-command minimum sizes, similar to
kvaser_usb_leaf_verify_size(). The extended handlers already read up to 96
bytes based on dlc, whatever cmd_len says. That over-read predates this
patch, and neither bound fixes it.

The later patch in this series, "can: kvaser_usb: reject devices reporting
zero max outstanding tx", only changes probe in kvaser_usb_core.c and does
not touch these parsers.

>  		if (pos + cmd_len > len) {
>  			/* We got first part of a command */
>  			int leftover_bytes;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-kvaser-usb-validate-v1-0-f266b85aa908%40gmail.com