Re: [PATCH 3/3] HID: wiimote: use scoped cleanup in led creation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The check `if (!instance_id)` after `ida_alloc_min()` incorrectly assumes 0 on failure, missing negative error codes and leading to an out-of-bounds array access and kernel BUG.
--

--- Patch [3]: [PATCH 3/3] HID: wiimote: use scoped cleanup in led creation ---
commit 2a6971d40f61933b6a7890df0931f14066de90e7
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: use scoped cleanup in led creation

This commit updates the wiimod_led_probe function to use modern scoped
cleanup for led allocation. It ensures the led structure is safely freed
on error paths while transferring ownership appropriately on success.

> diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
> index f42f9aa7b2d49..7c305099c82d6 100644
> --- a/drivers/hid/hid-wiimote-modules.c
> +++ b/drivers/hid/hid-wiimote-modules.c
> @@ -340,11 +340,11 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
>  {
>  	struct device *dev = &wdata->hdev->dev;
>  	size_t namesz = strlen(dev_name(dev)) + 9;

[ ... ]

[Severity: High]
Does this driver handle negative error codes from ida_alloc_min() correctly?

Looking at wiimote_hid_probe(), the return value is checked with a boolean
negation:

drivers/hid/hid-wiimote-core.c:wiimote_hid_probe() {
    ...
	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;
	}
    ...
}

If ida_alloc_min() fails due to memory pressure or IDA exhaustion, it
returns a negative error code like -ENOMEM. The check if (!instance_id)
evaluates to false, bypassing the fallback logic and allowing the
negative error code to be stored in wdata->instance_id.

This negative value is later used in an array index calculation:

drivers/hid/hid-wiimote-core.c:wiimote_modules_load() {
    ...
	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
		wiiproto_req_leds(wdata, player_leds[(wdata->instance_id - 1) % 4]);
	}
    ...
}

Can this cause an out-of-bounds array read, since the modulo operator
in C retains the sign of the dividend?

Additionally, when the device is removed or rolled back, this negative ID
is freed:

drivers/hid/hid-wiimote-core.c:wiimote_hid_remove() {
    ...
	ida_free(&wiimote_ida, wdata->instance_id);
    ...
}

Could passing the negative ID to ida_free() explicitly trigger a BUG_ON
and crash the kernel during teardown?

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