[PATCH v2] can: peak_usb: pcan_usb: shut down restart timer on free to avoid use-after-free

[PATCH v2] can: peak_usb: pcan_usb: shut down restart timer on free to avoid use-after-free

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

pcan_usb arms pdev->restart_timer from pcan_usb_restart_pending(), the
completion of the restart-command URB submitted by pcan_usb_restart_async()
on bus-off auto-restart or a manual CAN_MODE_START. That URB is not
anchored to dev->tx_submitted/rx_submitted, so peak_usb_unlink_all_urbs()
does not kill it, and the completion can run -- re-arming the timer via
mod_timer() -- after the interface has been stopped.

pcan_usb provides no teardown for the timer, so on disconnect (or a probe
error path reaching the adap_dev_free label) peak_usb_disconnect() calls
free_candev() with the timer still live. A pending or late-armed timer
then runs pcan_usb_restart() against the freed struct pcan_usb ->
use-after-free.

Add a pcan_usb dev_free callback that calls timer_shutdown_sync() on
restart_timer. peak_usb_disconnect() invokes dev_free() after
unregister_candev() and before free_candev(), by which point the
endpoint URBs have been flushed by usb_disable_interface(). Unlike
timer_delete_sync(), timer_shutdown_sync() also prevents a late URB
completion from re-arming the timer, closing the window a plain delete
would leave open.

Not reproduced on hardware (no PEAK PCAN-USB adapter); found by review of
the restart-timer teardown path.

Fixes: 46be265d3388 ("can: usb: PEAK-System Technik PCAN-USB specific part")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
index 8fd058c32856..113b2e745331 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);
+}
+
 static int pcan_usb_init(struct peak_usb_device *dev)
 {
 	struct pcan_usb *pdev = container_of(dev, struct pcan_usb, dev);
@@ -1051,6 +1064,7 @@ const struct peak_usb_adapter pcan_usb = {
 	.dev_decode_buf = pcan_usb_decode_buf,
 	.dev_encode_msg = pcan_usb_encode_msg,
 	.dev_start = pcan_usb_start,
+	.dev_free = pcan_usb_free,
 	.dev_restart_async = pcan_usb_restart_async,
 	.do_get_berr_counter = pcan_usb_get_berr_counter,
 };
-- 
2.43.0

Re: [PATCH v2] can: peak_usb: pcan_usb: shut down restart timer on free to avoid use-after-free

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete on the probe error path it claims to cover
  (the adap_dev_free label in peak_usb_create_dev()).
- [High] The dev_init() failure path in peak_usb_create_dev() skips the
  new dev_free() callback completely, so the restart timer is still…

--

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

This adds a pcan_usb dev_free callback that calls timer_shutdown_sync() on
pdev->restart_timer. The goal is to keep the restart timer from running
against a freed struct pcan_usb once free_candev() has run. The commit
message says this covers disconnect and the probe error path that reaches
the adap_dev_free label.

> 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: High]
Does this cover the adap_dev_free probe error path in peak_usb_create_dev()?
The commit message says it does.

On disconnect, usb_unbind_interface() calls usb_disable_interface() before
->disconnect(). That flushes the unanchored restart URB before dev_free()
and free_candev() run.

On probe failure, usb_probe_interface() takes its err: path, which does not
disable the interface endpoints. Nothing in peak_usb_create_dev() kills the
restart URB either:

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

register_candev() has already run at this point, so can0 can be brought up.
A bus-off followed by auto-restart or a manual restart then reaches
peak_usb_restart()->pcan_usb_restart_async(). That submits the bus-on
command on EP_CMDOUT with urb->context = pdev. The URB is not anchored, so
peak_usb_unlink_all_urbs() never touches it.

If the device delays EP_CMDOUT, the synchronous dev_set_bus(dev, 0) queued
behind it can time out, and probe goes to adap_dev_free. When the restart
URB completes later (the device finally ACKs, or an unplug flushes it with
-ESHUTDOWN), we get:

pcan_usb_restart_pending()
    struct pcan_usb *pdev = urb->context;
    mod_timer(&pdev->restart_timer, ...);

and pdev has already been freed by free_candev().

timer_shutdown_sync() only makes mod_timer() a no-op while the timer_list
memory is still valid. Can it stop this write to freed memory? If not, the
comment above ("so a late completion cannot re-arm it after free_candev()")
does not hold on this path.

The commit message says "by which point the endpoint URBs have been flushed
by usb_disable_interface()". That also seems to apply to disconnect only.

There is also a smaller window on this path. dev_free() runs before
unregister_candev(), so the interface can stay up with a timer that has
been shut down. The only guard against a new submission is this check:

pcan_usb_restart_async()
	if (timer_pending(&pdev->restart_timer))
		return -EBUSY;

After shutdown that check is always false. Can new restart URBs be
submitted in that window that never reach peak_usb_restart_complete()?

Would anchoring the restart URB and killing that anchor before
free_candev() on every path be a better fix than relying on the timer
shutdown alone?

> +}
> +
>  static int pcan_usb_init(struct peak_usb_device *dev)
>  {
>  	struct pcan_usb *pdev = container_of(dev, struct pcan_usb, dev);
> @@ -1051,6 +1064,7 @@ const struct peak_usb_adapter pcan_usb = {
>  	.dev_decode_buf = pcan_usb_decode_buf,
>  	.dev_encode_msg = pcan_usb_encode_msg,
>  	.dev_start = pcan_usb_start,
> +	.dev_free = pcan_usb_free,

[Severity: High]
Does a dev_init() failure still leave the restart timer live at
free_candev()? In peak_usb_create_dev(), this path jumps past the
adap_dev_free label, so pcan_usb_free() is never called:

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

pcan_usb_init() calls timer_setup() on restart_timer and then calls the
synchronous pcan_usb_get_serial(). That call can block for up to
PCAN_USB_COMMAND_TIMEOUT and then fail:

drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_init() {
    ...
	timer_setup(&pdev->restart_timer, pcan_usb_restart, 0);
    ...
	err = pcan_usb_get_serial(dev, &serial_number);
	if (err) {
		...
		return err;
	}
    ...
}

register_candev() has already run, so can0 can be brought up during that
window. A bus-off and restart can then submit the restart URB, and its
completion arms restart_timer.

When pcan_usb_get_serial() fails, unregister_candev() and free_candev() run
with no timer teardown. The timer may still be pending, or the restart URB
may still be in flight. If the timer fires,
pcan_usb_restart()->peak_usb_restart_complete() writes dev->can.state and
calls netif_wake_queue() on freed memory.

Should pcan_usb_init() shut down the timer (and kill the restart URB) on its
own error return? Alternatively, the core could call dev_free() on this path
too.

>  	.dev_restart_async = pcan_usb_restart_async,
>  	.do_get_berr_counter = pcan_usb_get_berr_counter,
>  };

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109044911.1163505-1-hj351016%40gmail.com