Re: [PATCH v3 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 · Medium: 0 (1) · 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] At this commit, the adap_dev_free error path in
  peak_usb_create_dev() (drivers/net/can/usb/peak_usb/pcan_usb_core.c)…

Pre-existing issues:
- [Medium] When peak_usb_disconnect() runs while the adapter is still
  powered (probe failure or sysfs unbind), PCAN_USB_STATE_CONNECTED is…

--

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

This commit checks the channel number in records from PCAN-USB FD devices
against the adapter's controller count. Records for an empty channel slot
are now rejected. It also clears the channel slot in pcan_usb_fd_free(),
stops RX on the sibling channels when a probe fails, and makes
peak_usb_disconnect() stop RX on all channels before it unregisters any
of them.

> 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);
> +
> +	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]
Does this error path leave the failing channel running on memory that
dev_free() has just released?

peak_usb_stop_rx() only walks dev->prev_siblings. So dev keeps
PCAN_USB_STATE_CONNECTED set, and its rx_submitted URBs are not poisoned.
On a single-channel adapter, or when channel 0 fails, prev_siblings is
NULL and nothing is stopped.

register_candev() runs before dev_init() and dev_set_bus(), so the
netdev may already be open when dev_set_bus(dev, 0) fails. In that case
pcan_usb_fd_free() finds no siblings and frees the shared objects:

pcan_usb_fd_free() {
    ...
	if (!dev->prev_siblings && !dev->next_siblings) {
		kfree(pdev->cmd_buffer_addr);
		kfree(pdev->usb_if);
	}
}

Only after that does the code fall through to:

lbl_unregister_candev:
	unregister_candev(netdev);

In the meantime, dev's RX completions can keep running
pcan_usb_fd_decode_buf() on the freed usb_if. The decoders read
usb_if->adapter and usb_if->dev[], and the timestamp decoding writes
cm_ignore_count and time_ref.

After that, unregister_candev()->peak_usb_ndo_stop()->pcan_usb_fd_stop()
does:

	if (pdev->usb_if->dev_opened_count == 1)
		pcan_usb_fd_set_options(dev, 0,
					PUCAN_OPTION_ERROR,
					PCAN_UFD_FLTEXT_CALIBRATION);
	pdev->usb_if->dev_opened_count--;

This reads and writes the freed usb_if. pcan_usb_fd_set_options() and
pcan_usb_fd_set_bus() also build commands in the freed cmd_buffer_addr.

The commit message says RX is stopped before the channel is freed, but
here that only covers the siblings.

A later patch in this series, "can: peak_usb: unregister the netdev
before dev_free() on probe failure", moves unregister_candev(netdev) in
front of dev_free(dev). With that change, ndo_stop kills
dev->rx_submitted while usb_if is still allocated. Could that ordering be
folded into this patch, so this commit is not left with the
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: Medium]
This is a pre-existing issue, and this patch did not introduce it.
peak_usb_disconnect() can run while the adapter is still powered, from a
probe failure or a sysfs unbind. CONNECTED is cleared before
unregister_candev(). Are the bus-off commands sent from ndo_stop dropped
in that case?

pcan_usb_fd_send_cmd() returns success without sending anything:

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

So two commands never reach the device: PUCAN_CMD_RESET_MODE from
peak_usb_ndo_stop()->pcan_usb_fd_set_bus(dev, 0), and the options-off
command from pcan_usb_fd_stop(). The CAN controller could then stay in
operational mode with no driver bound.

The old code already cleared CONNECTED just before each channel's
unregister_candev(), so the behavior is the same. This patch only clears
the bit on all channels up front.

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