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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The devres-managed delayed work `ns2->rumble_work` is bound to the `input->dev` lifecycle, but resides in `ns2`, leading to a Use-After-Free.
- [High] LEDs registered with `devm_led_classdev_register` on `input->dev` outlive the controller context `ns2`, leading to a Use-After-Free when sysfs attributes are accessed.
- [Medium] Unconditional cancellation of potentially uninitialized `ns2->rumble_work` in `switch2_remove()` causes lockdep splats and kernel warnings.
--

commit 62e28913646bc7a9de5cfbf6dcd030ac10396625
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.

    Signed-off-by: Vicki Pfau <vi@endrift.com>

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index f82034decb9ee..71a04c438271f 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c

[ ... ]

> @@ -3332,6 +3502,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]
Could this lead to a use-after-free? ns2->rumble_work is tied to the
lifecycle of input->dev, but it resides in ns2. If the physical controller
is unplugged:

switch2_remove()
  kref_put(&ns2->refcount, switch2_kref_put);
    kfree(ns2);

If a userspace process holds the input device open, input->dev remains alive.
When the file descriptor is finally closed, the devres cleanup for input->dev
will execute and attempt to cancel the delayed work on the already-freed ns2
structure.

[Severity: High]
Are there similar lifecycle issues with the LED registration? If the LED class
device is registered using devm_led_classdev_register(&input->dev, led), it
decouples the LED lifecycle from the controller context.

If the controller is unplugged and ns2 is freed, but the input device remains
open, could a write to the sysfs brightness file trigger a use-after-free?

switch2_player_led_brightness_set()
  struct switch2_controller *ns2 = input_get_drvdata(input);
  if (!ns2)
      return -ENODEV;
  
  guard(mutex)(&ns2->lock);

Would dereferencing ns2 here cause memory corruption if the context has
already been freed?

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

[ ... ]

> @@ -4038,6 +4245,9 @@ static void switch2_remove(struct hid_device *hdev)
>  	ns2->hdev = NULL;
>  	ida_free(&nintendo_player_id_allocator, ns2->player_id);
>  	mutex_unlock(&ns2->lock);
> +#if IS_ENABLED(CONFIG_NINTENDO_FF)
> +	cancel_delayed_work_sync(&ns2->rumble_work);

[Severity: Medium]
Will this execute safely if the delayed work was never initialized?

Since ns2 is zero-allocated and ns2->rumble_work is only initialized in
switch2_init_input(), what happens if the device is unplugged or fails
initialization before switch2_init_input() is called?

Would calling cancel_delayed_work_sync() on a zero-initialized work structure
trigger lockdep splats or kernel warnings?

> +#endif
>  	kref_put(&ns2->refcount, switch2_kref_put);
>  	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);

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