[PATCH] HID: nintendo-switch2-usb: check URB before use and reset state on free

[PATCH] HID: nintendo-switch2-usb: check URB before use and reset state on free

[Noah Weinberger]

Hello,

Proposed-fix limitation: the checks below do not establish URB or
interface lifetime across the pointer check, lock release, and subsequent
use. These proposed changes may therefore not resolve the teardown race
or the late-work use-after-free described below. Please review the
locking and work/interface lifetime before treating this as a complete
fix; this is an incomplete proposal for maintainer review, not a
validated resolution.

switch2_bulk_in() calls schedule_work() after dropping bulk_in_lock, so
the work can be queued after switch2_usb_free_in_urbs() has returned
from cancel_work_sync(). free_in_urbs() NULLs bulk_in[i].urb but never
resets bulk_in[i].state, so the late work still sees NS2_URB_IN and
dereferences urb->urb->transfer_buffer / urb->urb->actual_length on a
NULL pointer - the existing !urb->urb check sits after those
dereferences:

    if (urb->state != NS2_URB_IN)
            continue;
    spin_unlock_irqrestore(...);
    if (ns2_usb->cfg.parent) {
            err = switch2_receive_command(..., urb->urb->transfer_buffer,
                                    urb->urb->actual_length);   <-- NULL deref
    ...
    if (!urb->urb)   <-- too late

A malicious or misbehaving controller (or an unplug racing active
traffic) wins a narrow window to trigger this; the outcome is an oops
in workqueue context, and a use-after-free if the work runs after the
interface data is released. The patch re-checks urb->urb under the lock
before using it and resets the URB state when freeing, mirroring what
switch2_usb_shutdown() already does.

Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), drivers/hid/hid-nintendo-switch2-usb.c:51-121 (completion), 209-240 (free path), 288-316 (work).
New driver merged in this window (zero prior field exposure).

This was found during an assisted (AI-supported) static source review of
mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
reporter against that tree. The patch below has NOT been compiled and has
NOT been runtime tested, and no reproducer was produced - the triggering
conditions are described above instead. Please treat this as a report for
verification rather than a battle-tested fix.

Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
---
diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
index 8b3b684..c846cef 100644
--- a/drivers/hid/hid-nintendo-switch2-usb.c
+++ b/drivers/hid/hid-nintendo-switch2-usb.c
@@ -231,6 +231,7 @@ static void switch2_usb_free_in_urbs(struct switch2_usb *ns2_usb)
 
 		ns2_usb->bulk_in[i].urb = NULL;
 		ns2_usb->bulk_in[i].data = NULL;
+		ns2_usb->bulk_in[i].state = NS2_URB_FREE;
 		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
 
 		usb_kill_urb(urb);
@@ -298,6 +299,11 @@ static void switch2_usb_message_in_work(struct work_struct *work)
 		urb = &ns2_usb->bulk_in[i];
 		if (urb->state != NS2_URB_IN)
 			continue;
+			/* urb->urb is NULLed by switch2_usb_free_in_urbs() while we
+			 * may still run; re-check under the lock before using it.
+			 */
+			if (!urb->urb)
+				continue;
 		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
 
 		if (ns2_usb->cfg.parent) {

Re: [PATCH] HID: nintendo-switch2-usb: check URB before use and reset state on free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] HID: nintendo-switch2-usb: TOCTOU race in
  switch2_usb_message_in_work()
- [Low] HID: nintendo-switch2-usb: misleading indentation in
  switch2_usb_message_in_work()
- [Low] HID: nintendo-switch2-usb: missing Fixes and Signed-off-by
  trailers in changelog
--

commit ad9c19b8222174b67d5185145f7040b445c19cb7
Author: Noah Weinberger <NoahWeinberger2003@proton.me>

> HID: nintendo-switch2-usb: check URB before use and reset state on free
>
> Hello,
>
> Proposed-fix limitation: the checks below do not establish URB or
> interface lifetime across the pointer check, lock release, and subsequent
> use. These proposed changes may therefore not resolve the teardown race
> or the late-work use-after-free described below. Please review the
> locking and work/interface lifetime before treating this as a complete
> fix; this is an incomplete proposal for maintainer review, not a
> validated resolution.
>
> switch2_bulk_in() calls schedule_work() after dropping bulk_in_lock, so
> the work can be queued after switch2_usb_free_in_urbs() has returned
> from cancel_work_sync(). free_in_urbs() NULLs bulk_in[i].urb but never
> resets bulk_in[i].state, so the late work still sees NS2_URB_IN and
> dereferences urb->urb->transfer_buffer / urb->urb->actual_length on a
> NULL pointer - the existing !urb->urb check sits after those
> dereferences:
>
>     if (urb->state != NS2_URB_IN)
>             continue;
>     spin_unlock_irqrestore(...);
>     if (ns2_usb->cfg.parent) {
>             err = switch2_receive_command(..., urb->urb->transfer_buffer,
>                                     urb->urb->actual_length);   <-- NULL deref
>     ...
>     if (!urb->urb)   <-- too late
>
> A malicious or misbehaving controller (or an unplug racing active
> traffic) wins a narrow window to trigger this; the outcome is an oops
> in workqueue context, and a use-after-free if the work runs after the
> interface data is released. The patch re-checks urb->urb under the lock
> before using it and resets the URB state when freeing, mirroring what
> switch2_usb_shutdown() already does.
>
> Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), drivers/hid/hid-nintendo-switch2-usb.c:51-121 (completion), 209-240 (free path), 288-316 (work).
> New driver merged in this window (zero prior field exposure).
>
> This was found during an assisted (AI-supported) static source review of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and has
> NOT been runtime tested, and no reproducer was produced - the triggering
> conditions are described above instead. Please treat this as a report for
> verification rather than a battle-tested fix.
>
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>

[Severity: Low]
The commit message is missing a Signed-off-by: trailer, includes conversational
report boilerplate in the changelog body, and should include:

  Fixes: 9417132df63e ("HID: nintendo: Add preliminary Switch 2 controller driver")

> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 8b3b6841035d0..c103cedc181ff 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c

[ ... ]

> @@ -298,6 +299,11 @@ static void switch2_usb_message_in_work(struct work_struct *work)
>  		urb = &ns2_usb->bulk_in[i];
>  		if (urb->state != NS2_URB_IN)
>  			continue;
> +			/* urb->urb is NULLed by switch2_usb_free_in_urbs() while we
> +			 * may still run; re-check under the lock before using it.
> +			 */
> +			if (!urb->urb)
> +				continue;

[Severity: Low]
Does this over-indent the comment and if (!urb->urb) check by one tab level
after the unbraced continue?  This triggers a -Wmisleading-indentation
compiler warning (and a build failure with CONFIG_WERROR=y).

>  		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>  
>  		if (ns2_usb->cfg.parent) {

[Severity: High]
Can switch2_usb_free_in_urbs() still clear and free urb->urb right after
switch2_usb_message_in_work() drops bulk_in_lock here?

switch2_bulk_in() calls schedule_work(&ns2_usb->message_in_work) after
releasing bulk_in_lock, and switch2_usb_free_in_urbs() calls
cancel_work_sync() before killing the URBs, so message_in_work can still be
scheduled and run concurrently with teardown:

CPU0                                  CPU1 (kworker)
switch2_bulk_in()
  lock(&ns2_usb->bulk_in_lock)
  urb->state = NS2_URB_IN
  unlock(&ns2_usb->bulk_in_lock)

switch2_usb_free_in_urbs()
  ns2_usb->shutdown = true
  cancel_work_sync()
  usb_kill_anchored_urbs()

switch2_bulk_in() (resumes)
  schedule_work(&message_in_work)
                                      switch2_usb_message_in_work()
                                        lock(&ns2_usb->bulk_in_lock)
                                        if (!urb->urb)
                                        unlock(&ns2_usb->bulk_in_lock)
  lock(&ns2_usb->bulk_in_lock)
  ns2_usb->bulk_in[i].urb = NULL
  unlock(&ns2_usb->bulk_in_lock)
  usb_free_coherent(...)
  usb_free_urb(urb)
                                        switch2_receive_command(...,
                                          urb->urb->transfer_buffer, ...)
                                          <-- NULL dereference / UAF

If message_in_work runs after switch2_usb_disconnect() or the
switch2_usb_probe() error path frees ns2_usb, it also dereferences freed
ns2_usb memory, and when re-acquiring bulk_in_lock at the end of the loop it
does not check ns2_usb->shutdown before resubmitting the URB.  Killing the
URBs before cancel_work_sync() (or moving schedule_work() inside bulk_in_lock
while checking ns2_usb->shutdown) would prevent new work from being queued
after cancel_work_sync() completes.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/HP2LhXZGIipAd2XxKx7dDh8q3SopkJRAxo8IQkuyMxTlR0YfO3rS3CT9NqUkg3ispO72yfhC7Oa4MoZdXgo6lNIEjC1WvQFlrKreFzCnmFM=@proton.me?part=1

Re: [PATCH] HID: nintendo-switch2-usb: check URB before use and reset state on free

[Vicki Pfau]

Hi Noah,

On 11/8/26 2:29 PM, Noah Weinberger wrote:
> Hello,
> 
> Proposed-fix limitation: the checks below do not establish URB or
> interface lifetime across the pointer check, lock release, and subsequent
> use. These proposed changes may therefore not resolve the teardown race
> or the late-work use-after-free described below. Please review the
> locking and work/interface lifetime before treating this as a complete
> fix; this is an incomplete proposal for maintainer review, not a
> validated resolution.
> 
> switch2_bulk_in() calls schedule_work() after dropping bulk_in_lock, so
> the work can be queued after switch2_usb_free_in_urbs() has returned
> from cancel_work_sync(). free_in_urbs() NULLs bulk_in[i].urb but never
> resets bulk_in[i].state, so the late work still sees NS2_URB_IN and
> dereferences urb->urb->transfer_buffer / urb->urb->actual_length on a
> NULL pointer - the existing !urb->urb check sits after those
> dereferences:
> 
>     if (urb->state != NS2_URB_IN)
>             continue;
>     spin_unlock_irqrestore(...);
>     if (ns2_usb->cfg.parent) {
>             err = switch2_receive_command(..., urb->urb->transfer_buffer,
>                                     urb->urb->actual_length);   <-- NULL deref
>     ...
>     if (!urb->urb)   <-- too late
> 
> A malicious or misbehaving controller (or an unplug racing active
> traffic) wins a narrow window to trigger this; the outcome is an oops
> in workqueue context, and a use-after-free if the work runs after the
> interface data is released. The patch re-checks urb->urb under the lock
> before using it and resets the URB state when freeing, mirroring what
> switch2_usb_shutdown() already does.
> 
> Affected version/commit: mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07), drivers/hid/hid-nintendo-switch2-usb.c:51-121 (completion), 209-240 (free path), 288-316 (work).
> New driver merged in this window (zero prior field exposure).
> 
> This was found during an assisted (AI-supported) static source review of
> mainline fa4ad798f6f681031775266044ba1b0cf3c670bd (v7.4-rc1, 2026-11-07); every code path cited below was re-verified by the
> reporter against that tree. The patch below has NOT been compiled and has
> NOT been runtime tested, and no reproducer was produced - the triggering
> conditions are described above instead. Please treat this as a report for
> verification rather than a battle-tested fix.
> 
> Reported-by: Noah Weinberger <noahweinberger2003@proton.me>
> ---
> diff --git a/drivers/hid/hid-nintendo-switch2-usb.c b/drivers/hid/hid-nintendo-switch2-usb.c
> index 8b3b684..c846cef 100644
> --- a/drivers/hid/hid-nintendo-switch2-usb.c
> +++ b/drivers/hid/hid-nintendo-switch2-usb.c
> @@ -231,6 +231,7 @@ static void switch2_usb_free_in_urbs(struct switch2_usb *ns2_usb)
>  
>  		ns2_usb->bulk_in[i].urb = NULL;
>  		ns2_usb->bulk_in[i].data = NULL;
> +		ns2_usb->bulk_in[i].state = NS2_URB_FREE;
>  		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>  
>  		usb_kill_urb(urb);
> @@ -298,6 +299,11 @@ static void switch2_usb_message_in_work(struct work_struct *work)
>  		urb = &ns2_usb->bulk_in[i];
>  		if (urb->state != NS2_URB_IN)
>  			continue;
> +			/* urb->urb is NULLed by switch2_usb_free_in_urbs() while we
> +			 * may still run; re-check under the lock before using it.
> +			 */
> +			if (!urb->urb)
> +				continue;
>  		spin_unlock_irqrestore(&ns2_usb->bulk_in_lock, flags);
>  
>  		if (ns2_usb->cfg.parent) {

NAK this approach. This doesn't actually close the window (assuming it actually exists), it only narrows it, which should be pretty apparent by looking at what locks are held during freeing the URBs. The code is pretty aggressive about not scheduling the work after shutdown so I'm unclear if this is even possible. Furthermore, it makes sure the work is canceled before it frees the URBs. Just because the LLM says so doesn't mean it's true.

I can add an additional shutdown check in the beginning of the work, which would resolve this if it's possible.

Vicki