Re: [PATCH] can: gs_usb: kill RX URBs before destroying the netdevs

[Marc Kleine-Budde]

On 23.09.2026 07:03:52, Fan Wu wrote:
> gs_usb_disconnect() destroys the channels one by one via
> gs_destroy_candev()/free_candev(). gs_can_close() disposes the RX bulk
> URBs on the shared parent->rx_submitted anchor only when the last
> active channel is closed. With two or more channels up, the earlier
> channels are freed while their RX URBs are still submitted, and a
> completion in gs_usb_receive_bulk_callback() accesses the freed struct
> gs_can and struct net_device.
>
> Fix this by killing the anchored RX URBs in gs_usb_disconnect() before
> the first netdev is destroyed, and in the error path of gs_usb_probe()
> before the previously created netdevs are destroyed.
>
> usb_kill_anchored_urbs() waits for running completions and a killed URB
> completes with -ENOENT, so the completion handler returns without
> resubmitting the URB. The kill in gs_can_close() of the last active
> channel then operates on an already empty anchor.
>
> This issue was found by an in-house static analysis tool.
>
> Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
> Cc: stable@xxxxxxxxxxxxxxx
> Co-developed-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Song Li <songl@xxxxxxxxxx>
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>

The netdev review pipeline has some remarks about the
usb_kill_anchored_urbs:

| https://netdev-ai.bots.linux.dev/sashiko/#patch-15522

Please have a look and send a v2 patch.

regards,
Marc

--- Patch [11]: [PATCH net 11/16] can: gs_usb: kill RX URBs before destroying the netdevs ---
commit 4b68d32240b11c912a1f8bfe710a965b66660960
Author: Fan Wu <fanwu01@xxxxxxxxxx>

can: gs_usb: kill RX URBs before destroying the netdevs

This patch moves usb_kill_anchored_urbs() in the gs_usb_probe() error
path so that it runs before the previously created candevs are destroyed.
It also adds a kill in gs_usb_disconnect() before the first netdev is
destroyed. The goal is to stop RX completions in
gs_usb_receive_bulk_callback() from touching a freed struct gs_can and
struct net_device.

Link: https://patch.msgid.link/20260923070352.487595-1-fanwu01@xxxxxxxxxx

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 3b9b2f104d868..f604358c8259d 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1595,10 +1595,10 @@ static int gs_usb_probe(struct usb_interface *intf,
>
>  			/* on failure destroy previously created candevs */
>  			icount = i;
> +			usb_kill_anchored_urbs(&parent->rx_submitted);
>  			for (i = 0; i < icount; i++)
>  				gs_destroy_candev(parent->canch[i]);

Discovered by: gpt-6-astra, opus-5-5 · Missed by: sashiko-gemini · Ranked Critical by: gpt-6-astra
Is the probe error path still exposed to the same use-after-free?

usb_kill_anchored_urbs() kills the URBs once but does not poison the
anchor. The netdevs canch[1..icount-1] stay registered and can be opened
while canch[0] is unregistered and freed. The endpoints are also still
enabled during probe, so new submissions succeed.

A possible sequence:

gs_usb_probe() error path
  usb_kill_anchored_urbs(&parent->rx_submitted);
  gs_destroy_candev(parent->canch[0]);
    unregister_candev()
    free_candev()
                              ip link set can1 up
                              gs_can_open()
                                if (!parent->active_channels) {
                                  ...
                                  usb_anchor_urb(urb, &parent->rx_submitted);
                                  rc = usb_submit_urb(urb, GFP_KERNEL);

A gs_can_open() that is already running when the kill happens is not
serialized with it either, so it can keep submitting URBs after the kill.

If the device then sends a frame with hf->channel = 0, it passes the
range check, and gs_usb_receive_bulk_callback() does:

	dev = parent->canch[hf->channel];

	netdev = dev->netdev;

Both of these reads hit freed memory. The canch[] slots are never cleared,
so the device_detach loop could also call netif_device_detach() on the
freed netdev.

The disconnect path doesn't seem to have this problem, because the
endpoints are disabled there and resubmission fails.

Would usb_poison_anchored_urbs() work here, or unregistering every netdev
before freeing any of them? The later HScanT workaround commit in this
series doesn't appear to change this teardown logic either.

>
> -			usb_kill_anchored_urbs(&parent->rx_submitted);
>  			kfree(parent);
>  			return rc;
>  		}

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked Critical by: gpt-6-astra
This isn't a bug introduced by this patch, but
gs_usb_receive_bulk_callback() only checks that hf->channel is in range
before it dereferences the slot:

	if (hf->channel >= parent->channel_cnt)
		goto device_detach;

	dev = parent->canch[hf->channel];

	netdev = dev->netdev;

gs_usb_probe() sets parent->channel_cnt to the full icount before it
creates any channel. gs_make_candev() also calls register_candev() before
the next channel is created.

Suppose can0 is brought up while gs_make_candev(1) is still running its
control transfers, each with a 1000 ms timeout. Could a frame with
channel = 1 then dereference the NULL canch[1] in URB completion context?

Something similar happens when gs_make_candev(i) fails, because
gs_usb_probe() stores the ERR_PTR in parent->canch[i]:

	parent->canch[i] = gs_make_candev(i, intf, &dconf);
	if (IS_ERR_OR_NULL(parent->canch[i])) {

A completion with hf->channel = i would dereference that ERR_PTR. This
applies to a completion that runs before the kill, or to one from URBs
resubmitted after the kill as described above. The device_detach loop
only checks "if (parent->canch[rc])", so it would also pass the ERR_PTR
to netif_device_detach().

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 gs_make_candev() calls register_candev()
before gs_usb_probe() has finished setting up the channel:

	parent->canch[i]->parent = parent;
	...
	parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);

If the netdev is opened before dev->parent is assigned, gs_can_open()
reads a NULL parent. It then dereferences it at
"if (!parent->active_channels)".

There is a second problem with hf_size_rx. Take a classic, non-timestamp
can0 that is opened while later channels are still being probed.
gs_can_open() allocates its RX buffers with
kmalloc(dev->parent->hf_size_rx), which is 20 bytes at that point.

A later channel that reports FD and HW_TIMESTAMP raises
parent->hf_size_rx to struct_size(hf, canfd_ts, 1), which is 80 bytes.
The resubmit path then reuses the old buffer with the new size:

resubmit_urb:
	usb_fill_bulk_urb(urb, parent->udev,
			  parent->pipe_in,
			  hf, parent->hf_size_rx,
			  gs_usb_receive_bulk_callback, parent);

Can the device then DMA up to 80 bytes into the kmalloc-32 object? These
URBs keep being resubmitted until all channels are closed.

> @@ -1636,6 +1636,8 @@ static void gs_usb_disconnect(struct usb_interface *intf)
>  		return;
>  	}
>
> +	usb_kill_anchored_urbs(&parent->rx_submitted);
> +
>  	for (i = 0; i < parent->channel_cnt; i++)
>  		if (parent->canch[i])
>  			gs_destroy_candev(parent->canch[i]);

Discovered by: opus-5-5 · Missed by: gpt-6-astra, sashiko-gemini · Confirmed by: gpt-6-astra · Ranked Critical by: gpt-6-astra
This isn't a bug introduced by this patch, but should gs_usb_disconnect()
cancel parent->timestamp before the kfree(parent) that follows?

The shared work is started and stopped based on the feature bits of
whichever channel opens first or closes last.

In gs_can_open():

	if (!parent->active_channels) {
		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
			gs_usb_timestamp_init(parent);

In gs_can_close():

	if (!parent->active_channels) {
		usb_kill_anchored_urbs(&parent->rx_submitted);

		if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
			gs_usb_timestamp_stop(parent);
	}

dev->feature is read per channel from the device's BT_CONST reply. Say
can0 has HW_TIMESTAMP and can1 does not. The sequence open can0, open
can1, close can0, close can1 then leaves gs_usb_timestamp_work()
re-arming itself with nothing to cancel it.

If can0 is reopened, gs_usb_timestamp_init() runs spin_lock_init() and
INIT_DELAYED_WORK() on a work item that is still armed. On disconnect,
kfree(parent) runs with the work still pending. gs_usb_timestamp_work()
would then use parent->tc_lock and parent->tc, and issue a control
message through parent->udev, all on freed memory.

In the opposite ordering, cancel_delayed_work_sync() runs on a
delayed_work that was never initialized.

-- 
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: signature.asc
Description: PGP signature