Re: [PATCH] wifi: rtl8xxxu: fix use-after-free from rx_urb_wq on stop

[Ping-Ke Shih]

Fan Wu <fanwu01@xxxxxxxxxx> wrote:

> 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.
> 
> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Fan Wu <fanwu01@xxxxxxxxxx>

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

---
https://github.com/pkshih/rtw.git