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

[Vicki Pfau <vi@endrift.com>]

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