Re: [PATCH] can: peak_usb: validate uCAN receive record lengths

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The decoders blindly trust the device-provided channel index to retrieve the device pointer, leading to a NULL pointer dereference if the channel is within array bounds but uninitialized.
--

commit 35cea41238e6f7cd42684d301dadde24023e663a
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

can: peak_usb: validate uCAN receive record lengths

pcan_usb_fd_decode_buf() walks uCAN records packed in one USB
receive buffer. Require each record to contain the fixed header for its
type, and verify CAN payload bytes before copying them into the skb.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> index eb4f5884ad735..45ae4611ae4c3 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> @@ -566,6 +566,13 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
>  				     dev->can.ctrlmode);
>  	}
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference just above this hunk in pcan_usb_fd_decode_canmsg().

The function currently checks the channel index against the array size:

	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
		return -ENOMEM;

	dev = usb_if->dev[pucan_msg_get_channel(rm)];
	netdev = dev->netdev;

Does this bounds check properly validate against the number of initialized
channels? If the adapter only has 1 channel (like the PCAN-USB FD), channel 0
is initialized but channel 1 is NULL. Since ARRAY_SIZE(usb_if->dev) is 2, a
malicious USB device could send a message with a channel index of 1, which
passes the bounds check but results in a crash when dev->netdev is accessed.

It appears this same pattern exists in pcan_usb_fd_decode_status(),
pcan_usb_fd_decode_error(), and pcan_usb_fd_decode_overrun() in this file.
Should we verify if dev is NULL before dereferencing it in these decoders?

> +	if (!(rx_msg_flags & PUCAN_MSG_RTR) &&
> +	    le16_to_cpu(rx_msg->size) - offsetof(struct pucan_rx_msg, d) <
> +	    cfd->len) {
> +		kfree_skb(skb);
> +		return -EBADMSG;
> +	}
> +
>  	cfd->can_id = le32_to_cpu(rm->can_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706092836.79754-1-pengpeng@iscas.ac.cn?part=1