Re: [PATCH v12 2/3] HID: nintendo: Add rumble support for Switch 2 controllers
From: sashiko-bot@kernel.org
Date: 2026-07-15 04:00:21
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Use-After-Free during devm cleanup of delayed work because `ns2` is freed before `input->dev`. - [High] Use-After-Free in `switch2_play_effect()` when the FF-memless timer fires concurrently with or after device removal. - [High] Use-After-Free in LED sysfs callbacks because `devm_led_classdev_register` is bound to the `input->dev` lifecycle. - [Medium] Re-initialization of `rumble_lock` in `switch2_probe()` causes lockdep splats and metadata corruption upon driver rebind. --
--- Patch [2]: [PATCH v12 2/3] HID: nintendo: Add rumble support for Switch 2 controllers ---commit 2b2da5db7899af041358cec4f201f43322db88a4
Author: Vicki Pfau [off-list ref]
HID: nintendo: Add rumble support for Switch 2 controllers
This adds rumble support for both the "HD Rumble" linear resonant actuator
type as used in the Joy-Cons and Pro Controller, as well as the eccentric
rotating mass type used in the GameCube controller. Note that since there's
currently no API for exposing full control of LRAs with evdev, it only
simulates a basic rumble for now.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c index 78c9ad3c16104..76eb4861ab230 100644 --- a/drivers/hid/hid-nintendo.c +++ b/drivers/hid/hid-nintendo.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -3209,6 +3260,128 @@ static void switch2_kref_put(struct kref *refcount) kfree(ns2); } +#if IS_ENABLED(CONFIG_NINTENDO_FF) +static void switch2_encode_rumble(struct switch2_hd_rumble *rumble, uint8_t buffer[5]) +{ + buffer[0] = rumble->hi_freq; + buffer[1] = (rumble->hi_freq >> 8) | (rumble->hi_amp << 2); + buffer[2] = (rumble->hi_amp >> 6) | (rumble->lo_freq << 4); + buffer[3] = (rumble->lo_freq >> 4) | (rumble->lo_amp << 6); + buffer[4] = rumble->lo_amp >> 2; +} + +static int switch2_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) +{ + struct switch2_controller *ns2 = input_get_drvdata(dev); + unsigned long flags; + + if (effect->type != FF_RUMBLE) + return 0; + + if (!ns2) + return -ENODEV; + + spin_lock_irqsave(&ns2->rumble_lock, flags);
[Severity: High] Can switch2_play_effect() access ns2 after it has been freed? The ff-memless timer is only destroyed when input->dev is freed. If an active rumble effect arms this timer and the device is unplugged while the timer is active, switch2_remove() will free ns2 if a file descriptor remains open. The timer could then fire, calling switch2_play_effect(), which fetches the stale ns2 pointer via input_get_drvdata() and attempts to acquire ns2->rumble_lock. [ ... ]
quoted hunk ↗ jump to hunk
@@ -3332,6 +3505,26 @@ static int switch2_init_input(struct switch2_controller *ns2) return -EINVAL; } +#if IS_ENABLED(CONFIG_NINTENDO_FF) + ns2->rumble_buffer = devm_kzalloc(&input->dev, 64, GFP_KERNEL); + if (!ns2->rumble_buffer) { + input_free_device(input); + return -ENOMEM; + } + ret = devm_delayed_work_autocancel(&input->dev, &ns2->rumble_work, switch2_rumble_work);
[Severity: High] Can this lead to a use-after-free during device removal? Since evdev keeps input->dev alive until the file descriptor is closed, tying the delayed work to input->dev means the cancellation action will be deferred. However, switch2_remove() drops the final kref on ns2 and frees it immediately on unplug. When the file descriptor is eventually closed, input->dev is destroyed, and the devm action cancel_delayed_work_sync(&ns2->rumble_work) will execute on the already-freed ns2 struct. [Severity: High] Does a similar use-after-free exist in the LED registration? In switch2_init_input(), the LED device is registered using: devm_led_classdev_register(&input->dev, led); Because this is tied to the input->dev lifecycle rather than the HID device, if the device is unbound, switch2_remove() frees ns2. But if input->dev is kept alive by an open file descriptor, the LED sysfs attributes remain registered. A subsequent read/write to the brightness attribute triggers switch2_player_led_brightness_set(), which fetches the freed ns2 and attempts to acquire ns2->lock.
+ if (ret < 0) {
+ input_free_device(input);
+ return ret;
+ }
+
+ input_set_capability(input, EV_FF, FF_RUMBLE);
+ ret = input_ff_create_memless(input, NULL, switch2_play_effect);
+ if (ret) {
+ input_free_device(input);
+ return ret;
+ }
+#endif[ ... ]
quoted hunk ↗ jump to hunk
@@ -3996,6 +4202,10 @@ static int switch2_probe(struct hid_device *hdev, const struct hid_device_id *id else ns2->player_id = ret; +#if IS_ENABLED(CONFIG_NINTENDO_FF) + spin_lock_init(&ns2->rumble_lock); +#endif
[Severity: Medium] Will this corrupt the lock state on driver rebind? The ns2 structure is shared with a USB config interface (via the switch2_controllers list) and can outlive the HID device binding. If the HID driver is unbound and then rebound, switch2_probe() retrieves the existing ns2 and calls spin_lock_init(&ns2->rumble_lock) on the already active lock, which can cause lockdep splats and metadata corruption.
+
ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
if (ret) {
hid_err(hdev, "hw_start failed %d\n", ret);-- Sashiko AI review · https://sashiko.dev/#/patchset/20260715033409.3599913-1-vi@endrift.com?part=2