Re: [PATCH 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (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] This patch adds a memory leak to the gs_usb_probe() error path
  in drivers/net/can/usb/gs_usb.c.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the
  gs_usb_probe() register-failure path keeps an existing use-after-free.

--

can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

This patch moves register_candev() out of gs_make_candev() and into a
second loop in gs_usb_probe(). Every channel, including dev->parent and
parent->hf_size_rx, is now fully set up before any channel is registered.
This closes the window where userspace could bring up a partially
initialized CAN interface.

Closes: https://sashiko.dev/#/bug/linux-be54de7d-0b75-408c-b3ee-dd90c9756857
Closes: https://sashiko.dev/#/bug/linux-71076d59-0843-4749-a5ab-fdd98a08582f

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 15f28833f2deb..985d6a9113d96 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1490,18 +1490,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
>  	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
> -	rc = register_candev(dev->netdev);
> -	if (rc) {
> -		dev_err(&intf->dev,
> -			"Couldn't register candev for channel %d (%pe)\n",
> -			channel, ERR_PTR(rc));
> -		goto out_can_rx_offload_del;
> -	}
> -
>  	return dev;
>  
> -out_can_rx_offload_del:
> -	can_rx_offload_del(&dev->offload);
>  out_free_candev:
>  	free_candev(dev->netdev);
>  	return ERR_PTR(rc);
> @@ -1593,14 +1583,14 @@ static int gs_usb_probe(struct usb_interface *intf,
>  			/* save error code to return later */
>  			rc = PTR_ERR(parent->canch[i]);
>  
> -			/* on failure destroy previously created candevs */
> +			/* on failure clean up previously created candevs */
>  			icount = i;
> -			for (i = 0; i < icount; i++)
> -				gs_destroy_candev(parent->canch[i]);
> +			for (i = 0; i < icount; i++) {
> +				can_rx_offload_del(&parent->canch[i]->offload);
> +				free_candev(parent->canch[i]->netdev);
> +			}
>  
> -			usb_kill_anchored_urbs(&parent->rx_submitted);
> -			kfree(parent);
> -			return rc;
> +			goto out_usb_kill_anchored_urbs;
>  		}
>  		parent->canch[i]->parent = parent;
>  

[ ... ]

> @@ -1621,7 +1611,30 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
>  	}
>  
> +	for (i = 0; i < icount; i++) {
> +		struct gs_can *dev = parent->canch[i];
> +
> +		rc = register_candev(dev->netdev);
> +		if (rc) {
> +			dev_err(&intf->dev,
> +				"Couldn't register candev for channel %d (%pe)\n",
> +				i, ERR_PTR(rc));
> +
> +			icount = i;
> +			for (i = 0; i < icount; i++)
> +				gs_destroy_candev(parent->canch[i]);

[Severity: High]
Does this leak the candevs for channel i and every channel after it?

The second loop only runs after the first loop has called gs_make_candev()
for all channels. At that point every canch[] entry holds an allocated
netdev with rx-offload attached.

When register_candev() fails for channel i, only canch[0..i-1] go through
gs_destroy_candev(). Channels canch[i] through canch[channel_cnt - 1] never
get can_rx_offload_del() or free_candev().

The kfree(parent) at out_usb_kill_anchored_urbs then frees canch[], which
held the only pointers to those netdevs.

Before this patch, gs_make_candev() cleaned up its own channel when
register_candev() failed, and no later channels had been allocated yet.

The USB device can trigger this. register_candev() returns -EINVAL when
can_bittiming_const_valid() fails:

drivers/net/can/dev/dev.c:can_bittiming_const_valid() {
    ...
	if (!btc->sjw_max)
		return false;
    ...
}

gs_make_candev() copies sjw_max straight from the device's
GS_USB_BREQ_BT_CONST reply. A device that reports sjw_max == 0 for channel
0 would leak every channel on each probe.

Should this error path also run can_rx_offload_del() and free_candev() on
channels i through channel_cnt - 1, the same teardown the first loop's
error path uses?

> +
> +			goto out_usb_kill_anchored_urbs;
> +		}
> +	}
> +
>  	return 0;
> +
> +out_usb_kill_anchored_urbs:
> +	usb_kill_anchored_urbs(&parent->rx_submitted);
> +	kfree(parent);
> +
> +	return rc;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but can this teardown order
cause a use-after-free in gs_usb_receive_bulk_callback()?

The old gs_make_candev() failure path had the same pattern. Registered
channels 0..i-1 are destroyed one at a time, and gs_destroy_candev() frees
each netdev right after unregistering it. The freed pointers stay in
parent->canch[].

The shared RX URBs are only killed when the last active channel closes:

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

The only other place they are killed is out_usb_kill_anchored_urbs, which
runs after the whole destroy loop.

Suppose userspace has already brought up canch[0] and canch[1] when
register_candev() fails for a later channel. Unregistering canch[0] closes
it, but active_channels is still 1, so the RX URBs keep running after
canch[0] is freed.

If the device then sends a frame with hf->channel == 0, the callback
dereferences the freed entry:

gs_usb_receive_bulk_callback() {
    ...
	dev = parent->canch[hf->channel];

	netdev = dev->netdev;
	stats = &netdev->stats;

	if (!netif_device_present(netdev))
    ...
}

A frame with an out-of-range channel id goes to the device_detach label
instead. That code calls netif_device_detach() on every non-NULL canch[]
entry, which writes to the state field of the freed netdev.

The window is shorter now that no USB control messages run between
registrations, but the ordering is the same. gs_usb_disconnect() is not
affected, because usb_unbind_interface() disables the interface before it
calls disconnect.

Would it help to unregister all the registered channels first, so the last
close stops the shared RX URBs, and only then free them?

>  
>  static void gs_usb_disconnect(struct usb_interface *intf)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025-gs_usb-fixes-v1-0-f74ebbd4da36%40pengutronix.de