[PATCH v2 0/4] gs_usb: fixes

[PATCH v2 0/4] gs_usb: fixes

[Marc Kleine-Budde]

Hello,

this series contains fixes for issues found by sashiko.

Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
Changes in v2:
- patch #2: add missing cleanup if register_candev() fails (pointed out by sashiko)
- patch #3: do not increase rx_errors during OOM (pointed out by sashiko)
- Link to v1: https://patch.msgid.link/20261025-gs_usb-fixes-v1-0-f74ebbd4da36@pengutronix.de

---
Marc Kleine-Budde (4):
      can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB
      can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()
      can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails
      can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

 drivers/net/can/usb/gs_usb.c | 74 ++++++++++++++++++++++++++++++--------------
 1 file changed, 51 insertions(+), 23 deletions(-)
---
base-commit: 734bef548a96e8774d6eef5dfa2026c6edc4e80b
base-tree: git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git main
change-id: 20261025-gs_usb-fixes-250f38833107

Best regards,
--  
Marc Kleine-Budde <mkl@pengutronix.de>

[PATCH v2 1/4] can: gs_usb: gs_can_start_xmit(): don't leak uninitialized memory over USB

[Marc Kleine-Budde]

The driver allocates the transmission buffer using kmalloc(), which means
the memory remains uninitialized. Then various headers are filled and, but
only the actual CAN frame data payload (of length cfd->len or cf->len) is
copied into the buffer, leaving the remaining buffer uninitialized.

Fix this information leak, allocate the buffer with kzalloc().

Closes: https://sashiko.dev/#/bug/linux-0a8c0e8a-c623-426a-8db3-f55111f6cd84
Fixes: 26949ac935e3 ("can: gs_usb: add CAN-FD support")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 3b9b2f104d86..15f28833f2de 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -861,7 +861,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb,
 	if (!urb)
 		goto nomem_urb;
 
-	hf = kmalloc(dev->hf_size_tx, GFP_ATOMIC);
+	hf = kzalloc(dev->hf_size_tx, GFP_ATOMIC);
 	if (!hf)
 		goto nomem_hf;
 

-- 
2.53.0

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

[Marc Kleine-Budde]

In gs_usb_probe(), gs_make_candev() is called to allocate and set up the
CAN channel, as well as to call register_candev(). However, gs_usb_probe()
defers the assignment of dev->parent and parent->hf_size_rx until after
gs_make_candev() returns. This opens a race window for the user space to
bring up the CAN interface, while it's not completely setup.

To close the race window, initialize all CAN interfaces in a first loop and
then register them in a second loop.

Closes: https://sashiko.dev/#/bug/linux-be54de7d-0b75-408c-b3ee-dd90c9756857
Closes: https://sashiko.dev/#/bug/linux-71076d59-0843-4749-a5ab-fdd98a08582f
Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 54 ++++++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 17 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 15f28833f2de..2564c1672793 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);
@@ -1590,17 +1580,18 @@ static int gs_usb_probe(struct usb_interface *intf,
 
 		parent->canch[i] = gs_make_candev(i, intf, &dconf);
 		if (IS_ERR_OR_NULL(parent->canch[i])) {
+			const unsigned int cur = i;
+
 			/* save error code to return later */
 			rc = PTR_ERR(parent->canch[i]);
 
-			/* on failure destroy previously created candevs */
-			icount = i;
-			for (i = 0; i < icount; i++)
-				gs_destroy_candev(parent->canch[i]);
+			/* on failure clean up previously created candevs */
+			for (i = 0; i < cur; 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 +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);
+			}
+
+			goto out_usb_kill_anchored_urbs;
+		}
+	}
+
 	return 0;
+
+out_usb_kill_anchored_urbs:
+	usb_kill_anchored_urbs(&parent->rx_submitted);
+	kfree(parent);
+
+	return rc;
 }
 
 static void gs_usb_disconnect(struct usb_interface *intf)

-- 
2.53.0

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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On 27.10.2026 09:40:52, netdev-bot+sashiko@kernel.org wrote:
> [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?

Valid issue, will fix.

pw-bot: cr

regards,
Marc

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 3/4] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[Marc Kleine-Budde]

If the allocation of an SKB fails in gs_usb_receive_bulk_callback(), the
function neither updates the statistics nor checks whether an overflow flag
is set in the host frame.

Instead of directly resubmitting the URB, first update the dropped RX
statistics then check if an overflow flag is set.

Closes: https://sashiko.dev/#/bug/linux-7b3fe6a0-4628-4d0e-aec0-69f4bf8ad5d4
Fixes: 68c5724ecd15 ("can: gs_usb: gs_usb_receive_bulk_callback(): resubmit URB on skb allocation failure")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 2564c1672793..e3bf1c7cdd83 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -673,8 +673,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
 		if (hf->flags & GS_CAN_FLAG_FD) {
 			skb = alloc_canfd_skb(netdev, &cfd);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				netdev->stats.rx_dropped++;
+
+				goto check_overflow;
+			}
 
 			cfd->can_id = le32_to_cpu(hf->can_id);
 			cfd->len = data_length;
@@ -686,8 +689,11 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 			memcpy(cfd->data, hf->canfd->data, data_length);
 		} else {
 			skb = alloc_can_skb(netdev, &cf);
-			if (!skb)
-				goto resubmit_urb;
+			if (!skb) {
+				netdev->stats.rx_dropped++;
+
+				goto check_overflow;
+			}
 
 			cf->can_id = le32_to_cpu(hf->can_id);
 			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
@@ -728,6 +734,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		netif_wake_queue(netdev);
 	}
 
+check_overflow:
 	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
 		stats->rx_over_errors++;
 		stats->rx_errors++;

-- 
2.53.0

[PATCH v2 4/4] can: gs_usb: gs_usb_receive_bulk_callback(): add missing call to can_rx_offload_irq_finish

[Marc Kleine-Budde]

If the overflow flag is set for a host frame and the allocation of the
error SKB fails, the URB should not be resubmitted immediately.

Instead, call can_rx_offload_irq_finish(), since an SKB may have been added
to rx-offload.

Closes: https://sashiko.dev/#/bug/linux-441137e2-cd75-411f-9ff5-dc4d473c5ddb
Fixes: 24bc41b45583 ("can: gs_usb: convert to NAPI/rx-offload to avoid OoO reception")
Cc: stable@vger.kernel.org
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index e3bf1c7cdd83..83bf9a1ac0ac 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -741,7 +741,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 
 		skb = alloc_can_err_skb(netdev, &cf);
 		if (!skb)
-			goto resubmit_urb;
+			goto can_rx_offload_irq_finish;
 
 		cf->can_id |= CAN_ERR_CRTL;
 		cf->len = CAN_ERR_DLC;
@@ -750,6 +750,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
 		gs_usb_rx_offload(dev, skb, hf);
 	}
 
+can_rx_offload_irq_finish:
 	can_rx_offload_irq_finish(&dev->offload);
 
 resubmit_urb:

-- 
2.53.0