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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown order in `f81604_close()` calls `close_candev()` before killing URBs, leaving the device vulnerable to `restart_work` Use-After-Free and `echo_skb` Double-Free.
- [High] TX URBs are not anchored, causing them to leak and trigger a Use-After-Free on device disconnect.
- [High] Duplicate URB submission and memory leak during CAN bus-off recovery.
- [High] Pending `clear_reg_work` is leaked if `f81604_open()` fails, leading to a use-after-free upon device disconnect.
--

commit a9d42819f6aa6bc14b0da3547f398a4d371e4aab
Author: Ji-Ze Hong (Peter Hong) <peter_hong@fintek.com.tw>
Subject: can: usb: f81604: fix use-after-free on disconnect

This commit unregisters the URBs before cancelling clear_reg_work. This
prevents an URB completion handler from scheduling work after
cancel_work_sync() has returned, which would otherwise result in a
use-after-free.

Closes: https://lore.kernel.org/all/20260824133009.CF85A1F00A3A@smtp.kernel.org/

> 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);
>  
>  	netif_stop_queue(netdev);
> -	cancel_work_sync(&priv->clear_reg_work);
>  	close_candev(netdev);

[Severity: High]
This isn't a bug introduced by this patch, but close_candev() is executed
while URBs are still active and capable of triggering concurrent callbacks.

Can this order result in a use-after-free or double-free? 

If an active URB completes concurrently, could it trigger can_bus_off() (which
reschedules restart_work AFTER it was cancelled in close_candev()) or access
an echo_skb that is currently being flushed?

>  
>  	f81604_unregister_urbs(priv);
> +	cancel_work_sync(&priv->clear_reg_work);
>  
>  	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but looking at f81604_start_xmit():

drivers/net/can/usb/f81604.c:f81604_start_xmit() {
[ ... ]
	write_urb = usb_alloc_urb(0, GFP_ATOMIC);
	if (!write_urb)
		goto nomem_urb;
[ ... ]
	ret = usb_submit_urb(write_urb, GFP_ATOMIC);
[ ... ]
}

The TX URB is created here but is never anchored to priv->urbs_anchor.

When the device is disconnected, f81604_disconnect() frees the netdev. Since
the TX URB is unanchored, f81604_close() does not kill it.

If the uncancelled TX URB completes asynchronously later, would it access the
freed netdev pointer in f81604_write_bulk_callback()?

drivers/net/can/usb/f81604.c:f81604_write_bulk_callback() {
	struct net_device *netdev = urb->context;

	if (!netif_device_present(netdev))
		return;
[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak during CAN
bus-off recovery:

drivers/net/can/usb/f81604.c:f81604_set_mode() {
	switch (mode) {
	case CAN_MODE_START:
		ret = f81604_start(netdev);
[ ... ]
}

When the device enters CAN_STATE_BUS_OFF, the core schedules bus-off recovery
which invokes f81604_set_mode(..., CAN_MODE_START).

This calls f81604_start() which unconditionally registers URBs:

drivers/net/can/usb/f81604.c:f81604_start() {
[ ... ]
	ret = f81604_register_urbs(priv);
	if (ret)
		return ret;
[ ... ]
}

Does this unconditionally allocate and submit a new set of RX and INT URBs
without killing or unregistering the previously active URBs?

[Severity: High]
This is a pre-existing issue, but there appears to be a leaked work struct if
f81604_open() fails:

drivers/net/can/usb/f81604.c:f81604_start() {
[ ... ]
	ret = f81604_register_urbs(priv);
	if (ret)
		return ret;

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

If f81604_set_normal_mode() fails, the error path unregisters the URBs, but
does not cancel priv->clear_reg_work (which might have been scheduled if an
INT URB completed immediately).

Because the interface was never UP, ndo_stop (f81604_close()) is not called
when the device is unplugged:

drivers/net/can/usb/f81604.c:f81604_disconnect() {
[ ... ]
		unregister_netdev(priv->netdev[i]);
		free_candev(priv->netdev[i]);
[ ... ]
}

Will this cause a use-after-free when the pending clear_reg_work executes
and accesses the freed priv struct?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901-f81604-fix-v1-1-c55b5178f9f8@fintek.com.tw?part=1