Re: [PATCH v2] can: peak_usb: pcan_usb: shut down restart timer on free 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: unanchored restart URB and timer mishandled in
  pcan_usb_free()
--

commit 5d0ce26d62347660cb47c1b9c3037c160879b961
Author: Hyunjung Ko <hj351016@gmail.com>

can: peak_usb: pcan_usb: shut down restart timer on free to avoid use-after-free

This commit adds a dev_free callback to pcan_usb that calls
timer_shutdown_sync() on pdev->restart_timer before freeing the device.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 8fd058c328564..113b2e7453310 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -882,6 +882,19 @@ static int pcan_usb_start(struct peak_usb_device *dev)
>  	return pcan_usb_set_ext_vcc(dev, 0);
>  }
>  
> +static void pcan_usb_free(struct peak_usb_device *dev)
> +{
> +	struct pcan_usb *pdev = container_of(dev, struct pcan_usb, dev);
> +
> +	/*
> +	 * The restart timer is armed from the restart-command URB completion
> +	 * (pcan_usb_restart_pending()), which is not anchored and so outlives
> +	 * ndo_stop/unlink_all_urbs. Shut it down -- not just delete it -- so a
> +	 * late completion cannot re-arm it after free_candev().
> +	 */
> +	timer_shutdown_sync(&pdev->restart_timer);
> +}

[Severity: Medium]
Can timer_shutdown_sync(&pdev->restart_timer) prevent a use-after-free if
pcan_usb_restart_pending() runs after free_candev()?

pdev->restart_timer is embedded in struct pcan_usb (allocated inside
netdev_priv()), so a late pcan_usb_restart_pending() calling
mod_timer(&pdev->restart_timer, ...) after free_candev() still
dereferences freed memory.

Also, in peak_usb_create_dev(), register_candev() is called before
dev_init() and dev_set_bus():

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_create_dev() {
    ...
	err = register_candev(netdev);
    ...
	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);
    ...
}

If the registered interface is brought up and triggers a restart while
probe is still running, a failure in pcan_usb_get_serial() inside
pcan_usb_init() jumps to lbl_unregister_candev and skips pcan_usb_free()
completely.  A failure in dev_set_bus() runs pcan_usb_free() before
unregister_candev(), and because probe failure does not call
usb_disable_interface() and peak_usb_unlink_all_urbs() does not kill the
unanchored restart URB, that URB can remain in flight across
free_candev().

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