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