Re: [PATCH v3 09/10] can: peak_usb: unregister the netdev before dev_free() on probe failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] can: peak_usb: premature register_candev() in peak_usb_create_dev()
- [High] can: peak_usb: race in peak_usb_create_dev() due to early register_candev()
--

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

can: peak_usb: unregister the netdev before dev_free() on probe failure

This commit moves unregister_candev() before dev->adapter->dev_free() in the
adap_dev_free error path of peak_usb_create_dev() so that closing an already
opened netdev does not access resources freed by dev_free().

> 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..981168fead927 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -1039,8 +1039,13 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	 */
>  	peak_usb_stop_rx(dev->prev_siblings);
>  
> +	/* as in peak_usb_disconnect(), unregister the netdev before calling
> +	 * dev_free(): closing it may still use what dev_free() releases.
> +	 */
> +	unregister_candev(netdev);
>  	if (dev->adapter->dev_free)
>  		dev->adapter->dev_free(dev);
> +	goto lbl_restore_intf_data;
>  
>  lbl_unregister_candev:
>  	unregister_candev(netdev);

[Severity: High]
Can a similar use-after-free still occur on the lbl_unregister_candev error
path when dev->adapter->dev_init(dev) fails?

In peak_usb_create_dev(), register_candev(netdev) is called with
PCAN_USB_STATE_CONNECTED already set before dev->adapter->dev_init(dev):

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_create_dev() {
	...
	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->adapter->dev_init) {
		err = dev->adapter->dev_init(dev);
		if (err)
			goto lbl_unregister_candev;
	}
}

In pcan_usb_fd_init(), channel 0 allocates pdev->usb_if and
pdev->cmd_buffer_addr and then performs sleeping USB transfers in
pcan_usb_fd_read_fwinfo() and pcan_usb_fd_drv_loaded(). If the netdev is
opened during this window and pcan_usb_fd_drv_loaded() (or
pcan_usb_fd_read_fwinfo()) fails, pcan_usb_fd_init() frees
pdev->cmd_buffer_addr and pdev->usb_if before returning:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_init() {
	...
err_out_2:
	kfree(pdev->cmd_buffer_addr);
err_out_1:
	kfree(pdev->usb_if);
err_out:
	return err;
}

peak_usb_create_dev() then jumps to lbl_unregister_candev and calls
unregister_candev(netdev), which invokes peak_usb_ndo_stop() ->
pcan_usb_fd_stop() and pcan_usb_fd_set_bus(dev, 0), accessing the already
freed pdev->usb_if and pdev->cmd_buffer_addr.

[Severity: High]
Because register_candev(netdev) exposes the device before dev_init(),
dev_set_bus(dev, 0), and dev_get_can_channel_id() have completed, what
prevents userspace from opening or configuring the netdev while those
callbacks are still running?

In pcan_usb_pro_init(), usb_if is kept in a local variable across three
sleeping USB control requests before pdev->usb_if is assigned:

drivers/net/can/usb/peak_usb/pcan_usb_pro.c:pcan_usb_pro_init() {
	...
	pdev->usb_if = usb_if;
	/* publish dev to the decoders */
	smp_store_release(&usb_if->dev[dev->ctrl_idx], dev);
}

If peak_usb_ndo_open() -> peak_usb_start() -> pcan_usb_pro_start() runs
before pdev->usb_if is set, it dereferences a NULL pdev->usb_if (and
similarly for pdev->cmd_buffer_addr and pdev->usb_if in pcan_usb_fd_start()
or pcan_usb_fd_set_bittiming_slow() before pcan_usb_fd_init() populates
them).

Even after those pointers are assigned, peak_usb_create_dev() calls
dev_init(), dev_set_bus(dev, 0), and dev_get_can_channel_id() outside
rtnl_lock(). Could a concurrent ndo_open or bittiming callback race with
probe on the shared command buffer (pdev->cmd_buffer_addr or dev->cmd_buf),
or could dev_set_bus(dev, 0) in peak_usb_create_dev() switch the controller
back off after peak_usb_ndo_open() has already started it?

Would registering the candev only after hardware initialization and
dev_set_bus(dev, 0) have succeeded avoid all of these races?

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