Re: [PATCH net 10/20] can: peak_usb: add bounds check for USB channel index

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated CAN payload length from the USB packet causes a heap buffer overflow when copying data into the SKB and reading from the URB buffer.
- [High] Inadequate bounds checking allows a NULL pointer dereference of uninitialized channel device pointers.
- [High] Race condition during probe exposes the network interface before driver initialization is complete, leading to a NULL pointer dereference.
--

--- Patch [10]: [PATCH net 10/20] can: peak_usb: add bounds check for USB channel index ---
commit bda9ca94a61ede6bd59945cb6b8e4a5d2a8b5017
Author: James Gao <jamesgao5@outlook.com>

can: peak_usb: add bounds check for USB channel index

The channel control index ctrl_idx is derived from rx->len which comes
directly from a device USB payload. The mask 0x0f allows values 0-15, but
the array size of usb_if->dev[] is only 2. Values 2-15 cause heap
out-of-bounds read, eventually causing kernel panic in the IRQ context.

Link: https://patch.msgid.link/TYWPR01MB8559DBAAAA6A7F410400329CF0012@TYWPR01MB8559.jpnprd01.prod.outlook.com

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index aefcded8e12a8..b6be8c19e537f 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -534,12 +534,18 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
>  				      struct pcan_usb_pro_rxmsg *rx)
>  {
>  	const unsigned int ctrl_idx = (rx->len >> 4) & 0x0f;
> -	struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
> -	struct net_device *netdev = dev->netdev;
> +	struct peak_usb_device *dev;
> +	struct net_device *netdev;
>  	struct can_frame *can_frame;
>  	struct sk_buff *skb;
>  	struct skb_shared_hwtstamps *hwts;
>  
> +	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
> +		return -EINVAL;
> +
> +	dev = usb_if->dev[ctrl_idx];
> +	netdev = dev->netdev;

[Severity: High]
This isn't a bug introduced by this patch, but does checking against
ARRAY_SIZE() fully protect against NULL pointer dereferences here?

If a single-channel adapter is used, usb_if->dev[1] might be left
uninitialized (NULL). If a device sends URBs for channel 1, wouldn't it
bypass the ARRAY_SIZE() check and cause a kernel panic when dereferencing
dev->netdev?

There appears to be an identical inadequate bounds check in the FD variant
of the driver at pcan_usb_fd_decode_canmsg().

[Severity: High]
This is a pre-existing issue, but does this code overflow can_frame->data[]
when copying the payload?

Looking further down in pcan_usb_pro_handle_canmsg():

    can_frame->len = rx->len & 0x0f;
    ...
    } else {
        memcpy(can_frame->data, rx->data, can_frame->len);
    }

Since rx->len & 0x0f allows values up to 15, and can_frame->data[] is only an
8-byte array, could a malicious device send an oversized payload length and
overflow can_frame->data[]?

> +
>  	skb = alloc_can_skb(netdev, &can_frame);
>  	if (!skb)
>  		return -ENOMEM;

[ ... ]

> @@ -573,14 +579,20 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,
>  {
>  	const u16 raw_status = le16_to_cpu(er->status);
>  	const unsigned int ctrl_idx = (er->channel >> 4) & 0x0f;
> -	struct peak_usb_device *dev = usb_if->dev[ctrl_idx];
> -	struct net_device *netdev = dev->netdev;
> +	struct peak_usb_device *dev;
> +	struct net_device *netdev;
>  	struct can_frame *can_frame;
>  	enum can_state new_state = CAN_STATE_ERROR_ACTIVE;
>  	u8 err_mask = 0;
>  	struct sk_buff *skb;
>  	struct skb_shared_hwtstamps *hwts;
>  
> +	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
> +		return -EINVAL;
> +
> +	dev = usb_if->dev[ctrl_idx];
> +	netdev = dev->netdev;
> +
>  	/* nothing should be sent while in BUS_OFF state */
>  	if (dev->can.state == CAN_STATE_BUS_OFF)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but is there a race condition during probe
that could expose the network interface before driver initialization is
complete?

In peak_usb_create_dev(), register_candev() is called before the interface
is fully initialized:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_create_dev() {
    ...
    err = register_candev(netdev);
    ...
}

If userspace immediately brings the interface up (e.g., via udev),
pcan_usb_pro_start() is called and dereferences pdev->usb_if:

drivers/net/can/usb/peak_usb/pcan_usb_pro.c:pcan_usb_pro_start() {
    ...
    if (pdev->usb_if->dev_opened_count == 0) {
    ...
}

Since pdev->usb_if is only allocated and assigned later in
pcan_usb_pro_init(), wouldn't this cause a NULL pointer dereference?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=10