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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The allocated device ID from `ida_alloc_min()` can exceed 255, causing silent truncation when stored in the 8-bit `__u8 player_id` field. This truncation leads to an out-of-bounds array read, a kernel WARN, and an IDA resource leak.
--

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

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

This commit implements the original console behavior of turning on a specific
LED indicating the controller ID. It uses an IDA struct to assign a cycling
1-to-4 ID to each controller.

> 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;

[ ... ]

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

[ ... ]

> @@ -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: Medium]
Could ida_alloc_min() return a value greater than 255 here (for instance, if
256 virtual devices are created)? If so, since wdata->player_id is defined
as a __u8 in struct wiimote_data, wouldn't this assignment silently truncate
the ID and store 0 instead of 256?

[ ... ]

> @@ -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: Medium]
If wdata->player_id was truncated to 0, wouldn't (wdata->player_id - 1) % 4
evaluate to -1 due to C integer promotion to int? It looks like this would
result in an out-of-bounds read of player_leds[-1] and send an arbitrary
byte to the device.

[ ... ]

> @@ -1862,6 +1888,8 @@ static void wiimote_hid_remove(struct hid_device *hdev)
>  {
>  	struct wiimote_data *wdata = hid_get_drvdata(hdev);
>  
> +	ida_free(&wiimote_ida, wdata->player_id);

[Severity: Medium]
If the ID was truncated to 0, would passing 0 to ida_free() trigger a kernel
WARN because 0 was never allocated (since the minimum allocation is 1)? It
seems this would also permanently leak the actual underlying ID (like 256).

>  	hid_info(hdev, "Device removed\n");
>  	wiimote_destroy(wdata);
>  }

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