Re: [PATCH v4 1/4] HID: wiimote: turn on the LEDs indicating the controller id
From: sashiko-bot@kernel.org
Date: 2026-08-17 21:55:39
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 [off-list ref] 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).
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
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/20260817213840.1053216-1-rafael@rcpassos.me?part=1