Re: [PATCH v4 09/10] can: peak_usb: register the netdev only once the channel is set up

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] can: peak_usb: race on shared cmd_buffer_addr and fw_info in peak_usb_create_dev()
- [Medium] can: peak_usb: pcan_usb_fd_init() copies mutable ctrlmode from registered sibling
--

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

can: peak_usb: register the netdev only once the channel is set up

This commit moves register_candev() to the end of peak_usb_create_dev() so
that a CAN channel's netdev cannot be opened or configured by userspace
before dev_init(), dev_set_bus(), and dev_get_can_channel_id() have
completed.

> 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 4c08656e77681..ccfc26d3e5a6b 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -998,15 +998,6 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  	netdev->dev_id = ctrl_idx;
>  
> -	err = register_candev(netdev);
> -	if (err) {
> -		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
> -		goto lbl_restore_intf_data;
> -	}
> -
> -	if (dev->prev_siblings)
> -		(dev->prev_siblings)->next_siblings = dev;
> -
>  	/* keep hw revision into the netdevice */
>  	tmp16 = le16_to_cpu(usb_dev->descriptor.bcdDevice);
>  	dev->device_rev = tmp16 >> 8;
> @@ -1014,7 +1005,7 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	if (dev->adapter->dev_init) {
>  		err = dev->adapter->dev_init(dev);
>  		if (err)
> -			goto lbl_unregister_candev;
> +			goto lbl_restore_intf_data;
>  	}
>  
>  	/* set bus off */
> @@ -1027,6 +1018,18 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	/* get CAN channel id early */
>  	dev->adapter->dev_get_can_channel_id(dev, &dev->can_channel_id);
>  
> +	/* register the netdev only once the channel is fully set up, so that
> +	 * it can't be opened or configured while being initialized.
> +	 */
> +	err = register_candev(netdev);
> +	if (err) {
> +		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
> +		goto adap_dev_free;
> +	}

[Severity: Medium]
On multi-channel PCAN-USB FD adapters (such as PCAN-USB Pro FD and
PCAN-USB X6), can registering channel 0 here still leave a race on the
shared command buffer and fw_info while subsequent channels are being
initialized?

In peak_usb_probe(), peak_usb_create_dev() is called in a loop over
ctrl_count without holding rtnl_lock():

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_probe() {
	for (i = 0; i < peak_usb_adapter->ctrl_count; i++) {
		err = peak_usb_create_dev(peak_usb_adapter, intf, i);
		...
	}
}

Once iteration 0 calls register_candev(), channel 0 is visible to
userspace (for example udev or systemd-networkd) and its netdev or
ethtool callbacks can run under rtnl_lock() while peak_usb_create_dev()
is initializing channel 1 outside of rtnl_lock().

During channel 1 setup, pcan_usb_fd_init() attaches to channel 0's
usb_if and cmd_buffer_addr:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_init() {
	...
	} else {
		/* otherwise, simply copy previous sibling's values */
		struct pcan_usb_fd_device *ppdev =
			container_of(dev->prev_siblings,
				     struct pcan_usb_fd_device, dev);

		pdev->usb_if = ppdev->usb_if;
		pdev->cmd_buffer_addr = ppdev->cmd_buffer_addr;
	...
	pcan_usb_fd_set_clock_domain(dev, i);

	/* set LED in default state (end of init phase) */
	pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_DEF);
}

Channel 1 then uses that shared cmd_buffer_addr in
pcan_usb_fd_set_clock_domain(), pcan_usb_fd_set_can_led(), and
pcan_usb_fd_set_bus(), which can concurrently overwrite commands built by
channel 0's open, bittiming, or ethtool callbacks.

In addition, peak_usb_create_dev() for channel 1 calls
pcan_usb_fd_get_can_channel_id():

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_get_can_channel_id() {
	int err;
	struct pcan_usb_fd_if *usb_if = pcan_usb_fd_dev_if(dev);

	err = pcan_usb_fd_read_fwinfo(dev, &usb_if->fw_info);
	if (err)
		return err;
	...
}

pcan_usb_fd_read_fwinfo() calls pcan_usb_pro_send_req(), which zeroes the
shared usb_if->fw_info buffer with memset() before usb_control_msg(),
transiently clearing fw_info.cmd_out_ep to 0 while channel 0 may be
sending commands via pcan_usb_fd_send_cmd() or
pcan_usb_fd_restart_async().


[Severity: Medium]
Also on multi-channel PCAN-USB FD adapters, could channel 1 end up
copying runtime-modified ctrlmode flags from channel 0 during probe?

In pcan_usb_fd_init(), secondary channels copy dev.can.ctrlmode from
their previous sibling:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_init() {
	...
	} else {
		...
		/* do a copy of the ctrlmode[_supported] too */
		dev->can.ctrlmode = ppdev->dev.can.ctrlmode;
		dev->can.ctrlmode_supported = ppdev->dev.can.ctrlmode_supported;
	}
}

Because channel 0 is now registered via register_candev() before
pcan_usb_fd_init() runs for channel 1, userspace can configure channel 0
via can_changelink() -> can_ctrlmode_changelink() and set flags such as
CAN_CTRLMODE_FD, CAN_CTRLMODE_LISTENONLY, or CAN_CTRLMODE_ONE_SHOT in
ppdev->dev.can.ctrlmode before channel 1 copies it. Channel 1 would then
inherit those runtime flags (for example having CAN_CTRLMODE_FD set in
dev->can.ctrlmode while its netdev->mtu is still CAN_MTU) before being
registered.

[ ... ]

>  	netdev_info(netdev, "attached to %s channel %u (device 0x%08X)\n",
>  		    peak_usb_adapter->name, ctrl_idx, dev->can_channel_id);

[ ... ]

>
endasi

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-canfd_check_channel_idx-v4-0-bfbde81f8bab@peak-system.fr?part=9