Re: [PATCH 2/2] can: kvaser_usb_hydra: reject too-short commands in the receive path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Processing an unexpected or duplicate TX acknowledge command causes an out-of-bounds array access, leading to memory corruption.
- [High] kvaser_usb_hydra_cmd_size() reads the command length from the URB buffer without verifying sufficient data is available, and the patch's newly added minimum length check is insufficient to prevent out-of-bounds accesses during command dispatch.
- [High] The patch fails to validate command lengths in the leftover buffer path, leading to an integer underflow, and an error path fails to reset the leftover length, permanently wedging the receive path.
- [High] The dev_get_berr_counter implementation synchronously transmits a USB command using GFP_KERNEL allocations while running inside an RCU read-side critical section.
- [High] The driver blindly copies CAN frame payload data based on the device-supplied DLC without verifying that the command length is large enough, leading to a heap information leak.
- [High] The patch fixes an infinite loop in the URB completion callback but leaves the exact same vulnerability in the synchronous command wait function.
--

--- Patch [2]: [PATCH 2/2] can: kvaser_usb_hydra: reject too-short commands in the receive path ---
commit c7aa22e9c2e49b7c5c3858b9afe989871f77d49c
Author: Yiran Qiu <eritque-arcus@ikuyo.dev>

can: kvaser_usb_hydra: reject too-short commands in the receive path

This commit fixes an infinite loop in the URB-completion softirq caused by
extended commands with a length of zero. It adds a minimum-length check before
dispatching commands.

Cc: stable@vger.kernel.org

> 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..d44f9875fbe2f 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 +2156,15 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  
>  		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the call to
kvaser_usb_hydra_cmd_size() read the command length from the URB buffer
without verifying sufficient data is available?

If a malicious device sends an extended command split across URBs, or one with
a length of 4 or 5 bytes, kvaser_usb_hydra_cmd_size() reads the 16-bit length
field at offset 4:

kvaser_usb_hydra_cmd_size()
    ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);

This occurs without checking if the buffer has enough data, leading to an
out-of-bounds read.

> +		/* An extended command carries a device-supplied length; a
> +		 * command shorter than the command header would never advance
> +		 * @pos and would spin this URB-completion softirq forever.
> +		 */
> +		if (cmd_len < sizeof(struct kvaser_cmd_header)) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			break;
> +		}
> +

[Severity: High]
This is a pre-existing issue, but is the validation check added here
sufficient to prevent out-of-bounds accesses during command dispatch?

Since a command length of 4 or 5 passes this check, it allows
kvaser_usb_hydra_handle_cmd_ext() to be called. That function then reads
cmd_no_ext at offset 6:

kvaser_usb_hydra_handle_cmd_ext()
    switch (cmd->cmd_no_ext) {

Could this lead to an out-of-bounds array access on the URB buffer and
corrupted command processing?

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

[Severity: High]
This isn't a bug introduced by this patch, but earlier in
kvaser_usb_hydra_read_bulk_callback(), does the leftover buffer path fail to
validate command lengths?

If the leftover buffer evaluates cmd_len to 0, cmd_len - usb_rx_leftover_len
underflows to a massive unsigned integer. Once usb_rx_leftover_len
accumulates past 128 bytes, the driver prints an error and returns:

kvaser_usb_hydra_read_bulk_callback()
    if (remaining_bytes + usb_rx_leftover_len >
                                    KVASER_USB_HYDRA_MAX_CMD_LEN) {
            dev_err(&dev->intf->dev, "Format error\n");
            spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
            return;
    }

Crucially, it fails to reset usb_rx_leftover_len to 0. Would every subsequent
URB immediately hit this exact overflow condition and be dropped, permanently
wedging the device receive path?

[Severity: High]
This is a pre-existing issue, but does kvaser_usb_hydra_wait_cmd() contain the
exact same infinite loop vulnerability that this patch fixes?

kvaser_usb_hydra_wait_cmd() iterates over incoming USB data using a similar
loop. A malicious device could send an extended command with length 0, causing
cmd_len to evaluate to 0:

kvaser_usb_hydra_wait_cmd()
    pos += cmd_len;

Since the command is not the expected response, the loop adds 0 to pos and
spins forever without advancing. Will this lock up the kernel during device
probe or configuration?

[Severity: High]
This isn't a bug introduced by this patch, but does the
dev_get_berr_counter implementation synchronously transmit a USB command
using GFP_KERNEL allocations while running inside an RCU read-side critical
section?

The callback is invoked by can_fill_info() when responding to netlink
RTM_GETLINK dumps, which execute under rcu_read_lock(). Inside this RCU
critical section, kvaser_usb_hydra_get_berr_counter() dynamically allocates
memory with GFP_KERNEL and calls wait_for_completion_timeout() to submit a
synchronous URB:

kvaser_usb_hydra_get_berr_counter()
    err = kvaser_usb_hydra_send_simple_cmd(priv->dev,
                                           CMD_GET_CHIP_STATE_REQ,
                                           priv->channel);

Could this violate core kernel locking rules, leading to sleeping in atomic
context, lockdep splats, and potential RCU stalls?

[Severity: High]
This is a pre-existing issue, but does the driver blindly copy CAN frame
payload data based on the device-supplied DLC without verifying the command
length?

In kvaser_usb_hydra_rx_msg_ext(), the driver extracts the DLC and converts it
to a payload length. It then blindly copies cf->len bytes from the URB buffer:

kvaser_usb_hydra_rx_msg_ext()
    memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

If a malicious device sends an extended command with a small USB transfer
length but a large DLC, will the driver read out-of-bounds past the end of
the USB transfer buffer, and leak kernel heap memory to userspace?

[Severity: Critical]
This isn't a bug introduced by this patch, but does processing an
unexpected or duplicate TX acknowledge command cause an out-of-bounds array
access?

When processing a duplicate TX ACK, context->echo_index is reset to
dev->max_tx_urbs, which matches the size of the echo_skb array. The driver
directly accesses priv->can.echo_skb[context->echo_index] without bounds
checking:

kvaser_usb_hydra_tx_acknowledge()
    skb = priv->can.echo_skb[context->echo_index];
    if (skb)
            skb_hwtstamps(skb)->hwtstamp =
                               kvaser_usb_hydra_ktime_from_cmd(dev->cfg, cmd);

This accesses exactly one element past the end of the dynamically allocated
echo_skb array. If the out-of-bounds memory contains a non-zero value, it is
treated as a valid sk_buff pointer, and an 8-byte timestamp is written to it.
Could this lead to wild pointer writes and exploitable kernel memory
corruption?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260815-can-esd-hydra-fixes-v1-0-de644cbeaec2@ikuyo.dev?part=2