[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

[sashiko-bot]

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

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

[khj]

Thanks for the review.

You're right that timer_shutdown_sync() alone is not sufficient. The
restart URB submitted in pcan_usb_restart() is not anchored, so
peak_usb_unlink_all_urbs() does not kill it, and its completion handler
pcan_usb_restart_pending() can still run after free_candev() and
dereference the freed pcan_usb via mod_timer(&pdev->restart_timer, ...).
Shutting the timer down does not help once the object backing it is gone.

So the complete fix has to stop that URB as well, not just the timer --
e.g. anchor the restart URB so peak_usb_unlink_all_urbs() kills it, and
also cover the probe-error paths you point out (dev_free() being skipped
on a pcan_usb_get_serial() failure, and running before unregister_candev()
on a dev_set_bus() failure, with the restart URB potentially still in
flight because probe failure does not unlink it).

Since that becomes a teardown reorganization in your driver, I'd rather
not guess at the shape. Marc, how would you prefer this handled -- anchor
the restart URB into one of the existing anchors (tx_submitted) or a
dedicated one, and should the register_candev()-before-dev_init() ordering
be part of the same change? I'm happy to send a v3 once the preferred
direction is clear.

Thanks,
Hyunjung

2026년 11월 10일 (화) 오후 1:49, <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

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On 10.11.2026 14:04:08, khj wrote:
> You're right that timer_shutdown_sync() alone is not sufficient. The
> restart URB submitted in pcan_usb_restart() is not anchored, so
> peak_usb_unlink_all_urbs() does not kill it, and its completion handler
> pcan_usb_restart_pending() can still run after free_candev() and
> dereference the freed pcan_usb via mod_timer(&pdev->restart_timer, ...).
> Shutting the timer down does not help once the object backing it is gone.
>
> So the complete fix has to stop that URB as well, not just the timer --
> e.g. anchor the restart URB so peak_usb_unlink_all_urbs() kills it, and
> also cover the probe-error paths you point out (dev_free() being skipped
> on a pcan_usb_get_serial() failure, and running before unregister_candev()
> on a dev_set_bus() failure, with the restart URB potentially still in
> flight because probe failure does not unlink it).
>
> Since that becomes a teardown reorganization in your driver, I'd rather
> not guess at the shape. Marc, how would you prefer this handled -- anchor
> the restart URB into one of the existing anchors (tx_submitted) or a
> dedicated one, and should the register_candev()-before-dev_init() ordering
> be part of the same change? I'm happy to send a v3 once the preferred
> direction is clear.

Please discuss driver design with Stéphane Grosjean (Cc'ed). Your
patch/series should probably based on
https://lore.kernel.org/all/20261103-canfd_check_channel_idx-v8-0-4e8b2c0a8806@peak-system.fr/

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]