Re: [PATCH v12 2/3] HID: nintendo: Add rumble support for Switch 2 controllers

[sashiko-bot@kernel.org]

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 <vi@endrift.com>

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.

> 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

[ ... ]

> @@ -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.

[ ... ]

> @@ -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

[ ... ]

> @@ -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