Re: [PATCH 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 5 potential issue(s) to consider:

New issues:
- [High] Incorrect error check for `ida_alloc_min()` fails to catch negative error codes, leading to an out-of-bounds array read on `player_leds`.
- [Medium] The fallback logic for IDA allocation failure incorrectly claims ID 1, leading to ID collisions and corruption of the IDA state upon removal.
- [Medium] Mixing of `goto`-based error handling and scope-based cleanup helpers (`scoped_guard`) in the same function.
- [Low] Defining a static array in a header file duplicates it across multiple translation units.

Pre-existing issues:
- [High] Missing `state.lock` when calling `wiimote_cmd_abort` in `wiimote_queue`.
--

--- Patch [1]: [PATCH 1/3] HID: wiimote: turn on the LEDs indicating the controller id ---
commit 48125f73deb74f22332d54c849af1f5b48042ab2
Author: Rafael Passos <rafael@rcpassos.me>

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

This commit introduces a new feature to assign an ID to each connected
Wiimote using an IDA struct. It also updates the module loading sequence
to cycle the LEDs from 1 to 4 to visually indicate the assigned player ID.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 63c4fa8fbb9b6..0d36a38bdcdad 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[Severity: High]
This isn't a bug introduced by this patch, but does wiimote_queue() call
wiimote_cmd_abort() while holding queue.lock instead of state.lock?

drivers/hid/hid-wiimote-core.c:wiimote_queue() {
...
out_error:
	wiimote_cmd_abort(wdata);
out_unlock:
	spin_unlock_irqrestore(&wdata->queue.lock, flags);
}

The documentation for wiimote_cmd_abort() specifies that it requires
state.lock:

drivers/hid/hid-wiimote.h:
/* requires the state.lock spinlock to be held */
static inline void wiimote_cmd_abort(struct wiimote_data *wdata)

Could this cause a data race with other command state accesses?

[ ... ]

> @@ -671,6 +671,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->instance_id - 1) % 4]);
> +	}
> +
>  	return;
>  
>  error:

[Severity: Medium]
Is it acceptable to mix scope-based cleanup helpers like scoped_guard()
with goto-based error handling in the same function?

The cleanup documentation states that goto-based cleanup and scope-based
cleanup should not be mixed within the same routine.

[ ... ]

> @@ -1834,7 +1844,15 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	if (ret)
>  		goto err_free;
>  
> -	hid_info(hdev, "New device registered\n");
> +	instance_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	if (!instance_id) {
> +		hid_err(hdev, "cannot allocate controller id, defaulting to 1\n");
> +		instance_id = 1;
> +	}

[Severity: High]
Since ida_alloc_min() returns a negative error code on failure rather than 0,
does this check fail to catch allocation errors?

If ida_alloc_min() fails and returns a negative error code, the later
calculation in wiimote_modules_load():

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

would result in a negative modulo and an out-of-bounds array read.

[Severity: Medium]
Additionally, if the IDA allocation fails and this fallback logic is reached,
does assigning instance_id = 1 without actually reserving it in the IDA
risk corrupting the state?

[ ... ]

> @@ -1862,6 +1880,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
>  {
>  	struct wiimote_data *wdata = hid_get_drvdata(hdev);
>  
> +	ida_free(&wiimote_ida, wdata->instance_id);
> +
>  	hid_info(hdev, "Device removed\n");
>  	wiimote_destroy(wdata);
>  }

[Severity: Medium]
If the fallback logic in wiimote_hid_probe() set instance_id to 1 without
reserving it, will this unconditional free corrupt the IDA state if another
device had legitimately claimed ID 1?

[ ... ]

> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..639f67c24f15f 100644
> --- a/drivers/hid/hid-wiimote.h
> +++ b/drivers/hid/hid-wiimote.h
> @@ -50,6 +50,13 @@
>  #define WIIPROTO_FLAGS_IR (WIIPROTO_FLAG_IR_BASIC | WIIPROTO_FLAG_IR_EXT | \
>  							WIIPROTO_FLAG_IR_FULL)
>  
> +static const __u8 player_leds[] = {
> +        WIIPROTO_FLAG_LED1,
> +        WIIPROTO_FLAG_LED2,
> +        WIIPROTO_FLAG_LED3,
> +        WIIPROTO_FLAG_LED4
> +};
> +

[Severity: Low]
Does defining a static array in a header file cause it to be unnecessarily
duplicated in the .rodata section for every translation unit that includes
hid-wiimote.h?

[ ... ]

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