Re: [PATCH v16 2/4] HID: nintendo: Add rumble support for Switch 2 controllers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: nintendo: Use-after-free of ns2 in rumble_work due to missing sync cancel
- [High] hid: nintendo: Workqueue list corruption due to rumble_work re-initialization
--

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

HID: nintendo: Add rumble support for Switch 2 controllers

This commit adds basic rumble support for Switch 2 controllers,
simulating standard force feedback using the HD Rumble actuators.
It introduces a delayed work item to manage rumble state updates.

> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index c31ded267be76..c49ca95753206 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
[ ... ]
> @@ -3374,6 +3550,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 of the ns2 structure?

The devm_delayed_work_autocancel() helper binds the work cancellation to
the input device's devres group. If a userspace process holds the input
device open when the physical device is unplugged, the devres execution is
delayed until the file descriptor is closed. However, the driver's remove
function frees the ns2 controller structure synchronously via kref_put()
without waiting for the devres cleanup. If the delayed work is pending,
it will execute and access the freed ns2 structure in switch2_rumble_work().

[Severity: High]
Does this code corrupt the workqueue list if initialization is retried?

In switch2_init_input(), the input device is registered and exposed to
userspace. If a subsequent initialization step fails, such as the LED
registration, the error path unregisters the device. If userspace opens
the device and triggers a rumble effect before unregistration completes,
ns2->rumble_work is queued.

The state machine then retries switch2_init_controller(), which calls
switch2_init_input() again. This allocates a new input device and calls
devm_delayed_work_autocancel() on the exact same &ns2->rumble_work. This
unconditionally calls INIT_DELAYED_WORK() on an already-queued work item,
which overwrites its list pointers and corrupts the workqueue list.

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

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