Re: [PATCH net] can: kvaser_usb: validate command format before parsing in hydra receive path

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

Pre-existing issues:
- [High] Missing structural minimum length validation for CMD_EXTENDED payloads allows slab-out-of-bounds reads and potential kernel memory leaks.
- [High] Leftover buffer parsing errors fail to reset the buffer state, causing a permanent denial of service on the CAN interface.
--

--- Patch [1]: [PATCH net] can: kvaser_usb: validate command format before parsing in hydra receive path ---
commit 656b69d0272cad412a6756864385d2b4307a6e5f
Author: Cen Zhang (Microsoft) <[email protected]>

can: kvaser_usb: validate command format before parsing in hydra receive path

This patch addresses slab-out-of-bounds reads and infinite loops in the
kvaser_usb hydra receive path. It introduces a receive-path wrapper,
kvaser_usb_hydra_cmd_size_rx(), to validate buffer bounds before reading
the command size.

> 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..a964edde5783c 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

[ ... ]

> @@ -2124,7 +2139,14 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  
>  		cmd = (struct kvaser_cmd *)card_data->usb_rx_leftover;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		cmd_len = kvaser_usb_hydra_cmd_size_rx(cmd,
> +						       KVASER_USB_HYDRA_MAX_CMD_LEN);
> +
> +		if (!cmd_len) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			spin_unlock_irqrestore(usb_rx_leftover_lock, irq_flags);
> +			return;
> +		}

[Severity: High]
This is a pre-existing issue, but do these error paths need to reset
usb_rx_leftover_len to 0?

When an error is detected here (or in the subsequent pre-existing overflow 
check in kvaser_usb_hydra_read_bulk_callback), the code logs a format error,
drops the lock, and returns, but it fails to reset
card_data->usb_rx_leftover_len to 0.

On the next USB IN transfer, the non-zero leftover length forces execution back
into the leftover parsing block. The logic evaluates the exact same corrupted
command buffer, fails the exact same error check, and returns again.

Can this permanently wedge the parser and cause a denial of service on the CAN
interface?

[ ... ]

> @@ -2154,7 +2176,11 @@ static void kvaser_usb_hydra_read_bulk_callback(struct kvaser_usb *dev,
>  	while (pos < len) {
>  		cmd = buf + pos;
>  
> -		cmd_len = kvaser_usb_hydra_cmd_size(cmd);
> +		cmd_len = kvaser_usb_hydra_cmd_size_rx(cmd, len - pos);
> +		if (!cmd_len) {
> +			dev_err(&dev->intf->dev, "Format error\n");
> +			return;
> +		}
>  
>  		if (pos + cmd_len > len) {
>  			/* We got first part of a command */

[Severity: High]
This isn't a bug introduced by this patch, but does the parser loop trust 
cmd_len (derived from the attacker-controlled cmd->len field) without
validating that it meets the minimum structural size expected by the command
handler?

For example, if a device claims a length of 8, the parser only ensures 8 bytes
are present. The command is then dispatched to handlers like 
kvaser_usb_hydra_rx_msg_ext(), which unconditionally read fields at fixed
offsets far beyond the claimed length:

drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_rx_msg_ext()
    ...
    kcan_header = le32_to_cpu(cmd->rx_can.kcan_header);
    dlc = (kcan_header & KVASER_USB_KCAN_DATA_DLC_MASK) >>
        KVASER_USB_KCAN_DATA_DLC_SHIFT;
    ...
    memcpy(cf->data, cmd->rx_can.kcan_payload, cf->len);

Could this result in a slab-out-of-bounds read and potential kernel memory leak?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1