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