Re: [PATCH v6 08/13] can: peak_usb: reject records for unset channels in PCAN-USB Pro

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (2) · Medium: 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).

- [Medium] In pcan_usb_pro_decode_buf(), a record for a channel whose
  usb_if->dev[] slot is NULL now makes…

Pre-existing issues:
- [High] Use-after-free in the probe error path of peak_usb_create_dev().
- [High] NULL pointer dereference when a channel is opened before
  dev_init().

--

can: peak_usb: reject records for unset channels in PCAN-USB Pro

This commit makes the PCAN-USB Pro record decoders reject a record when
the channel's usb_if->dev[] slot is still NULL. It also clears the slot in
pcan_usb_pro_free(). Slots are now published with smp_store_release() and
read with READ_ONCE().

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> index cbc23953ce610..151c5ea2859be 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_pro.c
> @@ -543,7 +543,12 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
>  	if (ctrl_idx >= ARRAY_SIZE(usb_if->dev))
>  		return -EINVAL;
>  
> -	dev = usb_if->dev[ctrl_idx];
> +	dev = READ_ONCE(usb_if->dev[ctrl_idx]);
> +
> +	/* reject records for a channel that is not (or no longer) set up */
> +	if (!dev)
> +		return -EINVAL;

[Severity: Medium]
Should an empty slot be treated as an error here? The commit message
describes this as an expected window: channel 0's RX is running before
channel 1 has been initialized.

A negative return makes pcan_usb_pro_decode_buf() jump to fail and stop
parsing the URB:

pcan_usb_pro_decode_buf() {
    ...
		case PCAN_USBPRO_RXRTR:
			err = pcan_usb_pro_handle_canmsg(usb_if, &pr->rx_msg);
			if (err < 0)
				goto fail;
    ...
fail:
	if (err)
		pcan_dump_mem("received msg",
			      urb->transfer_buffer, urb->actual_length);
}

Both channels share the same bulk-in endpoint. Wouldn't that drop every
record that follows the channel 1 record in the same transfer? That
includes channel 0 CAN frames, channel 0 status records and RXTS
calibration records.

The non-zero return also makes peak_usb_read_bulk_callback() dump the
buffer a second time:

			err = dev->adapter->dev_decode_buf(dev, urb);
			if (err)
				pcan_dump_mem("received usb message",
					      urb->transfer_buffer,
					      urb->transfer_buffer_length);

Neither dump is rate limited, and both run from URB completion context.

Would it work better to skip just this record and go on to the next one?
The same applies to the -EINVAL return added to
pcan_usb_pro_handle_error().

Two later commits in the series seem to resolve this.

"can: peak_usb: register the netdevs only once all channels are set up"
makes this branch unreachable while RX is running.

"can: peak_usb: dump undecodable URBs once and rate limit decode errors"
removes the dump in pcan_usb_pro_decode_buf() and rate limits the one in
the callback.

At this commit, though, the window is still there.

[ ... ]

> @@ -933,7 +943,8 @@ static int pcan_usb_pro_init(struct peak_usb_device *dev)
>  	}
>  
>  	pdev->usb_if = usb_if;
> -	usb_if->dev[dev->ctrl_idx] = dev;
> +	/* publish dev to the decoders */
> +	smp_store_release(&usb_if->dev[dev->ctrl_idx], dev);

[Severity: High]
This is a pre-existing issue, but pdev->usb_if is only assigned here,
after the netdev has already been registered. In peak_usb_create_dev():

	set_bit(PCAN_USB_STATE_CONNECTED, &dev->state);
	...
	err = register_candev(netdev);
	...
	if (dev->adapter->dev_init) {
		err = dev->adapter->dev_init(dev);

pcan_usb_pro_init() also issues several synchronous USB requests on
channel 0 before it gets to this assignment.

If canX is configured and opened in that window, can
peak_usb_ndo_open()->...->pcan_usb_pro_start() dereference a NULL usb_if?

	if (pdev->usb_if->dev_opened_count == 0) {

RX completions in the same window would also reach
pcan_usb_pro_decode_buf() with a NULL usb_if. The handlers do
READ_ONCE(usb_if->dev[ctrl_idx]) before the new check, so the added NULL
check on dev does not cover this case.

The later commit "can: peak_usb: register the netdevs only once all
channels are set up" seems to fix this. It registers the netdevs in
peak_usb_probe() only after every channel's dev_init() and dev_set_bus()
have succeeded, and its commit message names this dereference.

> @@ -981,6 +992,9 @@ static void pcan_usb_pro_exit(struct peak_usb_device *dev)
>   */
>  static void pcan_usb_pro_free(struct peak_usb_device *dev)
>  {
> +	/* clear this channel's slot: dev is going to be freed */
> +	WRITE_ONCE(pcan_usb_pro_dev_if(dev)->dev[dev->ctrl_idx], NULL);
> +
>  	/* last device: can free pcan_usb_pro_interface object now */
>  	if (!dev->prev_siblings && !dev->next_siblings)
>  		kfree(pcan_usb_pro_dev_if(dev));

[Severity: High]
This isn't a bug introduced by this patch, but can this kfree() cause a
use-after-free on the probe error path in peak_usb_create_dev()?

adap_dev_free:
	peak_usb_stop_rx(dev->prev_siblings);

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

lbl_unregister_candev:
	unregister_candev(netdev);

For channel 0, dev->prev_siblings is NULL, so no RX gets stopped. Then
pcan_usb_pro_free() frees the shared pcan_usb_pro_interface before the
netdev is unregistered.

Suppose can0 was opened between register_candev() and a failing
dev_set_bus(dev, 0). unregister_candev()->...->pcan_usb_pro_stop() would
then read and decrement dev_opened_count in freed memory:

	if (pdev->usb_if->dev_opened_count == 1)
		pcan_usb_pro_set_ts(dev, 0);

	pdev->usb_if->dev_opened_count--;

Channel 0's own RX URBs were never poisoned, so they could also complete
into pcan_usb_pro_decode_buf() with the freed usb_if.

The WRITE_ONCE() added here runs before the kfree(), so it is fine by
itself. The later commit "can: peak_usb: register the netdevs only once
all channels are set up" removes register_candev() from
peak_usb_create_dev(), and its commit message names this use-after-free.

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