Re: [PATCH] can: usb: f81604: fix use-after-free on disconnect

[Marc Kleine-Budde]

> Unregister the URBs before cancelling clear_reg_work to prevent an URB
> completion handler from scheduling work after cancel_work_sync() has
> returned.
>
> Otherwise the pending work may run after the netdev and private data are
> freed, resulting in a use-after-free.
>
> Suggested-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20260824133009.CF85A1F00A3A@xxxxxxxxxxxxxxx/
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Ji-Ze Hong (Peter Hong) <peter_hong@xxxxxxxxxxxxx>

There was some more review feedback from the netdev sashiko, can you
take care of them?

regards,
Marc

pw-bot: cr

--- Patch [29]: [PATCH net-next 29/37] can: f81604: f81604_close(): fix use-after-free on disconnect ---
commit b7c039b44e69e7dd0760d88f656535fd56000877
Author: Ji-Ze Hong (Peter Hong) <peter_hong@xxxxxxxxxxxxx>

can: f81604: f81604_close(): fix use-after-free on disconnect

This commit moves cancel_work_sync(&priv->clear_reg_work) in f81604_close()
to after f81604_unregister_urbs(). After that change, an URB completion
handler can no longer schedule clear_reg_work once the cancel has returned.
The goal is to stop the work from running after the netdev and private data
are freed.

Closes: https://lore.kernel.org/all/20260824133009.CF85A1F00A3A@xxxxxxxxxxxxxxx/
Link: https://patch.msgid.link/20260901-f81604-fix-v1-1-c55b5178f9f8@xxxxxxxxxxxxx

> Cc: stable@xxxxxxxxxxxxxxx

Discovered by: gpt-6-astra, opus-5-5 · Missed by: sashiko-gemini
This isn't a bug, but should this have a Fixes: tag next to the stable Cc?
The teardown order in f81604_close() looks unchanged since the driver was
added, so the tag would be:

Fixes: 88da17436973 ("can: usb: f81604: add Fintek F81604 support")

> diff --git a/drivers/net/can/usb/f81604.c b/drivers/net/can/usb/f81604.c
> index 4c147b9d6d690..bb155b8769245 100644
> --- a/drivers/net/can/usb/f81604.c
> +++ b/drivers/net/can/usb/f81604.c
> @@ -1075,10 +1075,10 @@ static int f81604_close(struct net_device *netdev)
>  	f81604_set_reset_mode(priv);

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked High by: gpt-6-astra
This is a pre-existing issue, but can a bus-off restart running at the same
time undo this reset? can_restart_work()->can_restart() does not take RTNL.
The restart_work timer could fire while f81604_set_reset_mode() is doing
its control transfers:

can_restart()
  f81604_set_mode(CAN_MODE_START)
    f81604_start()
      f81604_set_normal_mode()
        clears MOD.RM, writes IER = IRQ_ALL & ~RI

In that case, cancel_delayed_work_sync() in close_candev() only waits for
the restart to finish. f81604_unregister_urbs() then kills the URBs the
restart submitted, but nothing puts the chip back into reset mode.

Would ndo_stop then return with the SJA1000 core still in operating mode on
the bus while the netdev is down?

>
>  	netif_stop_queue(netdev);
> -	cancel_work_sync(&priv->clear_reg_work);
>  	close_candev(netdev);

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked Critical by: gpt-6-astra
This reorders clear_reg_work, but doesn't restart_work have the same
problem? close_candev() runs cancel_delayed_work_sync(&priv->restart_work)
here, but the int URB stays live until f81604_unregister_urbs() below.

Suppose an int URB completes in that window with F81604_SJA1000_IRQ_EI and
SR.BS set. f81604_handle_can_bus_errors() then does:

	if (data->sr & F81604_SJA1000_SR_BS)
		can_state = CAN_STATE_BUS_OFF;
	...
		if (can_state == CAN_STATE_BUS_OFF)
			can_bus_off(netdev);

If restart_ms is set, can_bus_off() re-arms the work:

	if (priv->restart_ms)
		schedule_delayed_work(&priv->restart_work,
				      msecs_to_jiffies(priv->restart_ms));

The interrupt report could be a buffered or in-flight one, or come from a
malicious device. It can also arrive if the IER-off write in
f81604_set_reset_mode() failed, because f81604_close() ignores its return
value.

Nothing cancels restart_work after that. When it fires,
can_restart()->f81604_set_mode()->f81604_start() puts the chip back into
normal mode, resubmits the URBs and wakes the queue on an interface that is
down.

If the device is unplugged before restart_ms expires,
f81604_disconnect()->unregister_netdev() skips ndo_stop because the device
is already down. free_candev() then frees can_priv while the restart_work
timer is still armed.

Would it be better to call f81604_unregister_urbs() before close_candev(),
as gs_usb and peak_usb do?

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked Critical by: gpt-6-astra
This is a pre-existing issue, but can close_candev()->can_flush_echo_skb()
race here with URB completions that use priv->echo_skb[0]?

The int URB is only killed later, by f81604_unregister_urbs(). The TX URBs
from f81604_start_xmit() are never anchored, so they are never killed or
waited for:

f81604_start_xmit()
	can_put_echo_skb(skb, netdev, 0, 0);

	ret = usb_submit_urb(write_urb, GFP_ATOMIC);

can_flush_echo_skb() checks and frees echo_skb[i] without a lock.
f81604_handle_tx() does the same through can_get_echo_skb() and
can_free_echo_skb(). f81604_write_bulk_callback() does it through
can_free_echo_skb() on error statuses. Both run in completion context.

If both sides see a non-NULL echo_skb[0], could the skb be freed twice? Or
could __can_get_echo_skb() read it after the flush has freed it?

Killing all URBs, including TX, before close_candev() looks like it would
avoid this.

>
>  	f81604_unregister_urbs(priv);
> +	cancel_work_sync(&priv->clear_reg_work);

Discovered by: gpt-6-astra, opus-5-5 · Missed by: sashiko-gemini · Ranked Critical by: gpt-6-astra
This covers ndo_stop, but can clear_reg_work still be left pending after a
failed open? In f81604_start(), the URBs are already live when
f81604_set_normal_mode() runs:

	ret = f81604_register_urbs(priv);
	if (ret)
		return ret;

	ret = f81604_set_normal_mode(priv);
	if (ret) {
		f81604_unregister_urbs(priv);
		return ret;
	}

The int URB may complete with error bits in isrc during
f81604_set_normal_mode(). If so, f81604_handle_can_bus_errors() calls
schedule_work(&priv->clear_reg_work).

If f81604_set_normal_mode() then fails (for example -ENODEV during
hot-unplug), the error path kills the URBs but does not cancel the work.
f81604_open() then only calls close_candev().

Since the open failed, IFF_UP is never set, so on disconnect:

f81604_disconnect()
  unregister_netdev()
    netif_close_many() skips ndo_stop for devices without IFF_UP
  free_candev()
    frees the netdev and the embedded f81604_port_priv

Could f81604_clear_reg_work() then run on the freed priv?

Would adding cancel_work_sync() to the f81604_start() error path, or
calling it unconditionally in f81604_disconnect() before free_candev(),
cover this case?

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra
This is a pre-existing issue, but does each bus-off restart submit another
set of URBs on top of the ones already running?

can_restart()->f81604_set_mode() calls f81604_start(). f81604_start() then
calls f81604_register_urbs() without calling f81604_unregister_urbs()
first. The completion callbacks resubmit on success, so nothing kills the
open-time URBs at bus-off.

Each restart would then leave another F81604_MAX_RX_URBS bulk URBs plus an
int URB in flight, with their buffers, until ndo_stop.

Also, if f81604_set_normal_mode() fails during a restart, the error path
calls f81604_unregister_urbs(), which kills every anchored URB, including
the open-time set. Would the interface then stay up with no RX or int URBs
until the next down/up?

-- 
Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>