Re: [PATCH v11 1/3] HID: nintendo: Add preliminary Switch 2 controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Concurrent URB completion and device disconnect paths cause a Use-After-Free due to missing synchronization.
- [High] Immediate resubmission of failed bulk IN URBs causes an interrupt storm.
--

Note: The format of this review has been altered to use a free-form summary of findings rather than inline patch quoting due to recitation restrictions.

commit 77b4d4df4f2d3eefe6970b5429bc05e1a86ecc7e
Author: Vicki Pfau <vi@endrift.com>

HID: nintendo: Add preliminary Switch 2 controller driver

This adds a new driver for the Switch 2 controllers. The Switch 2 uses an
unusual split-interface design such that input and rumble occur on the main
HID interface, but all other communication occurs over a "configuration"
interface.

[Severity: High]
In switch2_bulk_in(), could this cause a use-after-free during device
disconnect?

When switch2_usb_disconnect() executes concurrently with an URB completion,
it calls usb_kill_anchored_urbs() to cancel in-flight URBs. However, the USB
core unanchors the URB just before invoking the completion handler. Thus,
usb_kill_anchored_urbs() misses the URB being processed:

switch2_usb_disconnect()
    usb_kill_anchored_urbs(&ns2_usb->bulk_in_anchor);
    cancel_work_sync(&ns2_usb->message_in_work);

If switch2_bulk_in() is preempted before it schedules work:

switch2_bulk_in()
    ...
    if (schedule)
        schedule_work(&ns2_usb->message_in_work);

the disconnect path will complete and free ns2_usb via devres. When the
completion handler resumes, it attempts to access the freed context. Does
this missing synchronization need to be addressed?

[Severity: High]
Can an interrupt storm occur if a bulk IN URB fails persistently?

In switch2_bulk_in(), if an URB completes with an error like -EPIPE or
-EPROTO, the schedule boolean is false, but the URB state is set to
NS2_URB_FREE. The driver then immediately evaluates it for resubmission:

switch2_bulk_in()
    ...
    usb_anchor_urb(ns2_urb->urb, &ns2_usb->bulk_in_anchor);
    err = usb_submit_urb(ns2_urb->urb, GFP_ATOMIC);

Since the device isn't shutting down and the anchor is empty, the exact same
failing URB is resubmitted instantly. This will likely fail again right
away, re-invoking the handler and creating an infinite loop. Should the
resubmission logic be delayed or halted upon encountering persistent
errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702214704.1859350-1-vi@endrift.com?part=1