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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Out-of-bounds memory read and IDA leak due to integer truncation of `player_id`.
- [Medium] Unconditional `ida_free(0)` in probe error paths triggers a kernel WARN splat.
--

--- Patch [1]: [PATCH v4 1/4] HID: wiimote: turn on the LEDs indicating the controller id ---
commit ee8a446b2fee34f66eb9e5ae9f8fb5e25ff963be
Author: Rafael Passos <[email protected]>

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..acf31d8b6991d 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);
> +
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
> +		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);

[Severity: High]
Does this calculation cause an out-of-bounds read if player_id is 0?

Because of C integer promotion rules, if player_id is 0, (0 - 1) % 4
evaluates to -1, which results in reading player_leds[-1].

> +	}
> +
>  	return;
[ ... ]
> @@ -1759,12 +1773,17 @@ 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);
>  
> +	ida_free(&wiimote_ida, wdata->player_id);
> +

[Severity: Medium]
Will this unconditionally free ID 0 if early probe steps fail?

If an error occurs in wiimote_hid_probe() before an ID is successfully
allocated, the error path jumps to err_free and calls wiimote_destroy().
Because player_id is initialized to 0, calling ida_free(&wiimote_ida, 0)
for an unallocated ID triggers a WARN(1) splat in the IDA core.

[ ... ]
> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 9c12f63f6dd2d..8e5002f515e24 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;

[Severity: High]
Can this u8 declaration lead to integer truncation and an IDA leak?

The ida_alloc_min() function returns an unbounded int. If the allocated
ID reaches 256, it will silently truncate to 0 when assigned to this
u8 field.

If truncation occurs, the 0 value will later cause the out-of-bounds read
in wiimote_modules_load() mentioned above. Furthermore, wiimote_destroy()
will pass 0 to ida_free(), permanently leaking the originally allocated ID
(e.g., 256).

>  
>  	union {
>  		struct input_dev *input;

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