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

[khj]

Thanks -- this is correct. v1's dev_stop + timer_delete_sync does not
cover the re-arm: restart_timer is armed from the restart-command URB
completion (pcan_usb_restart_pending),
which is not anchored and can run after the delete, and neither
disconnect nor the adap_dev_free probe-error path calls dev_stop at
all.

v2 drops the dev_stop approach and adds a pcan_usb .dev_free that
timer_shutdown_sync()s restart_timer. dev_free() runs after
unregister_candev() and before free_candev(),
and shutdown (unlike delete) also prevents a late URB completion from
re-arming the timer. Respinning as a new thread.

pw-bot: cr

2026년 11월 9일 (월) 오후 1:25, <netdev-bot+sashiko@xxxxxxxxxx>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · 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 leaves the use-after-free reachable, because
>   restart_timer can be armed again after pcan_usb_stop() has deleted it.
>
> --
>
> can: peak_usb: pcan_usb: cancel restart timer on stop to avoid use-after-free
>
> This adds a dev_stop callback to the PCAN-USB adapter. The callback calls
> timer_delete_sync() on pdev->restart_timer, so the timer is drained in
> peak_usb_ndo_stop(). The aim is to stop pcan_usb_restart() from running
> on a freed struct pcan_usb after free_candev().
>
> > 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);
>
> [Severity: High]
> Can restart_timer be armed again after this delete?
>
> The timer is armed by the restart URB completion, not by the timer
> callback. The completion calls mod_timer() every time and never checks
> urb->status:
>
> drivers/net/can/usb/peak_usb/pcan_usb.c:pcan_usb_restart_pending() {
>         ...
>         mod_timer(&pdev->restart_timer,
>                         jiffies + msecs_to_jiffies(PCAN_USB_STARTUP_TIMEOUT));
>         ...
> }
>
> pcan_usb_restart_async() submits that URB without anchoring it to
> dev->rx_submitted or dev->tx_submitted:
>
>         usb_fill_bulk_urb(urb, dev->udev,
>                         usb_sndbulkpipe(dev->udev, PCAN_USB_EP_CMDOUT),
>                         buf, PCAN_USB_CMD_LEN,
>                         pcan_usb_restart_pending, pdev);
>
>         return usb_submit_urb(urb, GFP_ATOMIC);
>
> So peak_usb_unlink_all_urbs() never kills it.
>
> Suppose a bus-off auto-restart, or a manual restart through netlink, has
> this URB in flight when the interface is brought down. The path is
> peak_usb_set_mode(CAN_MODE_START) -> peak_usb_restart() ->
> pcan_usb_restart_async(). peak_usb_ndo_stop() then runs as follows:
>
>   close_candev()
>     cancels only the restart work
>   peak_usb_unlink_all_urbs()
>     kills only rx_submitted and tx_submitted
>   dev->adapter->dev_stop()
>     timer is not pending yet, so nothing is deleted
>   dev->adapter->dev_set_bus(dev, 0)
>     synchronous usb_bulk_msg() on PCAN_USB_EP_CMDOUT
>
> Bulk URBs on one endpoint complete in order. Would the queued restart URB
> then complete and call mod_timer() after pcan_usb_stop() has returned?
>
> Next, suppose the device is unplugged, unbound, or the module is removed
> within PCAN_USB_STARTUP_TIMEOUT. peak_usb_disconnect() calls
> unregister_candev() on an interface that is already down, so ndo_stop
> and dev_stop do not run again. pcan_usb sets no .dev_free, so nothing
> stops the timer before free_candev():
>
> drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_disconnect() {
>         ...
>                 unregister_candev(netdev);
>
>                 kfree(dev->cmd_buf);
>                 dev->next_siblings = NULL;
>                 if (dev->adapter->dev_free)
>                         dev->adapter->dev_free(dev);
>
>                 free_candev(netdev);
>         ...
> }
>
> pcan_usb_restart() -> peak_usb_restart_complete() would then write
> dev->can.state and call netif_wake_queue() on freed memory.
>
> No disconnect is needed for a related problem. Wouldn't the late timer
> still move a stopped interface from CAN_STATE_STOPPED to
> CAN_STATE_ERROR_ACTIVE and wake its queue after ndo_stop?
>
> The commit message says:
>
>   pcan_usb_restart() does not re-arm the timer, so a single
>   timer_delete_sync() drains it.
>
> That holds for the callback. Does it cover the case here, though, where
> the timer is armed by the asynchronous URB completion?
>
> Would a pcan_usb .dev_free callback that calls timer_shutdown_sync()
> work instead? peak_usb_disconnect() calls it after unregister_candev()
> and before free_candev(). By then, usb_unbind_interface() has already
> flushed the endpoint URBs with usb_disable_interface(), because the
> driver has no soft_unbind.
>
> Another option is to anchor the restart URB and kill it before dev_stop
> deletes the timer.
>
> The probe error paths in peak_usb_create_dev() also reach free_candev()
> with no restart_timer teardown for pcan_usb. These are the adap_dev_free
> and lbl_unregister_candev labels. Would the dev_free approach cover those
> paths too?
>
> [ ... ]
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107153319.3765325-1-hj351016%40gmail.com