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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new scoped cleanup function `__wiimote_probe_cleanup()` fails to cancel async workers, timers, and loaded modules on error, leading to a Use-After-Free and dangling registered devices.
- [Medium] Truncation of `player_id` from `int` to `__u8` causes IDA double frees, leaked IDs, and an out-of-bounds array access.
--

--- Patch [3]: [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes ---
commit 29027bf7f674068d4630217f5005fa7c42f22f18
Author: Rafael Passos <rafael@rcpassos.me>
Subject: HID: wiimote: use scoped cleanup in wiimote and led probes

This patch refactors the cleanup paths in the wiimote and led probe functions
to use scoped cleanup macros. It also introduces a state mask to track the
hardware probing progress.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index d37d740a7162f..b549a7d58d512 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1774,16 +1774,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 new cleanup function safely handle asynchronous teardown?

In the original code, failures later in the probe function would route to
the err_free label which called wiimote_destroy(). That function cleanly
cancelled wdata->init_worker and unloaded any registered modules.

If a hardware event occurs during probe:

wiimote_hid_event()
   queues wdata->init_worker

And then an allocation fails (e.g. in wiidebug_init or ida_alloc_min):

__wiimote_probe_cleanup()
   hid_hw_stop(wdata->hdev)
   kfree(wdata)

Will the queued worker execute after wdata has been freed?

[ ... ]

> @@ -1792,68 +1816,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	ret = wiidebug_init(wdata);
>  	if (ret)
> -		goto err_free;
> +		return ret;
>  
> -	player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
>  	if (player_id < 1) {
>  		hid_err(hdev, "cannot allocate controller id\n");
>  		ret = player_id;
> -		goto err_free;
> +		return ret;
>  	}
> -
>  	wdata->player_id = player_id;

[Severity: Medium]
Could this assignment result in truncation if the allocated ID is larger
than 255?

ida_alloc_min() returns an int, but wdata->player_id is defined as a __u8.
If an ID exceeds 255, the truncation could result in 0.

If player_id becomes 0, could it cause an out-of-bounds read later?
Looking at wiimote_modules_load():

wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);

This might evaluate to player_leds[-1].

Additionally, truncation could lead to multiple devices sharing the same
truncated ID, which might cause IDA double frees when devices are removed.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260715212501.3920033-1-rafael@rcpassos.me?part=3