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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer truncation of IDA allocation results in out-of-bounds read, IDA resource leak, and kernel WARN.
- [Medium] Race condition causing use-before-initialization of `player_id`, leading to an out-of-bounds read.
- [Medium] Mixing `goto`-based error handling and `scoped_guard()` in the same function violates kernel cleanup subsystem rules.
--

--- Patch [1]: [PATCH v2 1/4] HID: wiimote: turn on the LEDs indicating the controller id ---
commit 49992cb763819a5276c65ada6594868205443ccd
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
> index 63c4fa8fbb9b6..91c2cec35f6eb 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[ ... ]

> @@ -671,6 +678,12 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  	spin_lock_irq(&wdata->state.lock);
>  	wdata->state.devtype = devtype;
>  	spin_unlock_irq(&wdata->state.lock);
> +
> +	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
> +	}
> +
>  	return;
>  
>  error:

[Severity: Medium]
Does using scoped_guard() in this function violate the cleanup subsystem
guidelines?

The wiimote_modules_load() function appears to use a goto-based failure path
(goto error;), but introduces a scoped_guard() scope-based cleanup macro.

The cleanup subsystem documentation indicates that mixing goto-based error
handling and scoped_guard() in the same function can create confusing
ownership semantics and should be avoided.

[ ... ]

> @@ -1834,7 +1851,16 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
>  
> -	hid_info(hdev, "New device registered\n");
> +	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;
> +	}
> +
> +	wdata->player_id = player_id;

[Severity: High]
Can this assignment truncate the allocated ID?

The player_id variable returned by ida_alloc_min() is an int, but
wdata->player_id is defined as a __u8 in struct wiimote_data.

If enough devices are created (e.g., via /dev/uhid) to allocate ID 256,
the value would truncate to 0.

When this 0 is later evaluated in wiimote_modules_load():

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

The expression (0 - 1) % 4 evaluates to -1, which would lead to an
out-of-bounds array read on player_leds[-1].

Additionally, during device removal in wiimote_hid_remove(), calling
ida_free(&wiimote_ida, wdata->player_id) with the truncated 0 would
trigger a WARN_ON_ONCE in the IDA core (since ID 0 was never allocated)
and permanently leak the originally allocated ID.

[Severity: Medium]
Is there a race condition where wdata->player_id is used before it is
initialized?

The hardware I/O is started and opened via hid_hw_open() earlier in
wiimote_hid_probe(), before the player_id is allocated and assigned here.

If the device immediately sends an event, could a background worker
executing wiimote_modules_load() read wdata->player_id while it is still 0?

This would cause the same out-of-bounds read on player_leds[-1] described
above.

[ ... ]

> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..a53f72d5077ef 100644
> --- a/drivers/hid/hid-wiimote.h
> +++ b/drivers/hid/hid-wiimote.h

[ ... ]

> @@ -153,6 +153,7 @@ struct wiimote_data {
>  	struct input_dev *mp;
>  	struct timer_list timer;
>  	struct wiimote_debug *debug;
> +	__u8 player_id;
>  
>  	union {
>  		struct input_dev *input;

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