wifi: rtl8xxxu: fix use-after-free from rx_urb_wq on stop
Subsystems (2)
PATCH 1/1 wifi: rtl8xxxu: fix use-after-free from rx_urb_wq on stop
rtl8xxxu arms rx_urb_wq from the RX completion path:rtl8xxxu_rx_complete() hands the URB to rtl8xxxu_queue_rx_urb(), which queues it on rx_urb_pending_list and, once the list grows past RTL8XXXU_RX_URB_PENDING_WATER, schedules rx_urb_wq. The worker rtl8xxxu_rx_urb_work() drains rx_urb_pending_list, recovers priv through container_of, and resubmits each URB through rtl8xxxu_submit_rx_urb(), which anchors it on rx_anchor and dereferences priv->udev.
rtl8xxxu_stop() cancels the sibling work items (c2hcmd_work, ra_watchdog, update_beacon_work) but never cancels rx_urb_wq, so a worker armed during the last burst of RX traffic can run rtl8xxxu_rx_urb_work() after rtl8xxxu_disconnect() has called ieee80211_free_hw(), which frees priv, producing a use-after-free. The window opens under active RX traffic (pending count above the watermark) followed by a disconnect.
There are two teardown races to close:
* rtl8xxxu_queue_rx_urb() decided whether to enqueue under rx_urb_lock but called schedule_work() after dropping the lock. A completion that observed shutdown == false and released the lock could then call schedule_work() after rtl8xxxu_stop() had set shutdown and cancel_work_sync() had already returned, arming the worker to run after the teardown. Move schedule_work() under the same !shutdown branch so the arming decision is atomic with the shutdown check.
* rtl8xxxu_rx_urb_work() anchors every URB it drained back onto rx_anchor through rtl8xxxu_submit_rx_urb(). A worker still running when usb_kill_anchored_urbs(&priv->rx_anchor) returned would submit a URB that escaped the kill. In rtl8xxxu_stop(), call cancel_work_sync(&priv->rx_urb_wq) before the kill so the worker is drained first.
After priv->shutdown is set under rx_urb_lock, completions can no longer queue rx_urb_wq. cancel_work_sync() then drains the last queued or running worker, and the following usb_kill_anchored_urbs() kills the URBs it may have submitted.
rtl8xxxu_disconnect() is covered because ieee80211_unregister_hw() guarantees .stop() runs for a live interface before ieee80211_free_hw() frees priv. The probe error path needs no cancel: rx_urb_wq is INIT_WORK()'d there but cannot have been scheduled, since no URB is submitted before ieee80211_register_hw() succeeds.
This bug was found by static analysis.
--- drivers/net/wireless/realtek/rtl8xxxu/core.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)
drivers/net/wireless/realtek/rtl8xxxu/core.c +14 -5
@@ -5792,14 +5792,19 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, {struct sk_buff *skb;unsigned long flags; -int pending =0 ;spin_lock_irqsave (&priv->rx_urb_lock, flags);if (!priv->shutdown) {list_add_tail (&rx_urb->list, &priv->rx_urb_pending_list); priv->rx_urb_pending_count++; - pending = priv->rx_urb_pending_count; +/* +* Arm the worker under rx_urb_lock so this is atomic with the +* shutdown check: moving it out of the lock would let a +* completion arm the work after rtl8xxxu_stop() canceled it. +*/ +if (priv->rx_urb_pending_count >RTL8XXXU_RX_URB_PENDING_WATER ) +schedule_work (&priv->rx_urb_wq); }else { skb = (struct sk_buff *)rx_urb->urb.context;dev_kfree_skb_irq (skb);
@@ -5807,9 +5812,6 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, }spin_unlock_irqrestore (&priv->rx_urb_lock, flags); - -if (pending >RTL8XXXU_RX_URB_PENDING_WATER ) -schedule_work (&priv->rx_urb_wq); }static void rtl8xxxu_rx_urb_work (struct work_struct *work)
@@ -7461,6 +7463,13 @@ static void rtl8xxxu_stop(struct ieee80211_hw *hw, bool suspend) priv->shutdown = true;spin_unlock_irqrestore (&priv->rx_urb_lock, flags); +/* +* Cancel before killing rx_anchor: the worker re-anchors every URB +* it drained via rtl8xxxu_submit_rx_urb(), so a worker still running +* after the kill could submit a URB that escapes it. +*/ +cancel_work_sync (&priv->rx_urb_wq); +usb_kill_anchored_urbs (&priv->rx_anchor);usb_kill_anchored_urbs (&priv->tx_anchor);if (priv->usb_interrupts)
Signature · 1 line
2.34.1
Quoted 43 lines · Fan Wu
rtl8xxxu arms
rx_urb_wqfrom the RX completion path:rtl8xxxu_rx_complete()hands the URB tortl8xxxu_queue_rx_urb(), which queues it onrx_urb_pending_listand, once the list grows pastRTL8XXXU_RX_URB_PENDING_WATER, schedulesrx_urb_wq. The workerrtl8xxxu_rx_urb_work()drainsrx_urb_pending_list, recovers priv throughcontainer_of, and resubmits each URB throughrtl8xxxu_submit_rx_urb(), which anchors it onrx_anchorand dereferencespriv->udev.
rtl8xxxu_stop()cancels the sibling work items (c2hcmd_work,ra_watchdog,update_beacon_work) but never cancelsrx_urb_wq, so a worker armed during the last burst of RX traffic can runrtl8xxxu_rx_urb_work()afterrtl8xxxu_disconnect()has calledieee80211_free_hw(), which frees priv, producing a use-after-free. The window opens under active RX traffic (pending count above the watermark) followed by a disconnect.There are two teardown races to close:
*
rtl8xxxu_queue_rx_urb()decided whether to enqueue underrx_urb_lockbut calledschedule_work()after dropping the lock. A completion that observed shutdown == false and released the lock could then callschedule_work()afterrtl8xxxu_stop()had set shutdown andcancel_work_sync()had already returned, arming the worker to run after the teardown. Moveschedule_work()under the same !shutdown branch so the arming decision is atomic with the shutdown check.*
rtl8xxxu_rx_urb_work()anchors every URB it drained back ontorx_anchorthroughrtl8xxxu_submit_rx_urb(). A worker still running whenusb_kill_anchored_urbs(&priv->rx_anchor)returned would submit a URB that escaped the kill. Inrtl8xxxu_stop(), callcancel_work_sync(&priv->rx_urb_wq)before the kill so the worker is drained first.After
priv->shutdownis set underrx_urb_lock, completions can no longer queuerx_urb_wq.cancel_work_sync()then drains the last queued or running worker, and the followingusb_kill_anchored_urbs()kills the URBs it may have submitted.
rtl8xxxu_disconnect()is covered becauseieee80211_unregister_hw()guarantees .stop() runs for a live interface beforeieee80211_free_hw()frees priv. The probe error path needs no cancel:rx_urb_wqis INIT_WORK()'d there but cannot have been scheduled, since no URB is submitted beforeieee80211_register_hw()succeeds.
Quoted 5 lines
This bug was found by static analysis.
Fixes 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)")Signed-off-by Fan Wu <[email protected]>
1 patch(es) applied to rtw-next branch of rtw.git, thanks.
6c080026ecc1 wifi: rtl8xxxu: fix use-after-free from rx_urb_wq on stop