Re: [PATCH] HID: winwing: fix use-after-free in force feedback teardown

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Incorrect teardown ordering in `winwing_remove()` causes `rumble_work` to be re-queued after cancellation, leading to a Use-After-Free.
- [High] The `lights_lock` mutex in `struct winwing_drv_data` is used without ever being initialized.
- [High] LED class devices remain registered after the hardware is stopped, leading to potential Use-After-Free or crashes in the low-level transport driver.
--

--- Patch [1]: [PATCH] HID: winwing: fix use-after-free in force feedback teardown ---
commit 7ead0cac4461910c7f089ed4dac8be573080aceb
Author: René Onier <[email protected]>

HID: winwing: fix use-after-free in force feedback teardown

winwing_init_ff() passes the driver's private data, allocated with
devm_kzalloc() in winwing_probe(), as the effect context to
input_ff_create_memless(). The memoryless force-feedback core takes
ownership of that pointer and frees it with kfree() from
input_ff_destroy() (ml_ff_destroy()) when the input device is
destroyed.

Fixes: 42d020b54edc ("HID: winwing: Enable rumble effects")

> diff --git a/drivers/hid/hid-winwing.c b/drivers/hid/hid-winwing.c
> index 9cd25a77999e6..19b92c2c6579a 100644
> --- a/drivers/hid/hid-winwing.c
> +++ b/drivers/hid/hid-winwing.c
> @@ -315,7 +315,8 @@ static void winwing_haptic_rumble_cb(struct work_struct *work)
>  static int winwing_play_effect(struct input_dev *dev, void *context,
>  		struct ff_effect *effect)
>  {
> -	struct winwing_drv_data *data = (struct winwing_drv_data *) context;
> +	struct hid_device *hdev = input_get_drvdata(dev);
> +	struct winwing_drv_data *data = hid_get_drvdata(hdev);
>  
>  	if (effect->type != FF_RUMBLE)
>  		return 0;
[ ... ]
> @@ -342,7 +343,12 @@ static int winwing_init_ff(struct hid_device *hdev, struct hid_input *hidinput)
>  
>  	input_set_capability(hidinput->input, EV_FF, FF_RUMBLE);
>  
> -	return input_ff_create_memless(hidinput->input, data,
> +	/*
> +	 * input_ff_create_memless() takes ownership of the context pointer
> +	 * and frees it on teardown; do not hand it the devm-managed drvdata.
> +	 * winwing_play_effect() fetches it from the input device instead.
> +	 */
> +	return input_ff_create_memless(hidinput->input, NULL,
>  			winwing_play_effect);
>  }

[Severity: High]
This is a pre-existing issue, but does the teardown ordering in
winwing_remove() in drivers/hid/hid-winwing.c cause rumble_work to be
re-queued after cancellation?

winwing_remove() {
	if (data)
		cancel_work_sync(&data->rumble_work);

	hid_hw_close(hdev);
	hid_hw_stop(hdev);
}

When hid_hw_stop() unregisters the input device, it invokes 
evdev_disconnect() and input_flush_device(). The flush executes
input_ff_flush(), which erases active effects by calling ff->playback()
with a magnitude of 0.

This triggers winwing_play_effect(), which re-schedules data->rumble_work
on the global workqueue. After winwing_remove() returns, devres frees the
data structure, leaving a pending work item that will access freed memory
when it runs.

Could this lead to a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but is the lights_lock mutex in
struct winwing_drv_data used without ever being initialized?

In drivers/hid/hid-winwing.c:winwing_probe():
	data = devm_kzalloc(&hdev->dev, data_size, GFP_KERNEL);
	if (!data)
		return -ENOMEM;

The driver attempts to acquire this uninitialized mutex when userspace writes
to the LED brightness sysfs attribute:

drivers/hid/hid-winwing.c:winwing_led_write():
	mutex_lock(&data->lights_lock);

Can this cause a kernel crash or undefined behavior when lock contention
occurs?

[Severity: High]
This is a pre-existing issue, but do the LED class devices remain registered
after the hardware is stopped?

In drivers/hid/hid-winwing.c:winwing_remove():
	hid_hw_stop(hdev);

The underlying hardware is stopped here, but the devm-managed LEDs remain
registered until after this function returns and the driver's devres group
is released.

If userspace writes to the LED brightness attribute during the window between
hid_hw_stop() and devres cleanup, this triggers invalid hardware access on a
stopped transport:

drivers/hid/hid-winwing.c:winwing_led_write():
	ret = hid_hw_output_report(led->hdev, buf, 14);

Could this crash the kernel or cause a use-after-free in the low-level
transport driver?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1