[PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[Myeonghun Pak]

rtl8150_disconnect() kills the RX tasklet before killing the URBs.
Endpoint flushing does not wait for completion handlers: a
read_bulk_callback() that passed the unplug check can still schedule
the tasklet after the first kill. During driver unbind, disabled
endpoints make RX resubmissions fail, so rx_fixup() can keep
rescheduling itself and access freed driver data after free_netdev().

Kill the tasklet again after unlink_all_urbs() has waited for all
completion handlers. tasklet_kill() drains any late tasklet and prevents
self-rescheduling; disabled endpoints prevent it from leaving an RX
URB in flight. Keep the first kill to stop an existing reschedule loop
before unregister_netdev().

Found by a static analysis tool under development.

Fixes: eff674a514bd ("USB: rtl8150_disconnect() needs tasklet_kill()")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---

 drivers/net/usb/rtl8150.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index d51e43170e03..98c36c377956 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -971,6 +971,8 @@ static void rtl8150_disconnect(struct usb_interface *intf)
 		tasklet_kill(&dev->tl);
 		unregister_netdev(dev->netdev);
 		unlink_all_urbs(dev);
+		/* A completion may have scheduled it after the first kill. */
+		tasklet_kill(&dev->tl);
 		free_all_urbs(dev);
 		free_skb_pool(dev);
 		dev_kfree_skb(dev->rx_skb);

base-commit: 0edbc945eb9edd0086f25b151a79cfe4adc0dfa8
-- 
2.43.0

Re: [PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[Oliver Neukum]

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

On 10.11.26 05:30, Myeonghun Pak wrote:

Hi,

> Kill the tasklet again after unlink_all_urbs() has waited for all
> completion handlers. tasklet_kill() drains any late tasklet and prevents
> self-rescheduling; disabled endpoints prevent it from leaving an RX
> URB in flight. Keep the first kill to stop an existing reschedule loop
> before unregister_netdev().
> 
> Found by a static analysis tool under development.

Good catch; nice tool.
But the fix is not correct.

> Fixes: eff674a514bd ("USB: rtl8150_disconnect() needs tasklet_kill()")
> Cc: stable@vger.kernel.org
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> 
> ---
> 
>   drivers/net/usb/rtl8150.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index d51e43170e03..98c36c377956 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -971,6 +971,8 @@ static void rtl8150_disconnect(struct usb_interface *intf)
>   		tasklet_kill(&dev->tl);
>   		unregister_netdev(dev->netdev);
>   		unlink_all_urbs(dev);
> +		/* A completion may have scheduled it after the first kill. */
> +		tasklet_kill(&dev->tl);

The tasklet, however, can also submit the URB again, which in turn
can submit the tasklet and so on. There is an unavoidable window
in that sequence, as tasklet_kill() will kill the tasklet, but it may
have run in the window.

Hence you need to also, in addition to your patch, actually prevent the
resubmission of the URBs. The attached fix will do so.

How do we do this? Shall I submit? Do you want a joint submission?

	Regards
		Oliver

[-- Attachment #2: 0001-net-usb-rtl8150-break-cycle-between-tasklet-and-URB.patch --]
[-- Type: text/x-patch, Size: 1907 bytes --]

From c8b88d45caef0c2fdfa0b1a51f77c2db567658bf Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 10 Nov 2026 11:26:34 +0100
Subject: [PATCH] net: usb: rtl8150: break cycle between tasklet and URB

The tasklet can resubmit the URB, which in turn can
resubmit the tasklet. The cycle needs to be broken
the simplest way to do so is to prevent the resubmission
of the URBs by poisoning them.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/rtl8150.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index d51e43170e03..635e65a0de43 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -353,11 +353,18 @@ static void free_all_urbs(rtl8150_t * dev)
 	usb_free_urb(dev->intr_urb);
 }
 
+static void reenable_all_urbs(rtl8150_t * dev)
+{
+	usb_unpoison_urb(dev->rx_urb);
+	usb_unpoison_urb(dev->tx_urb);
+	usb_unpoison_urb(dev->intr_urb);
+}
+
 static void unlink_all_urbs(rtl8150_t * dev)
 {
-	usb_kill_urb(dev->rx_urb);
-	usb_kill_urb(dev->tx_urb);
-	usb_kill_urb(dev->intr_urb);
+	usb_poison_urb(dev->rx_urb);
+	usb_poison_urb(dev->tx_urb);
+	usb_poison_urb(dev->intr_urb);
 }
 
 static inline struct sk_buff *pull_skb(rtl8150_t *dev)
@@ -518,7 +525,7 @@ static void intr_callback(struct urb *urb)
 	res = usb_submit_urb (urb, GFP_ATOMIC);
 	if (res == -ENODEV)
 		netif_device_detach(dev->netdev);
-	else if (res)
+	else if (res != -EPERM)
 		dev_err(&dev->udev->dev,
 			"can't resubmit intr, %s-%s/input0, status %d\n",
 			dev->udev->bus->bus_name, dev->udev->devpath, res);
@@ -751,6 +758,7 @@ static int rtl8150_open(struct net_device *netdev)
 	if (!dev->rx_skb)
 		return -ENOMEM;
 
+	reenable_all_urbs(dev);
 	set_registers(dev, IDR, 6, netdev->dev_addr);
 
 	usb_fill_bulk_urb(dev->rx_urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
-- 
2.56.0

Re: [PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (2) · Medium: 1 · 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).

- [Medium] The patch's new tasklet_kill() calls in rtl8150_disconnect()
  cannot reliably stop rx_fixup() when it keeps rescheduling itself, and…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the stale
  dev->rx_skb pointer can lead to a double free or a device DMA into…
- [High] This problem wasn't introduced by this patch, but pegasus.c, the
  driver rtl8150 was derived from, has the same late-completion tasklet…

--

net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

This adds a second tasklet_kill() in rtl8150_disconnect(), placed after
unlink_all_urbs(). It is meant to drain an RX tasklet that a late
read_bulk_callback() could have scheduled after the first kill, before the
driver data is freed.

> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index d51e43170e033..98c36c3779564 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -971,6 +971,8 @@ static void rtl8150_disconnect(struct usb_interface *intf)
>  		tasklet_kill(&dev->tl);
>  		unregister_netdev(dev->netdev);
>  		unlink_all_urbs(dev);
> +		/* A completion may have scheduled it after the first kill. */
> +		tasklet_kill(&dev->tl);

[Severity: Medium]
Can this tasklet_kill() actually end the rx_fixup() reschedule loop that the
commit message describes?

The commit message says "disabled endpoints make RX resubmissions fail, so
rx_fixup() can keep rescheduling itself". It then says "tasklet_kill()
drains any late tasklet and prevents self-rescheduling".

rtl8150_driver has no soft_unbind, so usb_unbind_interface() disables the
endpoints before calling ->disconnect(). The device is still attached, so
usb_submit_urb() passes its -ENODEV check. usb_hcd_link_urb_to_ep() then
returns -ENOENT, or -EPERM while usb_kill_urb() is in progress:

drivers/usb/core/hcd.c:usb_hcd_link_urb_to_ep() {
    ...
	if (unlikely(!urb->ep->enabled)) {
		rc = -ENOENT;
		goto done;
	}
    ...
}

In rx_fixup(), -ENODEV is the only error that skips the reschedule, and
nothing checks RTL8150_UNPLUG:

drivers/net/usb/rtl8150.c:rx_fixup() {
    ...
try_again:
	status = usb_submit_urb(dev->rx_urb, GFP_ATOMIC);
	if (status == -ENODEV) {
		netif_device_detach(dev->netdev);
	} else if (status) {
		set_bit(RX_URB_FAIL, &dev->flags);
		goto tlsched;
	}
    ...
tlsched:
	tasklet_schedule(&dev->tl);
}

tasklet_kill() only returns once the waiter sets TASKLET_STATE_SCHED
itself:

	wait_on_bit_lock(&t->state, TASKLET_STATE_SCHED, TASK_UNINTERRUPTIBLE);

tasklet_action_common() clears TASKLET_STATE_SCHED and wakes the waiter
just before it calls t->callback(t). rx_fixup() then sets the bit again
before it returns. All of this runs in non-preemptible softirq context.

On a CONFIG_SMP=n non-RT kernel, or when the waiter runs on the same CPU as
the tasklet, process context would only ever see TASKLET_STATE_SCHED set.
Wouldn't rtl8150_disconnect() then sleep in TASK_UNINTERRUPTIBLE forever
while ksoftirqd keeps running rx_fixup()?

The first tasklet_kill() already had this limitation. The new call relies
on the same assumption to close the use-after-free.

For tasklet_kill() to be reliable here, would rx_fixup() and the resched
path in read_bulk_callback() need to stop resubmitting and rescheduling
once RTL8150_UNPLUG is set?

>  		free_all_urbs(dev);
>  		free_skb_pool(dev);
>  		dev_kfree_skb(dev->rx_skb);

[Severity: High]
This is a pre-existing issue, but can dev->rx_skb still be stale when it is
freed here?

read_bulk_callback() passes dev->rx_skb to the stack, and if the pool is
empty it leaves the pointer unchanged:

	netif_rx(dev->rx_skb);
	...
	skb = pull_skb(dev);
	spin_unlock_irqrestore(&dev->rx_pool_lock, flags);
	if (!skb)
		goto resched;

	dev->rx_skb = skb;

The pool has only RX_SKB_POOL_SIZE (4) entries, and only rx_fixup() refills
it. rx_fixup() is also the only place that replaces the pointer. If every
GFP_ATOMIC allocation in fill_skb_pool() fails, it just reschedules:

	skb = pull_skb(dev);
	spin_unlock_irq(&dev->rx_pool_lock);
	if (skb == NULL)
		goto tlsched;
	dev->rx_skb = skb;

Suppose rtl8150_disconnect() runs in that state. The tasklet_kill() calls
stop the loop but leave the pointer as it is. Would dev_kfree_skb(dev->rx_skb)
then double free an skb that the network stack already consumed?

Something similar looks possible across close and open. rtl8150_close()
does not kill the tasklet or clear rx_skb. rtl8150_open() then reuses a
non-NULL rx_skb as the bulk-in buffer:

	if (dev->rx_skb == NULL)
		dev->rx_skb = pull_skb(dev);
	...
	usb_fill_bulk_urb(dev->rx_urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
		      dev->rx_skb->data, RTL8150_MTU, read_bulk_callback, dev);

Could the device then DMA packet data into freed memory?

[Severity: High]
This isn't a bug introduced by this patch, but pegasus.c, which rtl8150 was
derived from, seems to have the same late-completion tasklet problem. This
patch doesn't change it.

In pegasus read_bulk_callback(), when the resubmit fails, rx_tl is scheduled
without checking PEGASUS_UNPLUG again:

	else if (rx_status) {
		pegasus->flags |= PEGASUS_RX_URB_FAIL;
		goto tl_sched;
	}
	...
tl_sched:
	tasklet_schedule(&pegasus->rx_tl);

During unbind the endpoints are already disabled, so the resubmit fails with
-ENOENT. It fails with -EPERM once usb_kill_urb() has set urb->reject.

The only tasklet_kill() is in pegasus_close(), and it runs before the URBs
are killed:

	tasklet_kill(&pegasus->rx_tl);
	unlink_all_urbs(pegasus);

pegasus_disconnect() then goes from unlink_all_urbs() to free_netdev()
without draining the tasklet:

	unregister_netdev(pegasus->net);
	unlink_all_urbs(pegasus);
	free_all_urbs(pegasus);
	...
	free_netdev(pegasus->net);

Consider a completion that passes the netif_running() check before
__dev_close_many() clears __LINK_STATE_START. If it reaches
tasklet_schedule() after the tasklet_kill() in pegasus_close(), rx_tl is
left pending with no later kill.

rx_fixup() returns early on PEGASUS_UNPLUG, so there is no endless loop.
It still reads pegasus->flags, though, and tasklet_action_common() touches
the tasklet_struct inside netdev_priv(). Could either of those run after
free_netdev()?

Does pegasus_disconnect() need a tasklet_kill(&pegasus->rx_tl) after
unlink_all_urbs() too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110043016.56986-1-mhun512%40gmail.com

Re: [PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[Myeonghun Pak]

Hi Oliver,

Thanks for reviewing the patch and preparing the additional fix.
Please go ahead and submit it. I appreciate your help with addressing
the URB resubmission issue.

Thanks,
Myeonghun

2026년 11월 11일 (수) 오후 11:31, <netdev-bot+sashiko@kernel.org>님이 작성:
>
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 (2) · Medium: 1 · 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).
>
> - [Medium] The patch's new tasklet_kill() calls in rtl8150_disconnect()
>   cannot reliably stop rx_fixup() when it keeps rescheduling itself, and…
>
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the stale
>   dev->rx_skb pointer can lead to a double free or a device DMA into…
> - [High] This problem wasn't introduced by this patch, but pegasus.c, the
>   driver rtl8150 was derived from, has the same late-completion tasklet…
>
> --
>
> net: usb: rtl8150: fix rx tasklet use-after-free on disconnect
>
> This adds a second tasklet_kill() in rtl8150_disconnect(), placed after
> unlink_all_urbs(). It is meant to drain an RX tasklet that a late
> read_bulk_callback() could have scheduled after the first kill, before the
> driver data is freed.
>
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index d51e43170e033..98c36c3779564 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -971,6 +971,8 @@ static void rtl8150_disconnect(struct usb_interface *intf)
> >               tasklet_kill(&dev->tl);
> >               unregister_netdev(dev->netdev);
> >               unlink_all_urbs(dev);
> > +             /* A completion may have scheduled it after the first kill. */
> > +             tasklet_kill(&dev->tl);
>
> [Severity: Medium]
> Can this tasklet_kill() actually end the rx_fixup() reschedule loop that the
> commit message describes?
>
> The commit message says "disabled endpoints make RX resubmissions fail, so
> rx_fixup() can keep rescheduling itself". It then says "tasklet_kill()
> drains any late tasklet and prevents self-rescheduling".
>
> rtl8150_driver has no soft_unbind, so usb_unbind_interface() disables the
> endpoints before calling ->disconnect(). The device is still attached, so
> usb_submit_urb() passes its -ENODEV check. usb_hcd_link_urb_to_ep() then
> returns -ENOENT, or -EPERM while usb_kill_urb() is in progress:
>
> drivers/usb/core/hcd.c:usb_hcd_link_urb_to_ep() {
>     ...
>         if (unlikely(!urb->ep->enabled)) {
>                 rc = -ENOENT;
>                 goto done;
>         }
>     ...
> }
>
> In rx_fixup(), -ENODEV is the only error that skips the reschedule, and
> nothing checks RTL8150_UNPLUG:
>
> drivers/net/usb/rtl8150.c:rx_fixup() {
>     ...
> try_again:
>         status = usb_submit_urb(dev->rx_urb, GFP_ATOMIC);
>         if (status == -ENODEV) {
>                 netif_device_detach(dev->netdev);
>         } else if (status) {
>                 set_bit(RX_URB_FAIL, &dev->flags);
>                 goto tlsched;
>         }
>     ...
> tlsched:
>         tasklet_schedule(&dev->tl);
> }
>
> tasklet_kill() only returns once the waiter sets TASKLET_STATE_SCHED
> itself:
>
>         wait_on_bit_lock(&t->state, TASKLET_STATE_SCHED, TASK_UNINTERRUPTIBLE);
>
> tasklet_action_common() clears TASKLET_STATE_SCHED and wakes the waiter
> just before it calls t->callback(t). rx_fixup() then sets the bit again
> before it returns. All of this runs in non-preemptible softirq context.
>
> On a CONFIG_SMP=n non-RT kernel, or when the waiter runs on the same CPU as
> the tasklet, process context would only ever see TASKLET_STATE_SCHED set.
> Wouldn't rtl8150_disconnect() then sleep in TASK_UNINTERRUPTIBLE forever
> while ksoftirqd keeps running rx_fixup()?
>
> The first tasklet_kill() already had this limitation. The new call relies
> on the same assumption to close the use-after-free.
>
> For tasklet_kill() to be reliable here, would rx_fixup() and the resched
> path in read_bulk_callback() need to stop resubmitting and rescheduling
> once RTL8150_UNPLUG is set?
>
> >               free_all_urbs(dev);
> >               free_skb_pool(dev);
> >               dev_kfree_skb(dev->rx_skb);
>
> [Severity: High]
> This is a pre-existing issue, but can dev->rx_skb still be stale when it is
> freed here?
>
> read_bulk_callback() passes dev->rx_skb to the stack, and if the pool is
> empty it leaves the pointer unchanged:
>
>         netif_rx(dev->rx_skb);
>         ...
>         skb = pull_skb(dev);
>         spin_unlock_irqrestore(&dev->rx_pool_lock, flags);
>         if (!skb)
>                 goto resched;
>
>         dev->rx_skb = skb;
>
> The pool has only RX_SKB_POOL_SIZE (4) entries, and only rx_fixup() refills
> it. rx_fixup() is also the only place that replaces the pointer. If every
> GFP_ATOMIC allocation in fill_skb_pool() fails, it just reschedules:
>
>         skb = pull_skb(dev);
>         spin_unlock_irq(&dev->rx_pool_lock);
>         if (skb == NULL)
>                 goto tlsched;
>         dev->rx_skb = skb;
>
> Suppose rtl8150_disconnect() runs in that state. The tasklet_kill() calls
> stop the loop but leave the pointer as it is. Would dev_kfree_skb(dev->rx_skb)
> then double free an skb that the network stack already consumed?
>
> Something similar looks possible across close and open. rtl8150_close()
> does not kill the tasklet or clear rx_skb. rtl8150_open() then reuses a
> non-NULL rx_skb as the bulk-in buffer:
>
>         if (dev->rx_skb == NULL)
>                 dev->rx_skb = pull_skb(dev);
>         ...
>         usb_fill_bulk_urb(dev->rx_urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
>                       dev->rx_skb->data, RTL8150_MTU, read_bulk_callback, dev);
>
> Could the device then DMA packet data into freed memory?
>
> [Severity: High]
> This isn't a bug introduced by this patch, but pegasus.c, which rtl8150 was
> derived from, seems to have the same late-completion tasklet problem. This
> patch doesn't change it.
>
> In pegasus read_bulk_callback(), when the resubmit fails, rx_tl is scheduled
> without checking PEGASUS_UNPLUG again:
>
>         else if (rx_status) {
>                 pegasus->flags |= PEGASUS_RX_URB_FAIL;
>                 goto tl_sched;
>         }
>         ...
> tl_sched:
>         tasklet_schedule(&pegasus->rx_tl);
>
> During unbind the endpoints are already disabled, so the resubmit fails with
> -ENOENT. It fails with -EPERM once usb_kill_urb() has set urb->reject.
>
> The only tasklet_kill() is in pegasus_close(), and it runs before the URBs
> are killed:
>
>         tasklet_kill(&pegasus->rx_tl);
>         unlink_all_urbs(pegasus);
>
> pegasus_disconnect() then goes from unlink_all_urbs() to free_netdev()
> without draining the tasklet:
>
>         unregister_netdev(pegasus->net);
>         unlink_all_urbs(pegasus);
>         free_all_urbs(pegasus);
>         ...
>         free_netdev(pegasus->net);
>
> Consider a completion that passes the netif_running() check before
> __dev_close_many() clears __LINK_STATE_START. If it reaches
> tasklet_schedule() after the tasklet_kill() in pegasus_close(), rx_tl is
> left pending with no later kill.
>
> rx_fixup() returns early on PEGASUS_UNPLUG, so there is no endless loop.
> It still reads pegasus->flags, though, and tasklet_action_common() touches
> the tasklet_struct inside netdev_priv(). Could either of those run after
> free_netdev()?
>
> Does pegasus_disconnect() need a tasklet_kill(&pegasus->rx_tl) after
> unlink_all_urbs() too?
>
> --
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110043016.56986-1-mhun512%40gmail.com