Re: [PATCH 4/5] can: peak_usb: validate channel numbers in PCAN-USB FD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: peak_usb: missing NULL check on sibling channels leads to UAF and NULL deref
--

commit bb6a3841806ddddd6a5d411fd8594a762ea452bd
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

can: peak_usb: validate channel numbers in PCAN-USB FD

This patch adds validation for CAN channel numbers encoded in messages
received from PCAN-USB FD devices. The channel number is checked against
the number of controllers supported by the adapter to prevent out-of-bounds
array accesses.

> 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 82502594a4092..ddbb2be1de0dd 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
[ ... ]
> @@ -536,8 +537,12 @@ static int pcan_usb_fd_decode_canmsg(struct pcan_usb_fd_if *usb_if,
>  	struct sk_buff *skb;
>  	const u16 rx_msg_flags = le16_to_cpu(rm->flags);
>  
> -	if (pucan_msg_get_channel(rm) >= ARRAY_SIZE(usb_if->dev))
> -		return -ENOMEM;
> +	/* Reject invalid channel numbers reported by the firmware.
> +	 * Any channel below ctrl_count is guaranteed to have a valid
> +	 * entry in usb_if->dev[].
> +	 */
> +	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> +		return -EINVAL;
>  
>  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
>  	netdev = dev->netdev;

[Severity: High]
Does this actually guarantee that the usb_if->dev[] entry is valid?

Since sibling channels are initialized sequentially during device probe, can
this lead to a NULL pointer dereference? For example, if can0 is brought up
and receives a message from the shared USB endpoint with a channel number
for can1 before can1 completes its initialization, usb_if->dev[channel]
might still be NULL, causing a crash when accessing dev->netdev.

Additionally, what happens during device disconnect? If peak_usb_disconnect()
frees sibling channels in reverse order (e.g., can1 then can0), the freed
channel's pointer seems to remain in the shared usb_if->dev[] array. If can0
is still active and receives a message for the freed channel, could this
result in a use-after-free?

This same pattern appears to be repeated in pcan_usb_fd_decode_status(),
pcan_usb_fd_decode_error(), and pcan_usb_fd_decode_overrun() modified
further down in this patch.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-canfd_check_channel_idx-v1-0-0a3bb82f4e09@peak-system.fr?part=4