Re: [PATCH] can: kvaser_usb_leaf: validate received command extents

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

Pre-existing issues:
- [High] A malicious USB device can cause an infinite loop in the driver's receive paths by presenting a bulk endpoint with a wMaxPacketSize of 0.
- [High] Missing length bounds validation in Hydra command parsing allows out-of-bounds reads and infinite loops.
- [High] The driver does not validate that `max_outstanding_tx` is non-zero, allowing a malicious device to trigger a divide-by-zero panic.
--

--- Patch [1]: [PATCH] can: kvaser_usb_leaf: validate received command extents ---
commit 25c3dede7931a0e156ff3db1ab14070a92252c33
Author: Pengpeng Hou <[email protected]>

can: kvaser_usb_leaf: validate received command extents

The wait and bulk receive paths walk variable-length commands from a
USB buffer. A nonzero command shorter than CMD_HEADER_LEN can still be
dispatched, and the wait path copies a matching command into a fixed
caller-owned struct kvaser_cmd using the device-provided length. Reject
nonzero commands that do not contain the fixed header or that extend
beyond the current USB buffer item. In the wait path, also reject a
matching command that exceeds the destination before copying it.

> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> index df737cfc5ea0c..a876c7819b81c 100644
> --- a/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> +++ b/drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c
> @@ -691,13 +691,22 @@ static int kvaser_usb_leaf_wait_cmd(const struct kvaser_usb *dev, u8 id,
>  				continue;
>  			}

[Severity: High]
This isn't a bug introduced by this patch, but looking at the lines just above
this in kvaser_usb_leaf_wait_cmd():

			if (tmp->len == 0) {
				pos = round_up(pos + 1,
					       le16_to_cpu
						(dev->bulk_in->wMaxPacketSize));
				continue;
			}

If a malicious USB device supplies a wMaxPacketSize of 0, will round_up()
evaluate to 0?

Does this cause the loop variable pos to continually reset to 0 and create
an infinite loop?

>  
> -			if (pos + tmp->len > actual_len) {
> +			if (tmp->len < CMD_HEADER_LEN ||
> +			    tmp->len > actual_len - pos) {
>  				dev_err_ratelimited(&dev->intf->dev,
>  						    "Format error\n");
>  				break;
>  			}

[ ... ]

> @@ -1737,7 +1746,7 @@ static void kvaser_usb_leaf_read_bulk_callback(struct kvaser_usb *dev,
>  			continue;
>  		}

[Severity: High]
This is a pre-existing issue, but the same round_up() logic with wMaxPacketSize
appears right above here in kvaser_usb_leaf_read_bulk_callback():

		if (cmd->len == 0) {
			pos = round_up(pos + 1, le16_to_cpu
						(dev->bulk_in->wMaxPacketSize));
			continue;
		}

Can this also result in an infinite loop in softirq context if wMaxPacketSize
is 0?

>  
> -		if (pos + cmd->len > len) {
> +		if (cmd->len < CMD_HEADER_LEN || cmd->len > len - pos) {
>  			dev_err_ratelimited(&dev->intf->dev, "Format error\n");
>  			break;
>  		}

[Severity: High]
This is a pre-existing issue, but does kvaser_usb_hydra.c have a similar lack
of length bounds validation?

In kvaser_usb_hydra_read_bulk_callback() and kvaser_usb_hydra_wait_cmd():

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);
    ...
}

If a fragmented or malicious packet leaves fewer than 6 bytes, could reading
the len field at offset 4 access out-of-bounds memory?

Additionally, if the extracted cmd_len is 0:

drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:kvaser_usb_hydra_read_bulk_callback() {
    ...
    cmd_len = kvaser_usb_hydra_cmd_size(cmd);
    if (pos + cmd_len > len) {
        ...
    }
    ...
    pos += cmd_len;
}

Does the boundary check (pos + cmd_len > len) evaluate to false since pos < len,
causing the driver to update pos += 0 and loop infinitely?

[Severity: High]
This is a pre-existing issue, but is dev->max_tx_urbs validated when received
from the device firmware?

drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c:kvaser_usb_leaf_get_software_info_leaf() {
    ...
    dev->max_tx_urbs = le16_to_cpu(softinfo->max_outstanding_tx);
    ...
}

If a malicious device reports a max_outstanding_tx of 0, could this cause a
divide-by-zero panic later when handling transmit acknowledgments?

drivers/net/can/usb/kvaser_usb/kvaser_usb_leaf.c:kvaser_usb_leaf_tx_acknowledge() {
    ...
    context = &priv->tx_contexts[tid % dev->max_tx_urbs];
    ...
}

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