Re: [PATCH v2 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: 1 · High: 0 (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).

- [Critical] Broken error unwind in gs_usb_probe()
  (drivers/net/can/usb/gs_usb.c) when register_candev() fails.

Pre-existing issues:
- [High] The probe error unwind in gs_usb_probe() frees channel objects
  before it stops the shared RX URBs.

--

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(). The aim is to set dev->parent and
parent->hf_size_rx before any CAN interface becomes visible to userspace.
The error paths now share a common out_usb_kill_anchored_urbs label.

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..2564c16727935 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -1621,7 +1612,36 @@ 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) {
> +			const unsigned int cur = i;
> +
> +			dev_err(&intf->dev,
> +				"Couldn't register candev for channel %d (%pe)\n",
> +				i, ERR_PTR(rc));
> +
> +			for (i = 0; i < cur; i++)
> +				gs_destroy_candev(dev);
> +
> +			for (; i < parent->channel_cnt; i++) {
> +				can_rx_offload_del(&dev->offload);
> +				free_candev(dev->netdev);
> +			}

[Severity: Critical]
Both cleanup loops use dev, and dev still points at parent->canch[cur],
the channel whose register_candev() just failed. Should these use
parent->canch[i], like the gs_make_candev() failure loop earlier in
gs_usb_probe()?

As written, the first loop calls gs_destroy_candev() cur times on
canch[cur], which was never registered. unregister_candev() on a
NETREG_UNINITIALIZED netdev hits the WARN_ON(1) in
unregister_netdevice_many_notify(). free_netdev() then frees it
immediately:

net/core/dev.c:free_netdev() {
    ...
	if (dev->reg_state == NETREG_UNINITIALIZED ||
	    dev->reg_state == NETREG_DUMMY) {
		kvfree(dev);
    ...
}

So when cur >= 2, does each later pass touch freed memory and then free
it again?

The second loop then calls can_rx_offload_del() and free_candev() on the
same dev once for each remaining channel. If cur >= 1, dev has already
been freed. If cur == 0 and channel_cnt >= 2, canch[0] is freed
channel_cnt times.

Aren't the netdevs and NAPI instances of canch[cur + 1] through
canch[channel_cnt - 1] leaked as well?

Also, canch[0] through canch[cur - 1] are never unregistered, but their
dev->parent points at the struct gs_usb that kfree(parent) frees below.
Since probe fails, gs_usb_disconnect() never runs for them. Wouldn't any
later open, close, xmit or ethtool call on those netdevs dereference the
freed parent?

Userspace may have opened one of them during the window. In that case
gs_can_open() may have started the self-rearming parent->timestamp
delayed work through gs_usb_timestamp_init(), and only gs_can_close()
cancels it.

The device can force this failure. gs_make_candev() copies sjw_max from
the GS_USB_BREQ_BT_CONST reply without checking it. register_candev()
then fails with -EINVAL through can_bittiming_const_valid():

	if (!btc->sjw_max)
		return false;

A device that reports icount >= 2 and sjw_max == 0 for channel 1 would
reach this unwind every time it is plugged in.

The later patches in this series only touch gs_can_start_xmit() and
gs_usb_receive_bulk_callback(), so this is still present at the end of
the series.

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

[Severity: High]
This isn't a bug introduced by this patch, but the unwind frees the
channel objects before usb_kill_anchored_urbs() stops the shared RX URBs.
The baseline code used the same order.

Suppose userspace opened two or more of the registered channels during
the probe window. In that case gs_destroy_candev() -> unregister_candev()
-> gs_can_close() only kills the RX URBs once active_channels reaches 0:

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

So the RX URBs keep completing while canch[0] is freed, and
gs_usb_receive_bulk_callback() does:

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

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

	if (!netif_device_present(netdev))

Can this dereference a freed gs_can and net_device when the device sends
a frame with hf->channel == 0?

The device_detach path in the same callback walks every
parent->canch[rc]->netdev, so it seems to have the same exposure.

The disconnect path is not affected, because usb_unbind_interface()
disables the endpoints before calling ->disconnect(). The probe failure
path gets no such help from the USB core.

Would it make sense to kill the RX URBs before tearing down the channels
in this unwind?

> +
> +	return rc;
>  }

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