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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · 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).

- [High] The new 32-byte floor (cmd_len < sizeof(struct kvaser_cmd)) only
  covers the fixed part of an extended command.
- [High] The commit message says 'Treat a shorter length as a format
  error in both parsers', but the usb_rx_leftover reassembly path in…
- [Medium] The patch rejects every CMD_EXTENDED command whose len is
  below 32 bytes, including extended command numbers the driver doesn't…

--

can: kvaser_usb: hydra: reject commands shorter than a standard command

This commit treats any command length below sizeof(struct kvaser_cmd)
(32 bytes) as a format error. The check is added to both
kvaser_usb_hydra_wait_cmd() and kvaser_usb_hydra_read_bulk_callback().
The goal is to prevent an infinite loop on zero-length extended commands
and to stop handlers from reading past short 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..9adb4eb36e394 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

[ ... ]

> @@ -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)) {

[Severity: High]

Does a 32-byte floor cover the whole extended RX command? The fixed part
of struct kvaser_cmd_ext_rx_can ends at byte 32. kcan_payload[64] and
err_frame_data come after it, and kvaser_usb_hydra_rx_msg_ext() never
checks the payload size against the command's len:

kvaser_usb_hydra_rx_msg_ext() {
    ...
	if (flags & KVASER_USB_HYDRA_CF_FLAG_FDF) {
		cf->len = can_fd_dlc2len(dlc);
    ...
		memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);
    ...
}

Take a device that sends CMD_EXTENDED / CMD_RX_MESSAGE_FD with len == 32,
KVASER_USB_HYDRA_CF_FLAG_FDF set and DLC 15 in kcan_header. That command
passes the new check, and the memcpy() then reads bytes 32..95 of a
32-byte command.

An error frame reads err_frame_data at bytes 32..39. A classic frame
reads up to 8 bytes past the end.

In this function the buffer is the 3072-byte URB buffer. Suppose the
32-byte extended command ends exactly at byte 3072, for example after 95
standard commands. Can the memcpy() then read up to 64 bytes past the end
of that allocation?

For HCDs without DMA, such as vhci_hcd, hcd_buffer_alloc() uses
kmalloc(). The bytes read end up in a CAN FD frame passed to netif_rx(),
where any CAN socket can read them.

The commit message says:

  "so a shorter extended command makes the handlers read bytes that are
  not part of the command."

That kind of read still looks reachable here. The TX side sets len to
include the payload:

kvaser_usb_hydra_frame_to_cmd_ext() {
    ...
	*cmd_len = ALIGN(sizeof(struct kvaser_cmd_ext) -
			 sizeof(cmd->tx_can.kcan_payload) + nbr_of_bytes,
			 8);
    ...
}

Should the handler, or the parser using cmd_len, check the offset of
kcan_payload plus cf->len (or plus sizeof(err_frame_data)) against
cmd_len?

> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;

[Severity: Medium]

Does this floor also reject well-formed extended commands that the driver
just doesn't handle? The extended format only needs the 8-byte header.
kvaser_usb_hydra_handle_cmd_ext() already has a default branch for
unknown commands, and that branch only reads header bytes:

kvaser_usb_hydra_handle_cmd_ext() {
    ...
	default:
		dev_warn(&dev->intf->dev, "Unhandled extended command (%d)\n",
			 cmd->header.cmd_no);
		break;
    ...
}

Before this patch, an unknown extended command of 8 to 31 bytes was
skipped with pos += cmd_len, and the rest of the transfer was still
parsed. The commit message only shows that the extended commands the
driver handles or sends are at least 32 bytes.

With the return here, everything after such a command in the URB is
dropped, including CMD_TX_ACKNOWLEDGE / CMD_TX_ACKNOWLEDGE_FD and RX
frames. A following command that crosses the URB boundary is not saved
as leftover either.

kvaser_usb_hydra_tx_acknowledge() is the only place that does these
things:

- releases the echo skb
- resets context->echo_index
- decrements active_tx_contexts
- wakes the queue

Could each lost ACK leak a TX context, until the TX queue stays stopped?

It isn't clear that any shipping Kvaser firmware sends extended commands
shorter than 32 bytes, so this may only be theoretical.

Would it be safer to abort only on lengths that can't be skipped (shorter
than the 8-byte header)? The 32-byte requirement could then apply only to
CMD_TX_ACKNOWLEDGE_FD and CMD_RX_MESSAGE_FD.

> +		}
> +

[Severity: High]

The commit message says:

  "Treat a shorter length as a format error in both parsers."

Does that also cover the usb_rx_leftover reassembly path at the top of
kvaser_usb_hydra_read_bulk_callback()? There, cmd_len is computed from
usb_rx_leftover before the remaining bytes are copied in, and it is
never compared with sizeof(struct kvaser_cmd):

kvaser_usb_hydra_read_bulk_callback() {
    ...
	if (usb_rx_leftover_len) {
		...
		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
		...
		if (remaining_bytes + usb_rx_leftover_len == cmd_len) {
			kvaser_usb_hydra_handle_cmd(dev, cmd);
    ...
}

What happens if a transfer ends 1 to 5 bytes into a CMD_EXTENDED header,
before the le16 len field at bytes 4-5 has fully arrived?

In the main loop, the new check reads ext->len from bytes past len. The
URB buffer is reused, so those bytes are stale data from an earlier
transfer. If they give cmd_len >= 32, the partial header is saved as
leftover.

On the next callback, bytes 4-5 of usb_rx_leftover are stale as well.
For example, with 5 bytes saved, usb_rx_leftover[4] == 8 and
usb_rx_leftover[5] == 0 give cmd_len = 8 and remaining_bytes = 3. Since
3 + 5 == cmd_len, kvaser_usb_hydra_handle_cmd() runs on an 8-byte
extended command.

If cmd_no_ext is CMD_RX_MESSAGE_FD or CMD_TX_ACKNOWLEDGE_FD, the handlers
read fixed fields and payload up to byte 96 of usb_rx_leftover. That
stays inside the 128-byte array. The data is stale and not part of the
command, though, so it can produce bogus CAN frames or bogus TX
acknowledgements.

The new main-loop check can also reject a legitimately split command,
because it decides based on bytes that have not been received yet.

Would it make sense to add the minimum length check to the leftover
branch too? In both branches, ext->len could also be ignored until
offsetof(struct kvaser_cmd_ext, cmd_no_ext) bytes have arrived.

>  		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/20261111-kvaser-usb-validate-v2-0-193dc10ca19b%40gmail.com