Re: [PATCH v5 1/3] HID: wiimote: turn on the LEDs indicating the controller id

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: wiimote: UAF on probe error path due to early sysfs exposure
- [Medium] hid: wiimote: premature ida_free() races with running workers
--

commit a053e1ef17e333e649ad6046f972b3ed52cf7844
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: turn on the LEDs indicating the controller id

The behavior in a Wii/Wii U console is to have each controller turn on
a different LED indicating the controller id. This commit implements the
same behavior using the ida struct. Unlike switch controllers, each ID
only turns one LED (from 1 to 4).

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -670,7 +677,10 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  
>  	spin_lock_irq(&wdata->state.lock);
>  	wdata->state.devtype = devtype;
> +	/* after loading modules, set the Player ID LED cycling the available LEDs*/
> +	wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1U) % ARRAY_SIZE(player_leds)]);
>  	spin_unlock_irq(&wdata->state.lock);
> +
>  	return;

[Severity: High]
Could wdata->player_id be 0 here if the initialization worker runs before
probe finishes allocating the ID?

If a sysfs write to dev_attr_extension triggers the initialization worker
before wiimote_hid_probe() has assigned player_id via ida_alloc_range(),
wdata->player_id will still be 0. This would cause an underflow in the
calculation (0 - 1U) % 4, incorrectly indexing player_leds[3].

[ ... ]

> @@ -1759,12 +1770,18 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  	return wdata;
>  }
>  
> +/* Global id allocator for wii remotes */
> +static DEFINE_IDA(wiimote_ida);
> +
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
>  	unsigned long flags;
>  
>  	wiidebug_deinit(wdata);
>  
> +	if (wdata->player_id > 0)
> +		ida_free(&wiimote_ida, wdata->player_id);
> +
>  	/* prevent init_worker from being scheduled again */
>  	spin_lock_irqsave(&wdata->state.lock, flags);
>  	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>  	spin_unlock_irqrestore(&wdata->state.lock, flags);
>  
>  	cancel_work_sync(&wdata->init_worker);

[Severity: Medium]
Is it safe to free the player ID here before synchronously cancelling
wdata->init_worker?

If the asynchronous initialization worker or timer is actively executing (for
example, performing a periodic hotplug scan or LED setup), releasing the ID
early could allow a concurrently probing wiimote to claim the same player ID.

This would result in the still-executing worker of the tearing-down device
continuing to use the old wdata->player_id, which could potentially conflict
with the newly registered device. Should the ida_free() be moved to run
after cancel_work_sync() and timer_shutdown_sync()?

[ ... ]

> @@ -1834,7 +1851,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
>  
> -	hid_info(hdev, "New device registered\n");
> +	ret = ida_alloc_range(&wiimote_ida, 1, 255, GFP_KERNEL);
> +	if (ret < 1) {
> +		hid_err(hdev, "cannot allocate controller id\n");
> +		goto err_free;
> +	}
> +
> +	wdata->player_id = ret;

[Severity: High]
Does this sequence create a use-after-free window if probe fails?

Prior to this point in wiimote_hid_probe(), device_create_file() exposes
sysfs nodes like dev_attr_extension. If userspace writes to "extension",
it can queue wdata->init_worker.

If a subsequent step fails (like device_create_file for dev_attr_devtype
failing before ida_alloc_range), the error path jumps to err_ext. The
err_ext path removes the file and eventually calls kfree(wdata) without
calling cancel_work_sync(&wdata->init_worker). If the init_worker was
already queued by userspace, it will execute and dereference the freed
wdata.

Could the sysfs nodes be created only after player_id is allocated and
all probe setup is complete, or should the err_ext path be updated to
cancel pending work?

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