Re: [PATCH v3 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new `__wiimote_probe_cleanup` scoped cleanup function fails to cancel asynchronous work queues (`init_worker`, `queue.worker`), shut down timers, and unload modules before freeing `wdata`, leading to a use-after-free and memory leaks on the probe error path.
- [High] An out-of-bounds array read occurs in `wiimote_modules_load()` due to integer promotion of the uninitialized `player_id`, causing the index calculation to yield -1.
--

--- Patch [3]: [PATCH v3 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes ---
commit 54b3fd3af006d3a0af4f6d249c24ee95bd657f14
Author: Rafael Passos <[email protected]>

HID: wiimote: use scoped cleanup in wiimote and led probes
    
Cleanup code in wiimote/led probe function, using the scoped cleanup.
This prevents mistakes in future changes to this function.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 762b3c383194e..31ee86affc553 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[ ... ]

> @@ -1772,16 +1772,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  /* Global id allocator for wii remotes */
>  static DEFINE_IDA(wiimote_ida);
>  
> +#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
> +#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
> +
> +static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
> +{
> +	if (!wdata)
> +		return;
> +
> +	if (wdata->player_id)
> +		ida_free(&wiimote_ida, wdata->player_id);
> +
> +	// safe, debugfs checks IS_ERR_OR_NULL
> +	wiidebug_deinit(wdata);
> +	// safe, checks dev for NULL
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
> +	kfree(wdata);

[Severity: High]
Does this missing cancellation cause a use-after-free or memory leak if the
probe fails later?

During wiimote_hid_probe(), after hid_hw_start() and hid_hw_open() succeed,
the hardware is active and can generate HID events. These events are
processed by wiimote_hid_event(), which can asynchronously schedule
wdata->init_worker and wdata->queue.worker.

If a subsequent probe step like ida_alloc_min() fails, this cleanup function
runs and frees wdata without cancelling the work queues (cancel_work_sync()),
shutting down timers (timer_shutdown_sync()), or unloading modules.

If a worker accesses wdata after it has been freed, could it trigger a kernel
crash?

> +}
> +
> +DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
> +	__wiimote_probe_cleanup(_T))
> +

[ ... ]

> @@ -1790,68 +1814,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>  	if (ret) {
>  		hid_err(hdev, "HW start failed\n");
> -		goto err;
> +		return ret;
>  	}
> +	wdata->probe_state |= WIIMOTE_PROBE_HW_STARTED;
>  
>  	ret = hid_hw_open(hdev);
>  	if (ret) {
>  		hid_err(hdev, "cannot start hardware I/O\n");
> -		goto err_stop;
> +		return ret;
>  	}
> +	wdata->probe_state |= WIIMOTE_PROBE_HW_OPENED;

[Severity: High]
Can an event arriving here before player_id is initialized cause an
out-of-bounds read?

If the device sends a status report or hotplug event immediately after
hid_hw_open() completes, wiimote_hid_event() triggers and schedules
wdata->init_worker. 

The worker calls wiimote_init_detect() -> wiimote_init_set_type() ->
wiimote_modules_load().

Because wdata->player_id is only populated later in this function, it is
still 0 when wiimote_modules_load() is called.

In wiimote_modules_load(), the calculation (wdata->player_id - 1) promotes to
int and evaluates to -1. The modulo operator (-1 % 4) yields -1, leading to
an out-of-bounds access on the player_leds array:

drivers/hid/hid-wiimote-core.c:wiimote_modules_load() {
    ...
    wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
    ...
}

Could this lead to an infoleak or kernel oops?

>  
>  	ret = device_create_file(&hdev->dev, &dev_attr_extension);

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