Re: [PATCH] can: peak_usb: pcan_usb: cancel restart timer on stop to avoid use-after-free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] can: peak_usb: in-flight restart URB arms timer after
  pcan_usb_stop()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] can: peak_usb: incomplete restart_timer cleanup in
  peak_usb_create_dev() [1]
--

commit 711582c698eedc1cd7434111f52222eb7dcd4279
Author: Hyunjung Ko <hj351016@gmail.com>

can: peak_usb: pcan_usb: cancel restart timer on stop to avoid use-after-free

This commit adds a dev_stop callback to pcan_usb that calls
timer_delete_sync() on pdev->restart_timer when the interface is stopped.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 8fd058c328564..4c131958b5aa6 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -882,6 +882,14 @@ static int pcan_usb_start(struct peak_usb_device *dev)
>  	return pcan_usb_set_ext_vcc(dev, 0);
>  }
>  
> +static int pcan_usb_stop(struct peak_usb_device *dev)
> +{
> +	struct pcan_usb *pdev = container_of(dev, struct pcan_usb, dev);
> +
> +	timer_delete_sync(&pdev->restart_timer);
> +	return 0;
> +}

[Severity: Medium]
Can an in-flight restart URB complete after pcan_usb_stop() and re-arm
pdev->restart_timer?

pcan_usb_restart_async() submits its bulk URB without anchoring it to
dev->tx_submitted, so peak_usb_unlink_all_urbs() does not kill it before
calling dev_stop().  In addition, pcan_usb_restart_pending() unconditionally
calls mod_timer(&pdev->restart_timer, ...) without checking urb->status or
PCAN_USB_STATE_STARTED:

CPU0                              CPU1
can_restart()
  ...
    pcan_usb_restart_async()
      usb_submit_urb(urb, ...)
peak_usb_ndo_stop()
  peak_usb_unlink_all_urbs(dev)
  pcan_usb_stop(dev)
    timer_delete_sync(&pdev->restart_timer)
                                  pcan_usb_restart_pending(urb)
                                    mod_timer(&pdev->restart_timer, ...)
  ...
  free_candev(netdev)
                                  pcan_usb_restart()
                                    peak_usb_restart_complete(&pdev->dev)
                                      <-- use-after-free

If the interface remains registered, pcan_usb_restart() marks a stopped
interface as CAN_STATE_ERROR_ACTIVE and wakes its queue; if the USB device
disconnects after peak_usb_ndo_stop() has cleared IFF_UP, unregister_candev()
does not call peak_usb_ndo_stop() again and free_candev() frees pdev while
restart_timer is still armed.

Anchoring the restart URB so peak_usb_unlink_all_urbs() kills it before
pcan_usb_stop(), or only arming restart_timer in pcan_usb_restart_pending()
when urb->status == 0 and PCAN_USB_STATE_STARTED is set, would close this
race.

> +
>  static int pcan_usb_init(struct peak_usb_device *dev)
>  {
>  	struct pcan_usb *pdev = container_of(dev, struct pcan_usb, dev);

[Severity: Critical]
This problem wasn't introduced by this patch, but can a failed probe in
peak_usb_create_dev() free netdev while pdev->restart_timer is still armed
[1]?

peak_usb_create_dev() calls register_candev(netdev) before calling
dev->adapter->dev_init() (pcan_usb_init()) and dev->adapter->dev_set_bus():

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_create_dev() {
	...
	err = register_candev(netdev);
	if (err) {
		...
		goto lbl_restore_intf_data;
	}
	...
	if (dev->adapter->dev_init) {
		err = dev->adapter->dev_init(dev);
		if (err)
			goto lbl_unregister_candev;
	}

	/* set bus off */
	if (dev->adapter->dev_set_bus) {
		err = dev->adapter->dev_set_bus(dev, 0);
		if (err)
			goto adap_dev_free;
	}
	...
adap_dev_free:
	if (dev->adapter->dev_free)
		dev->adapter->dev_free(dev);

lbl_unregister_candev:
	unregister_candev(netdev);
	...
lbl_free_candev:
	free_candev(netdev);
}

Because register_candev() publishes the netdev and drops rtnl_lock before
pcan_usb_init() or dev_set_bus() blocks on USB bulk transfers, another
context can open the device and trigger a bus-off restart via
pcan_usb_restart_async().  If pcan_usb_get_serial() fails in pcan_usb_init(),
or if an in-flight restart URB completes after unregister_candev(),
free_candev() frees pdev while restart_timer is pending, causing a
use-after-free when pcan_usb_restart() fires.

[1] https://sashiko.dev/#/bug/linux-c6d07ad0-7ac4-41cf-a567-927ba96a5e93

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107153319.3765325-1-hj351016@gmail.com?part=1