* [PATCH 2/2] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work
2026-11-11 3:20 [PATCH 1/2] HID: nintendo-switch2-usb: Fix potential deadlock on teardown Vicki Pfau
@ 2026-11-11 3:20 ` Vicki Pfau
2026-11-11 3:37 ` sashiko-bot
2026-11-11 3:37 ` [PATCH 1/2] HID: nintendo-switch2-usb: Fix potential deadlock on teardown sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Vicki Pfau @ 2026-11-11 3:20 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input; +Cc: Vicki Pfau, Noah Weinberger
When detaching the USB interface the driver marks the interface as shutdown
to prevent work that may be executing in parallel from attempting to use
the URBs before freeing the URBs. There is theoretically a very narrow
window in which the work could be scheduled while the shutdown is running
in a separate thread, starting to execute before the shutdown cancels the
work.
By ensuring the scheduling happens within the locked scope and adding an
additional shutdown check, this closes the window entirely.
Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
Link: https://lore.kernel.org/linux-input/a2dc00e7-9cab-4315-87a4-795f256c1e03@endrift.com/T/
Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")
Signed-off-by: Vicki Pfau <vi@endrift.com>
---
drivers/hid/hid-nintendo-switch2-usb.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index d747000336dc..5dba25c738e1 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -52,12 +52,10 @@ static void switch2_bulk_in(struct urb *urb)
{
struct switch2_usb *ns2_usb = urb->context;
int i;
- bool schedule = false;
unsigned long flags;
switch (urb->status) {
case 0:
- schedule = true;
break;
case -ECONNRESET:
case -ENOENT:
@@ -71,16 +69,14 @@ static void switch2_bulk_in(struct urb *urb)
}
spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
- if (ns2_usb->shutdown)
- schedule = false;
-
for (i = 0; i < NS2_IN_URBS; i++) {
int err;
struct switch2_urb *ns2_urb;
if (ns2_usb->bulk_in[i].urb == urb) {
- if (schedule) {
+ if (urb->status == 0 && !ns2_usb->shutdown) {
ns2_usb->bulk_in[i].state = NS2_URB_IN;
+ schedule_work(&ns2_usb->message_in_work);
continue;
} else {
ns2_usb->bulk_in[i].state = NS2_URB_FREE;
@@ -115,9 +111,6 @@ static void switch2_bulk_in(struct urb *urb)
}
}
spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
-
- if (schedule)
- schedule_work(&ns2_usb->message_in_work);
}
static void switch2_bulk_out(struct urb *urb)
@@ -294,6 +287,10 @@ static void switch2_usb_message_in_work(struct work_struct *work)
unsigned long flags;
spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
+ if (ns2_usb->shutdown) {
+ spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
+ return;
+ }
for (i = 0; i < NS2_IN_URBS; i++) {
urb = &ns2_usb->bulk_in[i];
if (urb->state != NS2_URB_IN)
--
2.54.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] HID: nintendo-switch2-usb: Fix potential deadlock on teardown
2026-11-11 3:20 [PATCH 1/2] HID: nintendo-switch2-usb: Fix potential deadlock on teardown Vicki Pfau
2026-11-11 3:20 ` [PATCH 2/2] HID: nintendo-switch2-usb: Improve shutdown logic with inbound message work Vicki Pfau
@ 2026-11-11 3:37 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-11-11 3:37 UTC (permalink / raw)
To: Vicki Pfau, linux-input; +Cc: dmitry.torokhov
Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: nintendo-switch2-usb: UAF and NULL deref in
switch2_usb_free_in_urbs()
- [High] HID: nintendo-switch2-usb: UAF on switch2_controller during cfg
teardown
- [Medium] HID: nintendo-switch2-usb: URB resubmission after
switch2_usb_shutdown()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] HID: nintendo: missing usb_unanchor_urb() in
switch2_usb_probe() [1]
--
commit e36945396b8fc382e63f59fdcd5efcaa968354a1
Author: Vicki Pfau <vi@endrift.com>
HID: nintendo-switch2-usb: Fix potential deadlock on teardown
This commit replaces cancel_work_sync() on message_in_work with cancel_work()
in switch2_usb_free_in_urbs() and switch2_usb_shutdown(), and adds
cancel_work_sync() calls at the end of switch2_usb_probe() error handling and
switch2_usb_disconnect().
Link: https://sashiko.dev/#/message/20261006063000.A0B821F000FF%40smtp.kernel.org
> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 623473f3d2161..8eafc4fc513d4 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
> @@ -217,7 +217,7 @@ static void switch2_usb_free_in_urbs(struct switch2_usb *ns2_usb)
> ns2_usb->shutdown = true;
> spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>
> - cancel_work_sync(&ns2_usb->message_in_work);
> + cancel_work(&ns2_usb->message_in_work);
>
> usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
> for (i = 0; i < NS2_IN_URBS; i++) {
[Severity: High]
Can deferring cancel_work_sync(&ns2_usb->message_in_work) until after
switch2_usb_free_in_urbs() cause a NULL pointer dereference or use-after-free
on bulk_in[i].urb and its transfer_buffer?
cancel_work() returns immediately if switch2_usb_message_in_work() is already
running on a worker CPU. Once switch2_usb_message_in_work() drops
bulk_in_lock, switch2_usb_free_in_urbs() can clear bulk_in[i].urb and free the
coherent DMA buffer and URB while the worker is still using them:
CPU0 kworker
switch2_usb_disconnect() switch2_usb_message_in_work()
switch2_usb_free_in_urbs() spin_lock_irqsave(&bulk_in_lock)
cancel_work(&message_in_work) urb = &ns2_usb->bulk_in[i]
spin_unlock_irqrestore(&bulk_in_lock)
spin_lock_irqsave(&bulk_in_lock)
ns2_usb->bulk_in[i].urb = NULL
ns2_usb->bulk_in[i].data = NULL
spin_unlock_irqrestore(&bulk_in_lock)
usb_kill_urb(urb)
usb_free_coherent(..., data, ...)
usb_free_urb(urb)
switch2_receive_command(...,
urb->urb->transfer_buffer, ...)
<-- NULL deref or use-after-free
In addition, switch2_usb_free_out_urbs() also runs before cancel_work_sync()
and clears bulk_out[i].urb and bulk_out[i].data while leaving
bulk_out[i].state as NS2_URB_FREE, so if switch2_receive_command() sends a
command via switch2_usb_send_cmd(), it can pick a freed slot and dereference a
NULL urb->data or urb->urb.
> @@ -276,7 +276,7 @@ static void switch2_usb_shutdown(struct switch2_cfg_intf *intf)
> ns2_usb->shutdown = true;
> spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>
> - cancel_work_sync(&ns2_usb->message_in_work);
> + cancel_work(&ns2_usb->message_in_work);
>
> usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
> spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
[Severity: Medium]
Can an in-flight switch2_usb_message_in_work() resubmit a bulk IN URB after
switch2_usb_shutdown() finishes?
drivers/hid/hid-nintendo-switch2-usb.c:switch2_usb_message_in_work() {
...
spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
if (!urb->urb)
/* We're shutting down and this URB was freed */
break;
urb->state = NS2_URB_FREE;
/*
* We want exactly one bulk in URB scheduled at a time, so only
* reschedule this immediately if nothing else is scheduled
* currently.
*/
if (!usb_anchor_empty(&ns2_usb->bulk_in_anchor))
continue;
usb_anchor_urb(urb->urb, &ns2_usb->bulk_in_anchor);
err = usb_submit_urb(urb->urb, GFP_ATOMIC);
...
}
When switch2_usb_shutdown() runs while switch2_usb_message_in_work() is
blocked in switch2_receive_command(), cancel_work() does not wait for the work
item, and switch2_usb_shutdown() kills all anchored URBs while leaving
urb->urb non-NULL. When switch2_usb_message_in_work() reacquires
bulk_in_lock, it only checks !urb->urb and usb_anchor_empty(), not
ns2_usb->shutdown, so it re-anchors and resubmits urb->urb after shutdown.
Note that follow-up commit "HID: nintendo-switch2-usb: Improve shutdown logic
with inbound message work" checks ns2_usb->shutdown at the start of
switch2_usb_message_in_work(), but still does not check it after reacquiring
bulk_in_lock here.
> @@ -447,6 +447,7 @@ static int switch2_usb_probe(struct usb_interface *intf, const struct usb_device
> switch2_usb_free_out_urbs(ns2_usb);
> err_free_in:
> switch2_usb_free_in_urbs(ns2_usb);
> + cancel_work_sync(&ns2_usb->message_in_work);
> devm_kfree(&intf->dev, ns2_usb);
>
> return ret;
[Severity: Medium]
This problem wasn't introduced by this patch, but if usb_submit_urb() fails in
switch2_usb_probe() [1], does switch2_usb_free_in_urbs() hang forever in
usb_kill_anchored_urbs()?
drivers/hid/hid-nintendo-switch2-usb.c:switch2_usb_probe() {
...
spin_lock_irqsave(&ns2_usb->bulk_in_lock, flags);
ns2_usb->bulk_in[0].state = NS2_URB_OUT;
usb_anchor_urb(ns2_usb->bulk_in[0].urb, &ns2_usb->bulk_in_anchor);
ret = usb_submit_urb(ns2_usb->bulk_in[0].urb, GFP_ATOMIC);
spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
if (ret < 0)
goto err_free_out;
...
}
Because bulk_in[0].urb is anchored before usb_submit_urb() and is not
unanchored on submission failure, usb_kill_urb() inside
usb_kill_anchored_urbs() returns immediately without unanchoring the
unsubmitted URB, leaving usb_kill_anchored_urbs() spinning in an infinite loop
waiting for bulk_in_anchor to become empty.
> @@ -466,6 +467,8 @@ static void switch2_usb_disconnect(struct usb_interface *intf)
> switch2_controller_detach_cfg(ns2_usb->cfg.parent);
>
> switch2_usb_free_out_urbs(ns2_usb);
> +
> + cancel_work_sync(&ns2_usb->message_in_work);
> }
>
> #define SWITCH2_CONTROLLER(vend, prod) \
[Severity: High]
Can running switch2_controller_detach_cfg() (or the error path of
switch2_controller_attach_cfg()) before cancel_work_sync() cause a NULL
pointer dereference or use-after-free on struct switch2_controller?
Because switch2_usb_shutdown() no longer waits for switch2_usb_message_in_work()
to finish, switch2_controller_detach_cfg() can clear ns2->cfg->parent and drop
the last reference to ns2 while switch2_usb_message_in_work() is still running
or blocked on mutex_lock(&ns2->lock) inside switch2_receive_command():
CPU0 kworker
switch2_usb_disconnect() switch2_usb_message_in_work()
... if (ns2_usb->cfg.parent)
switch2_controller_detach_cfg(ns2) switch2_receive_command(ns2, ...)
mutex_lock(&ns2->lock)
ns2->cfg->shutdown(ns2->cfg)
cancel_work(&message_in_work)
ns2->cfg->parent = NULL
mutex_unlock(&ns2->lock)
...
kref_put(&ns2->refcount,
switch2_kref_put)
kfree(ns2)
mutex_lock(&ns2->lock)
<-- use-after-free
cancel_work_sync(&message_in_work)
Similarly, if ns2->cfg->parent is cleared between the if (ns2_usb->cfg.parent)
check and the call to switch2_receive_command(ns2_usb->cfg.parent, ...),
switch2_receive_command() will dereference NULL at mutex_lock(&ns2->lock).
[1] https://sashiko.dev/#/bug/linux-278e82ab-e1db-4cb6-bba0-623a80fa475c
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261111032125.2888320-1-vi@endrift.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread