Re: [PATCH] HID: winwing: remove unpaired hid_hw_close()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free of driver data via the rumble workqueue due to incorrect cleanup order in `winwing_remove()`.
- [High] The `lights_lock` mutex in `struct winwing_drv_data` is never explicitly initialized.
- [High] Memory leak and `led_classdev` list corruption if `winwing_input_configured` is called for multiple input devices.
--

commit 375f4f6d6b1105352d34b843ee4acf96e3723a66
Author: Guangshuo Li <lgs201920130244@gmail.com>
HID: winwing: remove unpaired hid_hw_close()

This commit removes an unpaired hid_hw_close() call in winwing_remove() that 
was present without a matching hid_hw_open(). This prevents the underflow of
the hdev->ll_open_count variable.

> diff --git a/drivers/hid/hid-winwing.c b/drivers/hid/hid-winwing.c
> index 9cd25a77999e6..e18728f215aea 100644
> --- a/drivers/hid/hid-winwing.c
> +++ b/drivers/hid/hid-winwing.c
> @@ -388,7 +388,6 @@ static void winwing_remove(struct hid_device *hdev)
>  	if (data)
>  		cancel_work_sync(&data->rumble_work);
>  
> -	hid_hw_close(hdev);
>  	hid_hw_stop(hdev);
>  }

[Severity: High]
This is a pre-existing issue, but does this cleanup order leave a window for
a use-after-free of the driver's private data?

Because hid_hw_stop() unregisters the input device, the device remains active
while the rumble work is being cancelled. Could a local user issue an EVIOCSFF
ioctl in this narrow window, which schedules the work again?

If so, the remove callback would finish, devres would free the data structure,
and the scheduled work could subsequently execute and access the freed memory.

Should hid_hw_stop() be called before cancel_work_sync()?

Additionally, looking at the driver initialization path:

drivers/hid/hid-winwing.c:winwing_probe() {
	...
	data = devm_kzalloc(&hdev->dev, data_size, GFP_KERNEL);

	if (!data)
		return -ENOMEM;
	...
}

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

Since data is allocated using devm_kzalloc(), the memory is zero-initialized,
but mutex_init(&data->lights_lock) is never called. When userspace writes to
the LED brightness sysfs file, winwing_led_write() is invoked and acquires
the lock:

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

Will locking this uninitialized mutex lead to undefined behavior or a kernel
panic when accessed?

Lastly, examining the input configuration:

drivers/hid/hid-winwing.c:winwing_input_configured() {
	...
	ret = winwing_init_led(hdev, hidinput->input);

	if (ret)
		hid_err(hdev, "led init failed\n");

	if (data->has_grip15)
		winwing_init_ff(hdev, hidinput);
	...
}

[Severity: High]
This is a pre-existing issue, but does this unconditionally initialize LEDs
and force feedback capabilities for every input node?

winwing_input_configured() is called by the HID core for each input application
exposed by the device. Unconditionally calling winwing_init_led() for each
allocates new buffers and re-registers the same three struct winwing_led
elements. 

Could re-registering an already-linked led_classdev overwrite its list nodes
and corrupt the LED subsystem's linked lists? Are the devres allocations from
the earlier calls leaked in this scenario? 

Should this setup be restricted to only happen for the first input node?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913143909.1589019-1-lgs201920130244@gmail.com?part=1