Re: [PATCH v2 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 3 potential issue(s) to consider:
- [High] can: peak_usb: TOCTOU race in peak_usb_disconnect() allows UAF of sibling devices
- [High] can: peak_usb: UAF in RX path due to dangling sibling pointer on probe failure
- [Medium] net/can: undocumented structural fix in disconnect path
--

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

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

The PCAN-USB FD family encodes the CAN channel number in messages
received from the device. This value is used as an index into the
adapter CAN device table. This commit validates the channel number
against the number of CAN controllers supported by the adapter before
performing the lookup.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index 55aad01cd8cac..751cd52cb5486 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -1038,7 +1038,17 @@ static void peak_usb_disconnect(struct usb_interface *intf)
>  	struct peak_usb_device *dev;
>  	struct peak_usb_device *dev_prev_siblings;
>  
> -	/* unregister as many netdev devices as siblings */
> +	/* First, kill all pending RX URBs. usb_kill_anchored_urbs() waits
> +	 * until all completion handlers have completed, ensuring that no
> +	 * decode_buf() callback can access usb_if->dev[] after this point.
> +	 */
> +	for (dev = usb_get_intfdata(intf); dev; dev = dev->prev_siblings)
> +		usb_kill_anchored_urbs(&dev->rx_submitted);

[Severity: Medium]
The commit message states the patch only adds channel number validation to the
PCAN-USB FD receive path. However, this diff introduces a concurrency fix in
peak_usb_disconnect() that kills all pending RX URBs across all channels before
unregistering devices.

Should this structural fix be documented in the commit message or split into a
separate patch?

[Severity: High]
This added loop drains RX URBs to prevent UAFs, but runs without holding
rtnl_lock or updating state that would prevent new URB submissions.

Could a concurrent `ifconfig up` call peak_usb_start(), allocating and
submitting new RX URBs that bypass this kill loop?

> +
> +	/* All RX URBs have been drained before reaching this point. No
> +	 * decode_buf() callback can access usb_if->dev[] anymore, making it
> +	 * safe to unregister and free the associated netdevs.
> +	 */
>  	for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
>  		struct net_device *netdev = dev->netdev;
>  		char name[IFNAMSIZ];

[ ... ]

> 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..71ac6fdcd99be 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_fd.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_fd.c

[ ... ]

> @@ -536,10 +537,19 @@ 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 */
> +	if (pucan_msg_get_channel(rm) >= usb_if->adapter->ctrl_count)
> +		return -EINVAL;
>  
>  	dev = usb_if->dev[pucan_msg_get_channel(rm)];
> +
> +	/* This should never happen during normal operation. However, do not
> +	 * trust the device and reject records targeting a valid channel
> +	 * without an associated netdev.
> +	 */
> +	if (!dev)
> +		return -EINVAL;

[Severity: High]
If new RX URBs bypass the kill loop in peak_usb_disconnect() as mentioned
above, they could remain active while sibling devices are sequentially freed.

If one of those URBs receives a packet targeting an already-freed sibling,
does it result in a use-after-free here, since usb_if->dev[] is never cleared?

Also, could this check fail to protect against a dangling pointer left by a
probe failure?

For example, pcan_usb_fd_init() populates the shared array with the device
pointer. If peak_usb_create_dev() then fails for a sibling device, its error
path frees the device but does not clear the usb_if->dev[] entry. Since this
check assumes unassociated slots are NULL, could an RX packet targeting the
failed channel result in a use-after-free?

> +
>  	netdev = dev->netdev;
>  
>  	if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-canfd_check_channel_idx-v2-0-f7b772929791@peak-system.fr?part=4