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

[netdev-bot+sashiko@kernel.org]

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