[PATCH v2 1/3] usb: core: Fix use-after-free in anchor wakeup protocol
From: Hongyu Xie
Date: Mon Nov 02 2026 - 21:21:56 EST
usb_wait_anchor_empty_timeout() and the anchor cleanup helpers
promise that once they return successfully (non-zero, for
usb_wait_anchor_empty_timeout()), no URB giveback is in flight for
the anchor any more, so the driver may free the memory holding it
(xpad, for example, embeds irq_out_anchor in its private structure
and kfrees it in xpad_disconnect()).
Two windows break this promise: the waiter can observe "anchor
list empty and suspend_wakeups == 0" while a giveback is still
about to access anchor->wait.
1. usb_anchor_resume_wakeups() decrements suspend_wakeups -- which
makes the wait condition true -- and only then calls
wake_up(&anchor->wait), with nothing in between serializing the
two. A waiter returns, the driver frees the anchor, and the
late wake_up() touches the freed wait queue head.
2. __usb_unanchor_urb() wakes waiters while holding anchor->lock,
but usb_wait_anchor_empty_timeout() evaluated the same condition
without the lock, so it could observe the emptied urb_list in
the middle of that critical section and return before the
wake_up() and the spin_unlock() had run. Reading the two
condition fields locklessly could likewise observe a stale
suspend_wakeups value after the giveback's unanchor on weakly
ordered systems.
Complete the locking protocol that commit fbc299437c06 ("usb:
core: Solve race condition in anchor cleanup functions")
established for the kill/poison/scuttle helpers:
usb_wait_anchor_empty_timeout() now evaluates the condition under
anchor->lock (a self-locking wrapper around the lockless check),
and usb_anchor_resume_wakeups() performs the suspend_wakeups
decrement, the wakeup check and the wake_up() within a single
anchor->lock critical section.
With the decrement and the wake_up() atomic with respect to the
locked condition check, a waiter can only observe the condition
true after every giveback that has incremented suspend_wakeups has
stopped accessing the anchor: the guarantee that the wakeup is
delayed until the completion handler has run now extends to the
anchor memory itself.
syzbot has reported this race repeatedly since 2019, most
recently as:
BUG: KASAN: slab-use-after-free in register_lock_class+0x8d1/0x980 kernel/locking/lockdep.c:1333
[...]
__wake_up_common_lock+0x25/0x1e0 kernel/sched/wait.c:105
__usb_hcd_giveback_urb+0x3ac/0x530 drivers/usb/core/hcd.c:1653
Freed by task 7828:
[...]
kfree+0x14a/0x380 mm/slub.c:4390
xpad_disconnect+0x359/0x490 drivers/input/joystick/xpad.c:2194
and:
BUG: KASAN: use-after-free in register_lock_class+0xe6a/0x1120 kernel/locking/lockdep.c:1336
[...]
__wake_up_common_lock+0xb4/0x130 kernel/sched/wait.c:136
usb_anchor_resume_wakeups drivers/usb/core/urb.c:958 [inline]
usb_anchor_resume_wakeups+0xbe/0xe0 drivers/usb/core/urb.c:951
__usb_hcd_giveback_urb+0x2df/0x5c0 drivers/usb/core/hcd.c:1674
Freed by task 3889:
[...]
kfree+0x173/0x390 mm/slab.c:3786
xpad_disconnect+0x1cb/0x530 drivers/input/joystick/xpad.c:1905
Both reports lack a reproducer; the analysis was verified
by walking the two stacks against the code, not at runtime. All
in-tree callers of usb_wait_anchor_empty_timeout() (and every
driver which frees an anchor after the anchor cleanup helpers) are
equally affected; xpad is merely the one syzbot found.
Fixes: 6ec4147e7bdb ("usb-anchor: Delay usb_wait_anchor_empty_timeout wake up till completion is done")
Fixes: 51a2f077c44e ("USB: introduce usb_anchor")
Reported-by: syzbot+7f5e98b052c766f47f68@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://lore.kernel.org/r/00000000000090170a0614431f47@xxxxxxxxxx
Reported-by: syzbot+59d3714b44d79d02f944@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://lore.kernel.org/r/00000000000038435e05e9f94cd4@xxxxxxxxxx
Cc: stable@xxxxxxxxxxxxxxx
Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
Cc: Oliver Neukum <oliver@xxxxxxxxxx>
Cc: Hans de Goede <hansg@xxxxxxxxxx>
Signed-off-by: Hongyu Xie <xiehongyu1@xxxxxxxxxx>
---
drivers/usb/core/urb.c | 51 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 45 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index c06b44ca507b..3afd184c9b32 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -139,19 +139,39 @@ void usb_anchor_urb(struct urb *urb, struct usb_anchor *anchor)
}
EXPORT_SYMBOL_GPL(usb_anchor_urb);
-static int usb_anchor_check_wakeup(struct usb_anchor *anchor)
+/* Callers must hold anchor->lock */
+static int __usb_anchor_check_wakeup(struct usb_anchor *anchor)
{
return atomic_read(&anchor->suspend_wakeups) == 0 &&
list_empty(&anchor->urb_list);
}
+/*
+ * Same check as __usb_anchor_check_wakeup(), but takes anchor->lock. It is
+ * meant for waiters which do not hold the lock: the critical section
+ * serializes with the URB giveback path, so once the check succeeds no
+ * giveback that has incremented suspend_wakeups will touch the anchor
+ * anymore and the caller may free the memory holding the anchor.
+ */
+static int usb_anchor_check_wakeup(struct usb_anchor *anchor)
+{
+ unsigned long flags;
+ int wakeup;
+
+ spin_lock_irqsave(&anchor->lock, flags);
+ wakeup = __usb_anchor_check_wakeup(anchor);
+ spin_unlock_irqrestore(&anchor->lock, flags);
+
+ return wakeup;
+}
+
/* Callers must hold anchor->lock */
static void __usb_unanchor_urb(struct urb *urb, struct usb_anchor *anchor)
{
urb->anchor = NULL;
list_del(&urb->anchor_list);
usb_put_urb(urb);
- if (usb_anchor_check_wakeup(anchor))
+ if (__usb_anchor_check_wakeup(anchor))
wake_up(&anchor->wait);
}
@@ -827,7 +847,7 @@ void usb_kill_anchored_urbs(struct usb_anchor *anchor)
usb_put_urb(victim);
spin_lock_irq(&anchor->lock);
}
- surely_empty = usb_anchor_check_wakeup(anchor);
+ surely_empty = __usb_anchor_check_wakeup(anchor);
spin_unlock_irq(&anchor->lock);
cpu_relax();
@@ -866,7 +886,7 @@ void usb_poison_anchored_urbs(struct usb_anchor *anchor)
usb_put_urb(victim);
spin_lock_irq(&anchor->lock);
}
- surely_empty = usb_anchor_check_wakeup(anchor);
+ surely_empty = __usb_anchor_check_wakeup(anchor);
spin_unlock_irq(&anchor->lock);
cpu_relax();
@@ -919,12 +939,23 @@ EXPORT_SYMBOL_GPL(usb_anchor_suspend_wakeups);
*/
void usb_anchor_resume_wakeups(struct usb_anchor *anchor)
{
+ unsigned long flags;
+
if (!anchor)
return;
+ /*
+ * The decrement, the wakeup check and the wake_up() must happen
+ * under anchor->lock: usb_wait_anchor_empty_timeout() waiters
+ * evaluate the same condition under the lock and may free the
+ * anchor once they observe it true, so this function must not
+ * touch anchor->wait after that.
+ */
+ spin_lock_irqsave(&anchor->lock, flags);
atomic_dec(&anchor->suspend_wakeups);
- if (usb_anchor_check_wakeup(anchor))
+ if (__usb_anchor_check_wakeup(anchor))
wake_up(&anchor->wait);
+ spin_unlock_irqrestore(&anchor->lock, flags);
}
EXPORT_SYMBOL_GPL(usb_anchor_resume_wakeups);
@@ -941,6 +972,14 @@ EXPORT_SYMBOL_GPL(usb_anchor_resume_wakeups);
int usb_wait_anchor_empty_timeout(struct usb_anchor *anchor,
unsigned int timeout)
{
+ /*
+ * The condition must be evaluated under anchor->lock: when this
+ * function returns non-zero, every URB giveback that has incremented
+ * suspend_wakeups has finished with the anchor, so the caller may
+ * free the anchor, which is typically embedded in the driver's
+ * private structure. A zero return (timeout) means a giveback may
+ * still be in flight and gives no such guarantee.
+ */
return wait_event_timeout(anchor->wait,
usb_anchor_check_wakeup(anchor),
msecs_to_jiffies(timeout));
@@ -997,7 +1036,7 @@ void usb_scuttle_anchored_urbs(struct usb_anchor *anchor)
struct urb, anchor_list);
__usb_unanchor_urb(victim, anchor);
}
- surely_empty = usb_anchor_check_wakeup(anchor);
+ surely_empty = __usb_anchor_check_wakeup(anchor);
spin_unlock_irqrestore(&anchor->lock, flags);
cpu_relax();
base-commit: 22430ae5d90ab288b0ee2ad99ae941f4a666b694
--
2.32.0