Re: [PATCH v4 06/10] 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 (2) · Medium: 0 · Low: 0 (1)

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] At the target commit, peak_usb_create_dev() calls
  register_candev() before dev_init() and dev_set_bus().

Pre-existing issues:
- [High] This is the same NULL-slot dereference this commit fixes for
  PCAN-USB FD, but in the sibling PCAN-USB Pro driver, which the commit…
- [High] Asynchronous restart URBs are not tracked, so one can outlive a
  channel freed on the probe-rollback path.
- [Low] On the probe-failure path the device is still attached.

--

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

This patch makes the PCAN-USB FD decoders check the record's channel
number against the adapter's controller count and reject records for
empty channel slots. It also clears a channel's slot in pcan_usb_fd_free().
It adds peak_usb_stop_rx(), which stops RX on the siblings already created
when a channel probe fails, and on all channels in 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..4c08656e77681 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

[ ... ]

> @@ -904,6 +907,22 @@ int pcan_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info
>  	return 0;
>  }
>  
> +/*
> + * stop RX on dev and on all its previous siblings: clearing CONNECTED makes
> + * peak_usb_ndo_open() fail, then poisoning the RX anchors waits for running
> + * completion handlers and makes any later submission on them fail.
> + */
> +static void peak_usb_stop_rx(struct peak_usb_device *dev)
> +{
> +	struct peak_usb_device *d;
> +
> +	for (d = dev; d; d = d->prev_siblings)
> +		clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);

[Severity: Low]
This is a pre-existing issue, but the adapter is still attached on the
probe failure path. Once CONNECTED is cleared here, pcan_usb_fd_send_cmd()
returns success without sending anything:

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

Later, peak_usb_disconnect()->unregister_candev()->peak_usb_ndo_stop() sends
the calibration-off command (through pcan_usb_fd_stop()) and the bus-off
command (through dev_set_bus(dev, 0)). Are both of these dropped silently?

If so, a channel 0 that was already opened stays bus-on. Meanwhile the
driver sets dev->can.state to CAN_STATE_STOPPED and decrements
dev_opened_count.

Before this patch, peak_usb_disconnect() cleared CONNECTED just before
unregister_candev() for each channel, so these commands were already being
dropped the same way.

> +
> +	for (d = dev; d; d = d->prev_siblings)
> +		usb_poison_anchored_urbs(&d->rx_submitted);
> +}
> +

[ ... ]

> @@ -1014,6 +1033,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);
> +
>  	if (dev->adapter->dev_free)
>  		dev->adapter->dev_free(dev);
>  

[Severity: High]
At this commit, peak_usb_create_dev() still calls register_candev() before
dev_init() and dev_set_bus():

	err = register_candev(netdev);
	...
	if (dev->adapter->dev_init) {
		err = dev->adapter->dev_init(dev);
	...
	if (dev->adapter->dev_set_bus) {
		err = dev->adapter->dev_set_bus(dev, 0);
		if (err)
			goto adap_dev_free;
	}

If dev_set_bus() fails, the netdev of the failing channel may already be
open, with its own RX URBs in flight. peak_usb_stop_rx(dev->prev_siblings)
stops only the earlier siblings, not dev.

For channel 0, dev_free() is pcan_usb_fd_free(). It frees pdev->usb_if and
pdev->cmd_buffer_addr, because no siblings exist yet. Then:

lbl_unregister_candev:
	unregister_candev(netdev);
	  peak_usb_ndo_stop()
	    peak_usb_unlink_all_urbs()
	    pcan_usb_fd_stop()
	      pdev->usb_if->dev_opened_count--;

Could this channel's RX completions, which are still running and decode
through pdev->usb_if, touch the freed usb_if? pcan_usb_fd_stop() above
would touch it too. Does this path need peak_usb_stop_rx(dev), so that the
failing channel itself is stopped as well?

The order of dev_free() before unregister_candev() predates this patch. A
later patch in this series, "can: peak_usb: register the netdev only once
the channel is set up", moves register_candev() after dev_init(),
dev_set_bus() and dev_get_can_channel_id(). It also drops
unregister_candev() from the error path, and its commit message describes
this use-after-free.

> @@ -1038,13 +1063,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: High]
This isn't a bug introduced by this patch, but this path is now being
hardened for the probe error case. Can an asynchronous restart URB outlive
the peak_usb_device freed here?

pcan_usb_fd_restart_async() submits the restart URB with dev as its
context. It does not anchor the URB or take a reference on dev.
pcan_usb_pro_restart_async() works the same way:

	usb_fill_bulk_urb(urb, dev->udev,
			  usb_sndbulkpipe(dev->udev, fw_info->cmd_out_ep),
			  buf, pc - buf,
			  pcan_usb_pro_restart_complete, dev);

	/* and submit it. */
	return usb_submit_urb(urb, GFP_ATOMIC);

On a two-channel adapter, can0 can be up while channel 1 is being probed,
with a restart pending from restart-ms or from a manual restart. If the
channel 1 probe then fails:

peak_usb_probe()
  peak_usb_disconnect()
    peak_usb_stop_rx()              poisons rx_submitted only
    unregister_candev()
      peak_usb_ndo_stop()
        close_candev()              cancels restart_work only
        peak_usb_unlink_all_urbs()  kills rx_submitted and tx_submitted
    free_candev()

The USB core has not disabled the interface on this path. When the restart
URB completes later:

pcan_usb_pro_restart_complete()
  peak_usb_restart_complete(urb->context)
    dev->can.state = CAN_STATE_ERROR_ACTIVE;
    netif_wake_queue(dev->netdev);

Would this write to the freed dev and its net_device?

[ ... ]

> 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: High]
This is a pre-existing issue, but the sibling PCAN-USB Pro driver has the
same NULL slot dereference, and this patch doesn't change it. In
pcan_usb_pro.c, pcan_usb_pro_handle_canmsg() only checks the table size:

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

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

pcan_usb_pro_handle_error() does the same thing. The channel 1 slot stays
NULL until pcan_usb_pro_init() runs for that channel:

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

By then, channel 0's RX URBs on the shared bulk-in endpoint may already be
running. pcan_usb_pro_free() never clears the slot either.

Can a channel 1 record that arrives in that window cause a NULL
dereference in URB completion context?

A later patch in this series, "can: peak_usb: reject records for unset
channels in PCAN-USB Pro", adds the READ_ONCE() and the NULL slot check to
both handlers. It also publishes the slot with smp_store_release() in
pcan_usb_pro_init() and clears it with WRITE_ONCE() in pcan_usb_pro_free().

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