Re: [PATCH v6 06/13] can: peak_usb: harden channel handling in PCAN-USB FD

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 (2) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The probe-error cleanup in peak_usb_create_dev() (adap_dev_free
  label) calls peak_usb_stop_rx(dev->prev_siblings).

Pre-existing issues:
- [Medium] PCAN-USB Pro has the same NULL channel-slot bug that this
  patch fixes for PCAN-USB FD.
- [Medium] When peak_usb_disconnect() runs from the probe-error path,
  peak_usb_stop_rx() clears PCAN_USB_STATE_CONNECTED on all siblings…

--

can: peak_usb: harden channel handling in PCAN-USB FD

The PCAN-USB FD decoders now check the channel number against the
adapter's controller count, and they drop records for a channel whose
usb_if->dev[] slot is NULL. The slot is cleared in pcan_usb_fd_free().
A new helper, peak_usb_stop_rx(), clears CONNECTED under RTNL and poisons
the RX anchors. It is called from the probe error path and from
peak_usb_disconnect().

> 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 a1b17e1e6fea6..49d2c01b8ef7e 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

[ ... ]

> @@ -1014,6 +1041,12 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	return 0;
>  
>  adap_dev_free:
> +	/* the whole interface is released when probing a channel fails:
> +	 * stop RX on the channels already created, since their RX URBs may
> +	 * carry records for this one.
> +	 */
> +	peak_usb_stop_rx(dev->prev_siblings);

[Severity: High]
Should this also stop RX on dev, and not only on its previous siblings?
The comment above peak_usb_stop_rx() says it stops "dev and on all its
previous siblings", but this call skips the channel that is failing.

At this commit, peak_usb_create_dev() calls register_candev() before
dev_init() and dev_set_bus(). So userspace may already have opened the
failing channel, and its RX URBs may be in flight, when
dev_set_bus(dev, 0) fails.

On a single-channel PCAN-USB FD, dev->prev_siblings is NULL, so nothing
gets stopped. The error path then runs:

peak_usb_create_dev()
    dev->adapter->dev_free(dev)
        pcan_usb_fd_free()
            kfree(pdev->cmd_buffer_addr);
            kfree(pdev->usb_if);
    unregister_candev(netdev)
        peak_usb_ndo_stop()
            dev->adapter->dev_stop(dev)
                pcan_usb_fd_stop()
                    pdev->usb_if->dev_opened_count--;

Can the RX completions of this channel, and then pcan_usb_fd_stop(), use
usb_if and the command buffer after they have been freed here?

The later patch in this series, "can: peak_usb: register the netdevs only
once all channels are set up", seems to close this window. It moves
register_candev() into peak_usb_probe(), after all channels have been
created. At this commit, though, the window is still open.

>  	if (dev->adapter->dev_free)
>  		dev->adapter->dev_free(dev);
>  

[ ... ]

> @@ -1038,13 +1071,14 @@ 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 */
> +	/* stop RX on all channels before unregistering and freeing them */
> +	peak_usb_stop_rx(usb_get_intfdata(intf));
> +
>  	for (dev = usb_get_intfdata(intf); dev; dev = dev_prev_siblings) {
>  		struct net_device *netdev = dev->netdev;
>  		char name[IFNAMSIZ];
>  
>  		dev_prev_siblings = dev->prev_siblings;
> -		clear_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
>  		strscpy(name, netdev->name, IFNAMSIZ);
>  
>  		unregister_candev(netdev);

[Severity: Medium]
This is a pre-existing issue, but when peak_usb_disconnect() is called
from the probe error path, the device is still attached and its interface
has not been disabled.

peak_usb_stop_rx() clears PCAN_USB_STATE_CONNECTED on every channel before
any unregister_candev() runs. For a channel that is already up,
unregister_candev() then calls:

peak_usb_ndo_stop()
    dev->adapter->dev_stop(dev)
    dev->adapter->dev_set_bus(dev, 0)
        pcan_usb_fd_send_cmd()

and pcan_usb_fd_send_cmd() starts with:

	/* usb device unregistered? */
	if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
		return 0;

After a failed probe, is the CAN controller left bus-on, while
peak_usb_ndo_stop() treats the bus-off command as successful?

The old loop already cleared CONNECTED just before each
unregister_candev(), so these commands were dropped before this patch
too. The patch now clears it on all channels at once.

> 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 15d275d6a1277..eee5b5eaa247d 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,16 @@ 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 = READ_ONCE(usb_if->dev[pucan_msg_get_channel(rm)]);
> +
> +	/* reject records for a channel that is not (or no longer) set up */
> +	if (!dev)
> +		return -EINVAL;
>  
> -	dev = usb_if->dev[pucan_msg_get_channel(rm)];
>  	netdev = dev->netdev;

[Severity: Medium]
This isn't a bug introduced by this patch, but the PCAN-USB Pro decoders
look like they have the same problem that this patch fixes for PCAN-USB
FD.

In pcan_usb_pro.c, pcan_usb_pro_handle_canmsg() and
pcan_usb_pro_handle_error() check ctrl_idx only against the table size,
then dereference the slot:

	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
		return -EINVAL;

	dev = usb_if->dev[ctrl_idx];
	netdev = dev->netdev;

The channels share the bulk-in endpoint, so channel 0's RX URBs could
carry a channel 1 record before pcan_usb_pro_init() has filled
usb_if->dev[1]. Can that lead to a NULL pointer dereference in URB
completion context? At this commit, register_candev() runs before
dev_init() in peak_usb_create_dev(), so channel 0 can already be open at
that point.

Unlike the new pcan_usb_fd_free(), pcan_usb_pro_free() also leaves
usb_if->dev[dev->ctrl_idx] set when the channel is freed.

The commit message does not mention PCAN-USB Pro. Two later patches in
this series appear to fix this: "can: peak_usb: reject records for unset
channels in PCAN-USB Pro" and "can: peak_usb: register the netdevs only
once all channels are set up".

>  
>  	if (rx_msg_flags & PUCAN_MSG_EXT_DATA_LEN) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022-canfd_check_channel_idx-v6-0-197704231551%40peak-system.fr