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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [Critical] Race condition in switch2_get_controller() allows calling kref_get() on an object with a 0-refcount.
- [Critical] The controller structure (ns2) is freed before its devm-managed LEDs are unregistered, causing Use-After-Free.
- [High] Reversed teardown order in switch2_usb_disconnect() causes Use-After-Free of ns2_usb.
- [High] Explicit input_unregister_device() on a devm-managed input device causes resource leaks and LED subsystem list corruption across USB reconnects.
- [High] Double spinlock acquisition in the USB probe error path causes a hard deadlock.
- [High] Probe failure for a duplicate HID device wrongly unregisters the active input device of the existing controller.
- [High] Dangling cfg->parent pointer on switch2_controller_attach_cfg failure leads to Use-After-Free.
- [High] Dereference of ns2->cfg without a NULL check in switch2_receive_command causes a crash.
- [Low] Redundant manual assignment of parent device for devm-managed input device.
--

Note: The format of this review has been altered to a free-form summary due
to recitation restrictions. The original diff is not quoted, but the
locations and issues are described below.

commit d9d2794b04a1b3ab24fb33bed74fba65c1f6f459
Author: Vicki Pfau <vi@endrift.com>
Subject: HID: nintendo: Add preliminary Switch 2 controller driver

This commit adds a new split driver for the Nintendo Switch 2 controllers,
handling input and rumble over a main HID interface and configuration over
a secondary interface.

[Severity: High]
In switch2_usb_disconnect() in drivers/input/joystick/nintendo-switch2-usb.c,
is the teardown sequence reversed?

The code calls cancel_work_sync() for message_in_work before it calls
usb_kill_anchored_urbs() for the bulk_in anchor. If an active IN URB
completes after the work is canceled but before the URBs are killed, its
completion handler could schedule the work again.

Will this lead to a use-after-free of the ns2_usb structure when devres
eventually frees it?

[Severity: Critical]
Looking at switch2_get_controller() and switch2_kref_put() in
drivers/hid/hid-nintendo.c, is there a race condition that allows acquiring
a zero-refcount object?

switch2_kref_put() deletes the entry from the list, but there is a window
where the refcount is zero before the lock is acquired and the item is
removed.

Can a concurrent switch2_get_controller() find this dying object and
blindly call kref_get() on it, resulting in list corruption and a
use-after-free? Should this use kref_get_unless_zero() instead?

[Severity: High]
In switch2_controller_put(), the code explicitly calls
input_unregister_device() on an input device that was allocated with
devm_input_allocate_device().

Does this violate the devm lifecycle? The input subsystem defers freeing a
devm-managed input device and its children until the parent unbinds.

On a USB reconnect, will this leak the old input device and cause a name
collision or list corruption in the LED subsystem when the driver tries
to register new LEDs with the same names?

[Severity: Low]
In switch2_init_input(), the code manually assigns the parent of the input
device to the hdev device.

Since devm_input_allocate_device() automatically sets the parent device,
isn't this manual assignment redundant?

[Severity: Critical]
In switch2_remove(), the code calls switch2_controller_put() and then drops
the final refcount via kref_put(), which frees the ns2 structure.

However, the LED class devices are embedded inside the ns2 structure and
are devm-managed.

Won't devres attempt to unregister these LEDs later during the driver
core's devres_release_all() phase? Will this cause a use-after-free
because the memory for the LEDs has already been freed?

[Severity: High]
In the err_free_in cleanup path of switch2_usb_probe(), if the urb pointer
is NULL, the code calls spin_lock_irqsave() again instead of releasing the
lock with spin_unlock_irqrestore().

Will this double spinlock acquisition cause a hard deadlock under memory
pressure when URB allocation fails?

[Severity: High]
In switch2_probe(), if a second HID interface attempts to bind to an
already claimed physical controller, it correctly detects this and returns
an error.

However, the error path jumps to err_put, which calls
switch2_controller_put().

Does this unintentionally unregister the active input device of the
existing, working controller? Should it just drop the newly acquired
refcount instead?

[Severity: High]
In switch2_controller_attach_cfg(), if switch2_init_controller() fails,
the code clears ns2->cfg but does not clear cfg->parent.

Does this leave a dangling pointer? If an IN URB completes concurrently,
could switch2_usb_message_in_work() observe a non-NULL cfg.parent and
pass a freed ns2 pointer to switch2_receive_command(), causing a
use-after-free?

[Severity: High]
In switch2_receive_command(), the code checks for a packet error and then
unconditionally accesses ns2->cfg->dev to print a warning.

Because this work can run concurrently with switch2_controller_detach_cfg()
(due to the teardown order issue mentioned above), is it possible for
ns2->cfg to be NULL here?

Will this cause a NULL pointer dereference?

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