Re: [PATCH 3/3] HID: wiimote: use scoped cleanup in led creation
From: sashiko-bot@kernel.org
Date: 2026-07-10 15:44:44
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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