[PATCH v2 0/4] HID: wiimote: new LED behavior on connect, scoped guards, uaf

[PATCH v2 0/4] HID: wiimote: new LED behavior on connect, scoped guards, uaf

[Rafael Passos]

Hi,
This patchset contains one feature change, and two cleanup patches.

The feature is turning different LEDs for each of the first 4 wiimotes connected.
From id 5 forward, the LED will cycle back to 1, and so on.
This uses the ida struct, so its quite simple and lightweight.
The hid_info log message prints out the controller id.

While implementing this feature, I decided to cleanup the code using
scoped_guard for the many spinlocks in the driver. There are two places
where the original lock/unlock version fits best, and I left them
untouched.
Lastly, I used the __free scope cleanup in the wiimote and LED probe functions.
The LED is trivial. The wiimote_probe required a new state tracker bitmask.

It was really fun working with this driver.
I tested it with 4 Wii Motion Plus remotes (gen2).
Video recording of my tests (48s video).
https://rcpassos.me/video/wiimote-led-linux-driver

Thanks,
Rafael Passos

---
V1: https://lore.kernel.org/linux-input/20260710153456.2093889-1-rafael@rcpassos.me/
Changes from v1:
    (1/3):
    - fix ida_alloc_min error handling to consider negative values
    - remove fallback to 1 on ida_alloc_min failure
    - move player_leds static array to hid-wiimote-core.c
    - s/instance_id/player_id/g
    - store player_id on an u8
    (2/3):
    - add header include for cleanup.h
    - add identation to one-liner scoped_guards
    (3/3):
    - add scoped cleanup function to wiimote_probe, with a bitmask to track state
      (I will also send in response to this patch, what I used to test it).
    (4/4) *new patch* :
    - sashiko found a pre-existing uaf. Unlikely, but correct.
      implemented using the playstation driver as an inspiration


Rafael Passos (4):
  HID: wiimote: turn on the LEDs indicating the controller id
  HID: wiimote: replace spinlock pairs with scoped_guard
  HID: wiimote: use scoped cleanup in wiimote and led probes
  HID: wiimote: fix uaf when hid events are handled during destroy

 drivers/hid/hid-wiimote-core.c    | 335 ++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c   |  50 ++---
 drivers/hid/hid-wiimote-modules.c |  24 +--
 drivers/hid/hid-wiimote.h         |   2 +
 4 files changed, 215 insertions(+), 196 deletions(-)

-- 
2.53.0

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

[Rafael Passos]

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).

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c | 54 ++++++++++++++++++++++++++++++----
 drivers/hid/hid-wiimote.h      |  1 +
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 63c4fa8fbb9b..91c2cec35f6e 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -621,6 +621,13 @@ static const __u8 * const wiimote_devtype_mods[WIIMOTE_DEV_NUM] = {
 	},
 };
 
+static const __u8 player_leds[] = {
+	WIIPROTO_FLAG_LED1,
+	WIIPROTO_FLAG_LED2,
+	WIIPROTO_FLAG_LED3,
+	WIIPROTO_FLAG_LED4
+};
+
 static void wiimote_modules_load(struct wiimote_data *wdata,
 				 unsigned int devtype)
 {
@@ -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);
+
+	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
+	}
+
 	return;
 
 error:
@@ -855,11 +868,11 @@ static void wiimote_init_set_type(struct wiimote_data *wdata,
 
 done:
 	if (devtype == WIIMOTE_DEV_GENERIC)
-		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: %04x EXT: %04x\n",
-			name, vendor, product, exttype);
+		hid_info(wdata->hdev, "cannot detect device; NAME: %s VID: %04x PID: %04x EXT: %04x (%d)\n",
+			name, vendor, product, exttype, wdata->player_id);
 	else
-		hid_info(wdata->hdev, "detected device: %s\n",
-			 wiimote_devtype_names[devtype]);
+		hid_info(wdata->hdev, "detected device: %s (%d)\n",
+			 wiimote_devtype_names[devtype], wdata->player_id);
 
 	wiimote_modules_load(wdata, devtype);
 }
@@ -1786,11 +1799,15 @@ static void wiimote_destroy(struct wiimote_data *wdata)
 	kfree(wdata);
 }
 
+/* Global id allocator for wii remotes */
+static DEFINE_IDA(wiimote_ida);
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
 	struct wiimote_data *wdata;
 	int ret;
+	int player_id;
 
 	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
@@ -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;
+
+	hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
@@ -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);
+
 	hid_info(hdev, "Device removed\n");
 	wiimote_destroy(wdata);
 }
@@ -1887,7 +1915,21 @@ static struct hid_driver wiimote_hid_driver = {
 	.remove = wiimote_hid_remove,
 	.raw_event = wiimote_hid_event,
 };
-module_hid_driver(wiimote_hid_driver);
+
+
+static int __init wiimote_init(void)
+{
+	return hid_register_driver(&wiimote_hid_driver);
+}
+
+static void __exit wiimote_exit(void)
+{
+	hid_unregister_driver(&wiimote_hid_driver);
+	ida_destroy(&wiimote_ida);
+}
+
+module_init(wiimote_init);
+module_exit(wiimote_exit);
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index 9c12f63f6dd2..a53f72d5077e 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;
 
 	union {
 		struct input_dev *input;
-- 
2.53.0

[PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard

[Rafael Passos]

Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

The multi line spinlock blocks were replaced by braced scoped_guard,
and one-liners by a scoped_guard without braces nor indentation.

There are two cases left in this driver using lock/unlock, because
guard would make the code more complex than current implementation.

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c    | 222 +++++++++++++-----------------
 drivers/hid/hid-wiimote-debug.c   |  50 +++----
 drivers/hid/hid-wiimote-modules.c |   7 +-
 3 files changed, 121 insertions(+), 158 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 91c2cec35f6e..d37d740a7162 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -7,6 +7,7 @@
 /*
  */
 
+#include <linux/cleanup.h>
 #include <linux/completion.h>
 #include <linux/device.h>
 #include <linux/hid.h>
@@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset,
 int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
 						const __u8 *wmem, __u8 size)
 {
-	unsigned long flags;
 	int ret;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
-	wiiproto_req_wreg(wdata, offset, wmem, size);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	    wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
+	    wiiproto_req_wreg(wdata, offset, wmem, size);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 	if (!ret && wdata->state.cmd_err)
@@ -381,21 +381,19 @@ int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
 ssize_t wiimote_cmd_read(struct wiimote_data *wdata, __u32 offset, __u8 *rmem,
 								__u8 size)
 {
-	unsigned long flags;
 	ssize_t ret;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_size = size;
-	wdata->state.cmd_read_buf = rmem;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, offset & 0xffff);
-	wiiproto_req_rreg(wdata, offset, size);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.cmd_read_size = size;
+		wdata->state.cmd_read_buf = rmem;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, offset & 0xffff);
+		wiiproto_req_rreg(wdata, offset, size);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.cmd_read_buf = NULL;
 
 	if (!ret) {
 		if (wdata->state.cmd_read_size == 0)
@@ -680,9 +678,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 	spin_unlock_irq(&wdata->state.lock);
 
 	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
-	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
 		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
-	}
 
 	return;
 
@@ -703,13 +700,11 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
 {
 	const __u8 *mods, *iter;
 	const struct wiimod_ops *ops;
-	unsigned long flags;
 
 	mods = wiimote_devtype_mods[wdata->state.devtype];
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
 
 	/* find end of list */
 	for (iter = mods; *iter != WIIMOD_NULL; ++iter)
@@ -736,7 +731,6 @@ static void wiimote_modules_unload(struct wiimote_data *wdata)
 
 static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 	int ret;
 
@@ -748,22 +742,20 @@ static void wiimote_ext_load(struct wiimote_data *wdata, unsigned int ext)
 			ext = WIIMOTE_EXT_UNKNOWN;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.exttype = ext;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.exttype = ext;
 }
 
 static void wiimote_ext_unload(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 
 	ops = wiimod_ext_table[wdata->state.exttype];
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.exttype = WIIMOTE_EXT_UNKNOWN;
-	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_USED;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.exttype = WIIMOTE_EXT_UNKNOWN;
+		wdata->state.flags &= ~WIIPROTO_FLAG_EXT_USED;
+	}
 
 	if (ops->remove)
 		ops->remove(ops, wdata);
@@ -771,7 +763,6 @@ static void wiimote_ext_unload(struct wiimote_data *wdata)
 
 static void wiimote_mp_load(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 	int ret;
 	__u8 mode = 2;
@@ -783,14 +774,12 @@ static void wiimote_mp_load(struct wiimote_data *wdata)
 			mode = 1;
 	}
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.mp = mode;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.mp = mode;
 }
 
 static void wiimote_mp_unload(struct wiimote_data *wdata)
 {
-	unsigned long flags;
 	const struct wiimod_ops *ops;
 
 	if (wdata->state.mp < 2)
@@ -798,10 +787,10 @@ static void wiimote_mp_unload(struct wiimote_data *wdata)
 
 	ops = &wiimod_mp;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.mp = 0;
-	wdata->state.flags &= ~WIIPROTO_FLAG_MP_USED;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.mp = 0;
+		wdata->state.flags &= ~WIIPROTO_FLAG_MP_USED;
+	}
 
 	if (ops->remove)
 		ops->remove(ops, wdata);
@@ -885,19 +874,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
 
 	wiimote_cmd_acquire_noint(wdata);
 
-	spin_lock_irq(&wdata->state.lock);
-	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
-	wiiproto_req_status(wdata);
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
+		wiiproto_req_status(wdata);
+	}
+
 
 	ret = wiimote_cmd_wait_noint(wdata);
 	if (ret)
 		goto out_release;
 
-	spin_lock_irq(&wdata->state.lock);
-	ext = wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED;
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
+		ext = wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED;
 
 	if (!ext)
 		goto out_release;
@@ -910,11 +899,11 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
 	wiimote_init_set_type(wdata, exttype);
 
 	/* schedule MP timer */
-	spin_lock_irq(&wdata->state.lock);
-	if (!(wdata->state.flags & WIIPROTO_FLAG_BUILTIN_MP) &&
-	    !(wdata->state.flags & WIIPROTO_FLAG_NO_MP))
-		mod_timer(&wdata->timer, jiffies + HZ * 4);
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		if (!(wdata->state.flags & WIIPROTO_FLAG_BUILTIN_MP) &&
+		!(wdata->state.flags & WIIPROTO_FLAG_NO_MP))
+			mod_timer(&wdata->timer, jiffies + HZ * 4);
+	}
 }
 
 /*
@@ -962,9 +951,8 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 	__u8 type, data[6];
 	bool ret, poll_mp;
 
-	spin_lock_irq(&wdata->state.lock);
-	flags = wdata->state.flags;
-	spin_unlock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock)
+		flags = wdata->state.flags;
 
 	wiimote_cmd_acquire_noint(wdata);
 
@@ -980,11 +968,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_mp_mapped(wdata);
 		ret = type == WIIMOTE_MP_SINGLE;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_PLUGGED);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_PLUGGED);
+			ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: !EXT && MP\n");
@@ -1005,10 +993,10 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_ext(wdata, data);
 		ret = type == wdata->state.exttype;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+			ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: EXT && !MP\n");
@@ -1031,11 +1019,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		type = wiimote_cmd_read_ext(wdata, data);
 		ret = type == wdata->state.exttype;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+			ret = ret && !(wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: !EXT && !MP\n");
@@ -1061,11 +1049,11 @@ static bool wiimote_init_check(struct wiimote_data *wdata)
 		ret = ret && type != WIIMOTE_MP_UNKNOWN;
 		ret = ret && type != WIIMOTE_MP_SINGLE;
 
-		spin_lock_irq(&wdata->state.lock);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
-		ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
-		spin_unlock_irq(&wdata->state.lock);
+		scoped_guard(spinlock_irq, &wdata->state.lock) {
+			ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_PLUGGED);
+			ret = ret && (wdata->state.flags & WIIPROTO_FLAG_EXT_ACTIVE);
+			ret = ret && (wdata->state.flags & WIIPROTO_FLAG_MP_ACTIVE);
+		}
 
 		if (!ret)
 			hid_dbg(wdata->hdev, "state left: EXT && MP\n");
@@ -1120,16 +1108,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 
 	wiimote_cmd_acquire_noint(wdata);
 
-	spin_lock_irq(&wdata->state.lock);
-
-	/* get state snapshot that we will then work on */
-	flags = wdata->state.flags;
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
 
-	/* disable event forwarding temporarily */
-	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
-	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
+	    /* get state snapshot that we will then work on */
+	    flags = wdata->state.flags;
 
-	spin_unlock_irq(&wdata->state.lock);
+	    /* disable event forwarding temporarily */
+	    wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
+	    wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
+	}
 
 	/* init extension and MP (deactivates current extension or MP) */
 	wiimote_cmd_init_ext(wdata);
@@ -1152,9 +1139,8 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 			hid_info(wdata->hdev, "cannot detect extension; %6phC\n",
 				 extdata);
 		} else if (exttype == WIIMOTE_EXT_NONE) {
-			spin_lock_irq(&wdata->state.lock);
-			wdata->state.exttype = WIIMOTE_EXT_NONE;
-			spin_unlock_irq(&wdata->state.lock);
+			scoped_guard(spinlock_irq, &wdata->state.lock)
+				wdata->state.exttype = WIIMOTE_EXT_NONE;
 		} else {
 			hid_info(wdata->hdev, "detected extension: %s\n",
 				 wiimote_exttype_names[exttype]);
@@ -1192,27 +1178,25 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 			mod_timer(&wdata->timer, jiffies + HZ * 4);
 	}
 
-	spin_lock_irq(&wdata->state.lock);
-
-	/* enable data forwarding again and set expected hotplug state */
-	if (mp) {
-		wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
-		if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
-			wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
-			wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
-		} else {
-			wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
-			wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
+		/* enable data forwarding again and set expected hotplug state */
+		if (mp) {
+			wdata->state.flags |= WIIPROTO_FLAG_MP_ACTIVE;
+			if (wdata->state.exttype == WIIMOTE_EXT_NONE) {
+				wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+				wdata->state.flags &= ~WIIPROTO_FLAG_MP_PLUGGED;
+			} else {
+				wdata->state.flags &= ~WIIPROTO_FLAG_EXT_PLUGGED;
+				wdata->state.flags |= WIIPROTO_FLAG_MP_PLUGGED;
+				wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
+			}
+		} else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
 			wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
 		}
-	} else if (wdata->state.exttype != WIIMOTE_EXT_NONE) {
-		wdata->state.flags |= WIIPROTO_FLAG_EXT_ACTIVE;
-	}
-
-	/* request status report for hotplug state updates */
-	wiiproto_req_status(wdata);
 
-	spin_unlock_irq(&wdata->state.lock);
+		/* request status report for hotplug state updates */
+		wiiproto_req_status(wdata);
+	}
 
 	hid_dbg(wdata->hdev, "detected extensions: MP: %d EXT: %d\n",
 		wdata->state.mp, wdata->state.exttype);
@@ -1244,11 +1228,8 @@ void __wiimote_schedule(struct wiimote_data *wdata)
 
 static void wiimote_schedule(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	__wiimote_schedule(wdata);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		__wiimote_schedule(wdata);
 }
 
 static void wiimote_init_timeout(struct timer_list *t)
@@ -1638,7 +1619,6 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	struct wiimote_data *wdata = hid_get_drvdata(hdev);
 	const struct wiiproto_handler *h;
 	int i;
-	unsigned long flags;
 
 	if (size < 1)
 		return -EINVAL;
@@ -1646,9 +1626,8 @@ static int wiimote_hid_event(struct hid_device *hdev, struct hid_report *report,
 	for (i = 0; handlers[i].id; ++i) {
 		h = &handlers[i];
 		if (h->id == raw_data[0] && h->size < size) {
-			spin_lock_irqsave(&wdata->state.lock, flags);
-			h->func(wdata, &raw_data[1]);
-			spin_unlock_irqrestore(&wdata->state.lock, flags);
+			scoped_guard(spinlock_irqsave, &wdata->state.lock)
+				h->func(wdata, &raw_data[1]);
 			break;
 		}
 	}
@@ -1666,11 +1645,9 @@ static ssize_t wiimote_ext_show(struct device *dev,
 {
 	struct wiimote_data *wdata = dev_to_wii(dev);
 	__u8 type;
-	unsigned long flags;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	type = wdata->state.exttype;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		type = wdata->state.exttype;
 
 	switch (type) {
 	case WIIMOTE_EXT_NONE:
@@ -1719,11 +1696,9 @@ static ssize_t wiimote_dev_show(struct device *dev,
 {
 	struct wiimote_data *wdata = dev_to_wii(dev);
 	__u8 type;
-	unsigned long flags;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	type = wdata->state.devtype;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		type = wdata->state.devtype;
 
 	switch (type) {
 	case WIIMOTE_DEV_GENERIC:
@@ -1774,14 +1749,11 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
 
 static void wiimote_destroy(struct wiimote_data *wdata)
 {
-	unsigned long flags;
-
 	wiidebug_deinit(wdata);
 
 	/* prevent init_worker from being scheduled again */
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
 
 	cancel_work_sync(&wdata->init_worker);
 	timer_shutdown_sync(&wdata->timer);
diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
index 5f74917781f2..df6f830ee791 100644
--- a/drivers/hid/hid-wiimote-debug.c
+++ b/drivers/hid/hid-wiimote-debug.c
@@ -7,6 +7,7 @@
 /*
  */
 
+#include <linux/cleanup.h>
 #include <linux/debugfs.h>
 #include <linux/module.h>
 #include <linux/seq_file.h>
@@ -25,7 +26,6 @@ static ssize_t wiidebug_eeprom_read(struct file *f, char __user *u, size_t s,
 {
 	struct wiimote_debug *dbg = f->private_data;
 	struct wiimote_data *wdata = dbg->wdata;
-	unsigned long flags;
 	ssize_t ret;
 	char buf[16];
 	__u16 size = 0;
@@ -41,20 +41,19 @@ static ssize_t wiidebug_eeprom_read(struct file *f, char __user *u, size_t s,
 	if (ret)
 		return ret;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_size = s;
-	wdata->state.cmd_read_buf = buf;
-	wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, *off & 0xffff);
-	wiiproto_req_reeprom(wdata, *off, s);
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.cmd_read_size = s;
+		wdata->state.cmd_read_buf = buf;
+		wiimote_cmd_set(wdata, WIIPROTO_REQ_RMEM, *off & 0xffff);
+		wiiproto_req_reeprom(wdata, *off, s);
+	}
 
 	ret = wiimote_cmd_wait(wdata);
 	if (!ret)
 		size = wdata->state.cmd_read_size;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->state.cmd_read_buf = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->state.cmd_read_buf = NULL;
 
 	wiimote_cmd_release(wdata);
 
@@ -99,12 +98,10 @@ static int wiidebug_drm_show(struct seq_file *f, void *p)
 {
 	struct wiimote_debug *dbg = f->private;
 	const char *str = NULL;
-	unsigned long flags;
 	__u8 drm;
 
-	spin_lock_irqsave(&dbg->wdata->state.lock, flags);
-	drm = dbg->wdata->state.drm;
-	spin_unlock_irqrestore(&dbg->wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &dbg->wdata->state.lock)
+		drm = dbg->wdata->state.drm;
 
 	if (drm < WIIPROTO_REQ_MAX)
 		str = wiidebug_drmmap[drm];
@@ -126,7 +123,6 @@ static ssize_t wiidebug_drm_write(struct file *f, const char __user *u,
 {
 	struct seq_file *sf = f->private_data;
 	struct wiimote_debug *dbg = sf->private;
-	unsigned long flags;
 	char buf[16];
 	ssize_t len;
 	int i;
@@ -150,12 +146,12 @@ static ssize_t wiidebug_drm_write(struct file *f, const char __user *u,
 	if (i == WIIPROTO_REQ_MAX)
 		i = simple_strtoul(buf, NULL, 16);
 
-	spin_lock_irqsave(&dbg->wdata->state.lock, flags);
-	dbg->wdata->state.flags &= ~WIIPROTO_FLAG_DRM_LOCKED;
-	wiiproto_req_drm(dbg->wdata, (__u8) i);
-	if (i != WIIPROTO_REQ_NULL)
-		dbg->wdata->state.flags |= WIIPROTO_FLAG_DRM_LOCKED;
-	spin_unlock_irqrestore(&dbg->wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &dbg->wdata->state.lock) {
+		dbg->wdata->state.flags &= ~WIIPROTO_FLAG_DRM_LOCKED;
+		wiiproto_req_drm(dbg->wdata, (__u8) i);
+		if (i != WIIPROTO_REQ_NULL)
+			dbg->wdata->state.flags |= WIIPROTO_FLAG_DRM_LOCKED;
+	}
 
 	return len;
 }
@@ -172,7 +168,6 @@ static const struct file_operations wiidebug_drm_fops = {
 int wiidebug_init(struct wiimote_data *wdata)
 {
 	struct wiimote_debug *dbg;
-	unsigned long flags;
 
 	dbg = kzalloc_obj(*dbg);
 	if (!dbg)
@@ -186,9 +181,8 @@ int wiidebug_init(struct wiimote_data *wdata)
 	dbg->drm = debugfs_create_file("drm", S_IRUSR,
 			dbg->wdata->hdev->debug_dir, dbg, &wiidebug_drm_fops);
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->debug = dbg;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->debug = dbg;
 
 	return 0;
 
@@ -197,14 +191,12 @@ int wiidebug_init(struct wiimote_data *wdata)
 void wiidebug_deinit(struct wiimote_data *wdata)
 {
 	struct wiimote_debug *dbg = wdata->debug;
-	unsigned long flags;
 
 	if (!dbg)
 		return;
 
-	spin_lock_irqsave(&wdata->state.lock, flags);
-	wdata->debug = NULL;
-	spin_unlock_irqrestore(&wdata->state.lock, flags);
+	scoped_guard(spinlock_irqsave, &wdata->state.lock)
+		wdata->debug = NULL;
 
 	debugfs_remove(dbg->drm);
 	debugfs_remove(dbg->eeprom);
diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index dccb78bb3afd..3cd614466740 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -30,6 +30,7 @@
  * input devices.
  */
 
+#include <linux/cleanup.h>
 #include <linux/device.h>
 #include <linux/hid.h>
 #include <linux/input.h>
@@ -341,7 +342,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 	struct device *dev = &wdata->hdev->dev;
 	size_t namesz = strlen(dev_name(dev)) + 9;
 	struct led_classdev *led;
-	unsigned long flags;
 	char *name;
 	int ret;
 
@@ -364,9 +364,8 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 
 	/* enable LED1 to stop initial LED-blinking */
 	if (ops->arg == 0) {
-		spin_lock_irqsave(&wdata->state.lock, flags);
-		wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
-		spin_unlock_irqrestore(&wdata->state.lock, flags);
+		scoped_guard(spinlock_irqsave, &wdata->state.lock)
+			wiiproto_req_leds(wdata, WIIPROTO_FLAG_LED1);
 	}
 
 	return 0;
-- 
2.53.0

[PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes

[Rafael Passos]

Cleanup code in wiimote/led probe function, using the scoped cleanup.
This prevents mistakes in future changes to this function.

In wiimote_probe_clenaup, a few functions are safe to call without
checking. For the hid_hw calls, a new bit mask was introduced to track
probing state.

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c    | 68 ++++++++++++++++++-------------
 drivers/hid/hid-wiimote-modules.c | 17 ++++----
 drivers/hid/hid-wiimote.h         |  1 +
 3 files changed, 48 insertions(+), 38 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index d37d740a7162..b549a7d58d51 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1774,16 +1774,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
 /* Global id allocator for wii remotes */
 static DEFINE_IDA(wiimote_ida);
 
+#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
+#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
+
+static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
+{
+	if (!wdata)
+		return;
+
+	if (wdata->player_id)
+		ida_free(&wiimote_ida, wdata->player_id);
+
+	// safe, debugfs checks IS_ERR_OR_NULL
+	wiidebug_deinit(wdata);
+	// safe, checks dev for NULL
+	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
+	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
+	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
+		hid_hw_close(wdata->hdev);
+	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
+		hid_hw_stop(wdata->hdev);
+	kfree(wdata);
+}
+
+DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
+	__wiimote_probe_cleanup(_T))
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
-	struct wiimote_data *wdata;
 	int ret;
-	int player_id;
 
 	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
-	wdata = wiimote_create(hdev);
+	struct wiimote_data *wdata __free(wiimote_probe_cleanup) = wiimote_create(hdev);
 	if (!wdata) {
 		hid_err(hdev, "Can't alloc device\n");
 		return -ENOMEM;
@@ -1792,68 +1816,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "HID parse failed\n");
-		goto err;
+		return ret;
 	}
 
 	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
 	if (ret) {
 		hid_err(hdev, "HW start failed\n");
-		goto err;
+		return ret;
 	}
+	wdata->probe_state |= WIIMOTE_PROBE_HW_STARTED;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev, "cannot start hardware I/O\n");
-		goto err_stop;
+		return ret;
 	}
+	wdata->probe_state |= WIIMOTE_PROBE_HW_OPENED;
 
 	ret = device_create_file(&hdev->dev, &dev_attr_extension);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
-		goto err_close;
+		return ret;
 	}
 
 	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
-		goto err_ext;
+		return ret;
 	}
 
 	ret = wiidebug_init(wdata);
 	if (ret)
-		goto err_free;
+		return ret;
 
-	player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
+	int 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;
+		return ret;
 	}
-
 	wdata->player_id = player_id;
 
+
 	hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
 
 	/* schedule device detection */
 	wiimote_schedule(wdata);
-
+	retain_and_null_ptr(wdata);
 	return 0;
-
-err_free:
-	wiimote_destroy(wdata);
-	return ret;
-
-err_ext:
-	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
-err_close:
-	hid_hw_close(hdev);
-err_stop:
-	hid_hw_stop(hdev);
-err:
-	input_free_device(wdata->ir);
-	input_free_device(wdata->accel);
-	kfree(wdata);
-	return ret;
 }
 
 static void wiimote_hid_remove(struct hid_device *hdev)
diff --git a/drivers/hid/hid-wiimote-modules.c b/drivers/hid/hid-wiimote-modules.c
index 3cd614466740..47fa6a8ecdae 100644
--- a/drivers/hid/hid-wiimote-modules.c
+++ b/drivers/hid/hid-wiimote-modules.c
@@ -341,11 +341,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;
-	struct led_classdev *led;
 	char *name;
 	int ret;
 
-	led = kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
+	struct led_classdev *led __free(kfree) =
+		kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;
 
@@ -359,8 +359,12 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 
 	wdata->leds[ops->arg] = led;
 	ret = led_classdev_register(dev, led);
-	if (ret)
-		goto err_free;
+	if (ret) {
+		wdata->leds[ops->arg] = NULL;
+		return ret;
+	}
+
+	retain_and_null_ptr(led);
 
 	/* enable LED1 to stop initial LED-blinking */
 	if (ops->arg == 0) {
@@ -369,11 +373,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
 	}
 
 	return 0;
-
-err_free:
-	wdata->leds[ops->arg] = NULL;
-	kfree(led);
-	return ret;
 }
 
 static void wiimod_led_remove(const struct wiimod_ops *ops,
diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
index a53f72d5077e..6812efa589c9 100644
--- a/drivers/hid/hid-wiimote.h
+++ b/drivers/hid/hid-wiimote.h
@@ -154,6 +154,7 @@ struct wiimote_data {
 	struct timer_list timer;
 	struct wiimote_debug *debug;
 	__u8 player_id;
+	__u8 probe_state;
 
 	union {
 		struct input_dev *input;
-- 
2.53.0

[PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy

[Rafael Passos]

In wiimote_destroy, in the time between setting WIIPROTO_FLAG_EXITING
and hid_hw_stop, new events could be handled.
The new behaviour mirrors hid-playstation's output_worker_initialized
flag-gate (source of inspiration).

This issue was reported as a pre-existign race condition by sashiko.
I confirmed it is possible, but very unlikely. It could only happen if
shutdown is initiated by Linux, and the controller is being used.

Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index b549a7d58d51..c9638ec352e4 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -92,6 +92,9 @@ static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
 	 */
 
 	spin_lock_irqsave(&wdata->queue.lock, flags);
+	/* Do not schedule work if controller is exiting */
+	if ((wdata->state.flags & WIIPROTO_FLAG_EXITING))
+		goto out_unlock;
 
 	memcpy(wdata->queue.outq[wdata->queue.head].data, buffer, count);
 	wdata->queue.outq[wdata->queue.head].size = count;
-- 
2.53.0

[TEST] Patch used for testing the wiimote_probe_cleanup

[Rafael Passos]

[DO NOT APPLY THIS PATCH] :)
This is the patch I used to simulate each possible failure case
in the probing function.
I can say going it was worth it, as I was originally not gating
ida_free, and the struct would corrupt.

Below are the logs from my last run with this.
I added a space between each iteration, to help reading:

[   76.680204] wiimote 0005:057E:0330.0006: Probing wiimote iteration 1
[   76.680216] __wiimote_probe_cleanup: wdata NULL
[   76.680224] wiimote 0005:057E:0330.0006: probe with driver wiimote failed with error -12

[   81.745168] wiimote 0005:057E:0330.0007: Probing wiimote iteration 2
[   81.745312] wiimote 0005:057E:0330.0007: probe with driver wiimote failed with error -12

[   86.869452] wiimote 0005:057E:0330.0008: Probing wiimote iteration 3
[   86.869935] wiimote 0005:057E:0330.0008: unknown main item tag 0x0
[   86.869983] wiimote 0005:057E:0330.0008: probe with driver wiimote failed with error -12

[   92.001194] wiimote 0005:057E:0330.0009: Probing wiimote iteration 4
[   92.001684] wiimote 0005:057E:0330.0009: unknown main item tag 0x0
[   92.001905] wiimote 0005:057E:0330.0009: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[   92.002075] __wiimote_probe_cleanup: hid_hw_stop cleanup
[   92.002105] wiimote 0005:057E:0330.0009: probe with driver wiimote failed with error -12

[   97.129059] wiimote 0005:057E:0330.000A: Probing wiimote iteration 5
[   97.129528] wiimote 0005:057E:0330.000A: unknown main item tag 0x0
[   97.129757] wiimote 0005:057E:0330.000A: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[   97.129774] __wiimote_probe_cleanup: hid_hw_close cleanup
[   97.129934] __wiimote_probe_cleanup: hid_hw_stop cleanup
[   97.129962] wiimote 0005:057E:0330.000A: probe with driver wiimote failed with error -12

[  102.265442] wiimote 0005:057E:0330.000B: Probing wiimote iteration 6
[  102.265925] wiimote 0005:057E:0330.000B: unknown main item tag 0x0
[  102.266521] wiimote 0005:057E:0330.000B: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  102.266538] __wiimote_probe_cleanup: hid_hw_close cleanup
[  102.266814] __wiimote_probe_cleanup: hid_hw_stop cleanup
[  102.266844] wiimote 0005:057E:0330.000B: probe with driver wiimote failed with error -12

[  107.405172] wiimote 0005:057E:0330.000C: Probing wiimote iteration 7
[  107.405602] wiimote 0005:057E:0330.000C: unknown main item tag 0x0
[  107.405776] wiimote 0005:057E:0330.000C: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  107.405794] __wiimote_probe_cleanup: hid_hw_close cleanup
[  107.405953] __wiimote_probe_cleanup: hid_hw_stop cleanup
[  107.405981] wiimote 0005:057E:0330.000C: probe with driver wiimote failed with error -12

[  112.525411] wiimote 0005:057E:0330.000D: Probing wiimote iteration 8
[  112.525883] wiimote 0005:057E:0330.000D: unknown main item tag 0x0
[  112.526323] wiimote 0005:057E:0330.000D: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  112.526362] __wiimote_probe_cleanup: hid_hw_close cleanup
[  112.526511] __wiimote_probe_cleanup: hid_hw_stop cleanup
[  112.526535] wiimote 0005:057E:0330.000D: probe with driver wiimote failed with error -12

[  117.673238] wiimote 0005:057E:0330.000E: Probing wiimote iteration 9
[  117.673700] wiimote 0005:057E:0330.000E: unknown main item tag 0x0
[  117.673885] wiimote 0005:057E:0330.000E: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  117.673909] __wiimote_probe_cleanup: ida cleanup
[  117.673929] __wiimote_probe_cleanup: hid_hw_close cleanup
[  117.674081] __wiimote_probe_cleanup: hid_hw_stop cleanup
[  117.674105] wiimote 0005:057E:0330.000E: probe with driver wiimote failed with error -12

[  122.805206] wiimote 0005:057E:0330.000F: Probing wiimote iteration 10
[  122.805669] wiimote 0005:057E:0330.000F: unknown main item tag 0x0
[  122.805895] wiimote 0005:057E:0330.000F: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  122.805934] wiimote 0005:057E:0330.000F: New device registered (Wiimote 1)
[  122.805940] __wiimote_probe_cleanup: wdata NULL
[  122.841287] wiimote 0005:057E:0330.000F: detected device: Nintendo Wii Remote Plus (Gen 2) (1)
[  122.841759] input: Nintendo Wii Remote Accelerometer as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input33
[  122.844724] input: Nintendo Wii Remote IR as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input34
[  122.845091] input: Nintendo Wii Remote as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input32
[  122.965128] wiimote 0005:057E:0330.000F: detected extension: Nintendo Wii Motion Plus
[  122.965276] input: Nintendo Wii Remote Motion Plus as /devices/virtual/misc/uhid/0005:057E:0330.000F/input/input35
[  127.877355] wiimote 0005:057E:0330.000F: Device removed

[  130.497046] wiimote 0005:057E:0330.0010: Probing wiimote iteration 11
[  130.497524] wiimote 0005:057E:0330.0010: unknown main item tag 0x0
[  130.497838] wiimote 0005:057E:0330.0010: hidraw5: BLUETOOTH HID v0.01 Gamepad [Nintendo RVL-CNT-01-TR] on 64:bc:58:9a:b9:28
[  130.497869] wiimote 0005:057E:0330.0010: New device registered (Wiimote 1)
[  130.497874] __wiimote_probe_cleanup: wdata NULL
[  130.533040] wiimote 0005:057E:0330.0010: detected device: Nintendo Wii Remote Plus (Gen 2) (1)
[  130.533424] input: Nintendo Wii Remote Accelerometer as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input37
[  130.533579] input: Nintendo Wii Remote IR as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input38
[  130.533699] input: Nintendo Wii Remote as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input36
[  130.641239] wiimote 0005:057E:0330.0010: detected extension: Nintendo Wii Motion Plus
[  130.641385] input: Nintendo Wii Remote Motion Plus as /devices/virtual/misc/uhid/0005:057E:0330.0010/input/input39
[  134.317450] wiimote 0005:057E:0330.0010: Device removed


Signed-off-by: Rafael Passos <rafael@rcpassos.me>
---
 drivers/hid/hid-wiimote-core.c | 45 ++++++++++++++++++++++++++++++----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1779,52 +1779,77 @@ static DEFINE_IDA(wiimote_ida);
 
 static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
 {
-	if (!wdata)
+	if (!wdata) {
+		printk(KERN_INFO "%s: wdata NULL\n", __func__);
 		return;
+	}
 
-	if (wdata->player_id)
+	if (wdata->player_id){
+		printk(KERN_INFO "%s: ida cleanup\n", __func__);
 		ida_free(&wiimote_ida, wdata->player_id);
+	}
 
 	// safe, debugfs checks IS_ERR_OR_NULL
 	wiidebug_deinit(wdata);
 	// safe, checks dev for NULL
 	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
 	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
-	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
+	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED){
+		printk(KERN_INFO "%s: hid_hw_close cleanup\n", __func__);
 		hid_hw_close(wdata->hdev);
-	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
+	}
+	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED){
 		hid_hw_stop(wdata->hdev);
+		printk(KERN_INFO "%s: hid_hw_stop cleanup\n", __func__);
+	}
 	kfree(wdata);
 }
 
 DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
 	__wiimote_probe_cleanup(_T))
 
+static int debug_connect_iterate = 0;
+
 static int wiimote_hid_probe(struct hid_device *hdev,
 				const struct hid_device_id *id)
 {
 	int ret;
+	debug_connect_iterate++;
+
+	hid_info(hdev, "Probing wiimote iteration %d\n", debug_connect_iterate);
 
 	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
 
-	struct wiimote_data *wdata __free(wiimote_probe_cleanup) = wiimote_create(hdev);
+	struct wiimote_data *wdata __free(wiimote_probe_cleanup) = NULL;
+	if (debug_connect_iterate == 1)
+		return -ENOMEM;
+
+        wdata = wiimote_create(hdev);
 	if (!wdata) {
 		hid_err(hdev, "Can't alloc device\n");
 		return -ENOMEM;
 	}
 
+	if (debug_connect_iterate == 2)
+		return -ENOMEM;
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "HID parse failed\n");
 		return ret;
 	}
 
+	if (debug_connect_iterate == 3)
+		return -ENOMEM;
+
 	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
 	if (ret) {
 		hid_err(hdev, "HW start failed\n");
 		return ret;
 	}
 	wdata->probe_state |= WIIMOTE_PROBE_HW_STARTED;
+	if (debug_connect_iterate == 4)
+		return -ENOMEM;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
@@ -1832,22 +1857,30 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		return ret;
 	}
 	wdata->probe_state |= WIIMOTE_PROBE_HW_OPENED;
+	if (debug_connect_iterate == 5)
+		return -ENOMEM;
 
 	ret = device_create_file(&hdev->dev, &dev_attr_extension);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
 		return ret;
 	}
+	if (debug_connect_iterate == 6)
+		return -ENOMEM;
 
 	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
 	if (ret) {
 		hid_err(hdev, "cannot create sysfs attribute\n");
 		return ret;
 	}
+	if (debug_connect_iterate == 7)
+		return -ENOMEM;
 
 	ret = wiidebug_init(wdata);
 	if (ret)
 		return ret;
+	if (debug_connect_iterate == 8)
+		return -ENOMEM;
 
 	int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
 	if (player_id < 1) {
@@ -1856,6 +1889,8 @@ static int wiimote_hid_probe(struct hid_device *hdev,
 		return ret;
 	}
 	wdata->player_id = player_id;
+	if (debug_connect_iterate == 9)
+		return -ENOMEM;
 
 
 	hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
-- 
2.53.0

Re: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Functions mix `goto`-based error handling cleanup labels with scope-based cleanup (`scoped_guard()`), violating the cleanup subsystem architectural rule.
- [Low] Incorrect 4-space indentation within `scoped_guard` blocks instead of standard kernel tabs.

Pre-existing issues:
- [High] Use-after-free of `wdata` because `queue.worker` can be rescheduled after it is cancelled in `wiimote_destroy()`.
--

--- Patch [2]: [PATCH v2 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit 343c163a7f9f1368f62e298a4bac0953c054d9f3
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: replace spinlock pairs with scoped_guard
    
Cleanup code replacing manual lock/unlock with scoped guards.
This does not change any behavior, but makes it safer to modify.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 91c2cec35f6eb..d37d740a7162f 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -362,13 +363,12 @@ void wiiproto_req_rmem(struct wiimote_data *wdata, bool eeprom, __u32 offset,
>  int wiimote_cmd_write(struct wiimote_data *wdata, __u32 offset,
>  						const __u8 *wmem, __u8 size)
>  {
[ ... ]
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> -	wiiproto_req_wreg(wdata, offset, wmem, size);
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +	    wiimote_cmd_set(wdata, WIIPROTO_REQ_WMEM, 0);
> +	    wiiproto_req_wreg(wdata, offset, wmem, size);
> +	}

[Severity: Low]
this isn't a bug, but did the indentation get changed to 4 spaces here instead
of standard kernel tabs?

[ ... ]
> @@ -885,19 +874,19 @@ static void wiimote_init_detect(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> -	wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> -	wiiproto_req_status(wdata);
> -	spin_unlock_irq(&wdata->state.lock);
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
> +		wdata->state.devtype = WIIMOTE_DEV_UNKNOWN;
> +		wiimote_cmd_set(wdata, WIIPROTO_REQ_SREQ, 0);
> +		wiiproto_req_status(wdata);
> +	}
> +
>  
>  	ret = wiimote_cmd_wait_noint(wdata);
>  	if (ret)
>  		goto out_release;

[Severity: Medium]
Does this mix goto-based cleanup labels with scope-based cleanup?

The cleanup guidelines state that for a given routine, all resources requiring
cleanup should be converted to scope-based cleanup, or none of them should be.

This function (as well as wiimote_init_check() and wiimote_modules_load())
still uses goto-based cleanup like goto out_release; and goto error;
combined with the new scoped_guard() declarations.

[ ... ]
> @@ -1120,16 +1108,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
>  
>  	wiimote_cmd_acquire_noint(wdata);
>  
> -	spin_lock_irq(&wdata->state.lock);
> -
> -	/* get state snapshot that we will then work on */
> -	flags = wdata->state.flags;
> +	scoped_guard(spinlock_irq, &wdata->state.lock) {
>  
> -	/* disable event forwarding temporarily */
> -	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
> -	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
> +	    /* get state snapshot that we will then work on */
> +	    flags = wdata->state.flags;

[Severity: Low]
this isn't a bug, but it looks like the contents of this scoped_guard block 
are also indented with 4 spaces instead of a tab.

[ ... ]
> @@ -1774,14 +1749,11 @@ static struct wiimote_data *wiimote_create(struct hid_device *hdev)
>  
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
> -	unsigned long flags;
> -
>  	wiidebug_deinit(wdata);
>  
>  	/* prevent init_worker from being scheduled again */
> -	spin_lock_irqsave(&wdata->state.lock, flags);
> -	wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> -	spin_unlock_irqrestore(&wdata->state.lock, flags);
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock)
> +		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
>  
>  	cancel_work_sync(&wdata->init_worker);
>  	timer_shutdown_sync(&wdata->timer);

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
in wiimote_destroy() have a use-after-free race condition?

Looking at the rest of wiimote_destroy(), cancel_work_sync(&wdata->queue.worker)
is called before hid_hw_stop(wdata->hdev). If a HID interrupt arrives
immediately after the worker is cancelled:

HID core calls wiimote_hid_event() ->
    handler_status_K() ->
    wiiproto_req_drm() ->
    wiimote_queue()

wiimote_queue() then calls schedule_work(&wdata->queue.worker) without
checking if the device is exiting.

Since hid_hw_stop() hasn't been called yet, the new worker is queued. After
hid_hw_stop() and kfree(wdata) complete, the queued worker can execute and
dereference the freed wdata.

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

Re: [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The new scoped cleanup function `__wiimote_probe_cleanup()` fails to cancel async workers, timers, and loaded modules on error, leading to a Use-After-Free and dangling registered devices.
- [Medium] Truncation of `player_id` from `int` to `__u8` causes IDA double frees, leaked IDs, and an out-of-bounds array access.
--

--- Patch [3]: [PATCH v2 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes ---
commit 29027bf7f674068d4630217f5005fa7c42f22f18
Author: Rafael Passos <rafael@rcpassos.me>
Subject: HID: wiimote: use scoped cleanup in wiimote and led probes

This patch refactors the cleanup paths in the wiimote and led probe functions
to use scoped cleanup macros. It also introduces a state mask to track the
hardware probing progress.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index d37d740a7162f..b549a7d58d512 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1774,16 +1774,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  /* Global id allocator for wii remotes */
>  static DEFINE_IDA(wiimote_ida);
>  
> +#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
> +#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
> +
> +static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
> +{
> +	if (!wdata)
> +		return;
> +
> +	if (wdata->player_id)
> +		ida_free(&wiimote_ida, wdata->player_id);
> +
> +	// safe, debugfs checks IS_ERR_OR_NULL
> +	wiidebug_deinit(wdata);
> +	// safe, checks dev for NULL
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
> +	kfree(wdata);
> +}

[Severity: High]
Does this new cleanup function safely handle asynchronous teardown?

In the original code, failures later in the probe function would route to
the err_free label which called wiimote_destroy(). That function cleanly
cancelled wdata->init_worker and unloaded any registered modules.

If a hardware event occurs during probe:

wiimote_hid_event()
   queues wdata->init_worker

And then an allocation fails (e.g. in wiidebug_init or ida_alloc_min):

__wiimote_probe_cleanup()
   hid_hw_stop(wdata->hdev)
   kfree(wdata)

Will the queued worker execute after wdata has been freed?

[ ... ]

> @@ -1792,68 +1816,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	ret = wiidebug_init(wdata);
>  	if (ret)
> -		goto err_free;
> +		return ret;
>  
> -	player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	int 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;
> +		return ret;
>  	}
> -
>  	wdata->player_id = player_id;

[Severity: Medium]
Could this assignment result in truncation if the allocated ID is larger
than 255?

ida_alloc_min() returns an int, but wdata->player_id is defined as a __u8.
If an ID exceeds 255, the truncation could result in 0.

If player_id becomes 0, could it cause an out-of-bounds read later?
Looking at wiimote_modules_load():

wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);

This might evaluate to player_leds[-1].

Additionally, truncation could lead to multiple devices sharing the same
truncated ID, which might cause IDA double frees when devices are removed.

[ ... ]

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Integer truncation of IDA allocation results in out-of-bounds read, IDA resource leak, and kernel WARN.
- [Medium] Race condition causing use-before-initialization of `player_id`, leading to an out-of-bounds read.
- [Medium] Mixing `goto`-based error handling and `scoped_guard()` in the same function violates kernel cleanup subsystem rules.
--

--- Patch [1]: [PATCH v2 1/4] HID: wiimote: turn on the LEDs indicating the controller id ---
commit 49992cb763819a5276c65ada6594868205443ccd
Author: Rafael Passos <rafael@rcpassos.me>

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).

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

[ ... ]

> @@ -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);
> +
> +	/* after loading all modules, set the LED for the player ID cycling from 1 to 4*/
> +	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
> +	}
> +
>  	return;
>  
>  error:

[Severity: Medium]
Does using scoped_guard() in this function violate the cleanup subsystem
guidelines?

The wiimote_modules_load() function appears to use a goto-based failure path
(goto error;), but introduces a scoped_guard() scope-based cleanup macro.

The cleanup subsystem documentation indicates that mixing goto-based error
handling and scoped_guard() in the same function can create confusing
ownership semantics and should be avoided.

[ ... ]

> @@ -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: High]
Can this assignment truncate the allocated ID?

The player_id variable returned by ida_alloc_min() is an int, but
wdata->player_id is defined as a __u8 in struct wiimote_data.

If enough devices are created (e.g., via /dev/uhid) to allocate ID 256,
the value would truncate to 0.

When this 0 is later evaluated in wiimote_modules_load():

    player_leds[(wdata->player_id - 1) % 4]

The expression (0 - 1) % 4 evaluates to -1, which would lead to an
out-of-bounds array read on player_leds[-1].

Additionally, during device removal in wiimote_hid_remove(), calling
ida_free(&wiimote_ida, wdata->player_id) with the truncated 0 would
trigger a WARN_ON_ONCE in the IDA core (since ID 0 was never allocated)
and permanently leak the originally allocated ID.

[Severity: Medium]
Is there a race condition where wdata->player_id is used before it is
initialized?

The hardware I/O is started and opened via hid_hw_open() earlier in
wiimote_hid_probe(), before the player_id is allocated and assigned here.

If the device immediately sends an event, could a background worker
executing wiimote_modules_load() read wdata->player_id while it is still 0?

This would cause the same out-of-bounds read on player_leds[-1] described
above.

[ ... ]

> 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;
>  
>  	union {
>  		struct input_dev *input;

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

Re: [PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Module cleanup commands (e.g., rumble off, LEDs off) are dropped during driver unload, leaving the controller hardware in an incorrect or stuck state.
--

--- Patch [4]: [PATCH v2 4/4] HID: wiimote: fix uaf when hid events are handled during destroy ---
commit 62bea240ecf4b4da704b1580ca426a1f9c83501a
Author: Rafael Passos <rafael@rcpassos.me>

HID: wiimote: fix uaf when hid events are handled during destroy

This commit adds a flag check to wiimote_queue() to drop events
if the driver is in the process of unloading. This mitigates a
use-after-free issue when handling events during destroy.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index b549a7d58d512..c9638ec352e44 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -92,6 +92,9 @@ static void wiimote_queue(struct wiimote_data *wdata, const __u8 *buffer,
>  	 */
>  
>  	spin_lock_irqsave(&wdata->queue.lock, flags);
> +	/* Do not schedule work if controller is exiting */
> +	if ((wdata->state.flags & WIIPROTO_FLAG_EXITING))
> +		goto out_unlock;

[Severity: Medium]
Will this aggressively drop hardware cleanup commands during driver unload?

Looking at wiimote_destroy(), the exiting flag is set before calling
wiimote_modules_unload():

drivers/hid/hid-wiimote-core.c:wiimote_destroy() {
    ...
	/* prevent init_worker from being scheduled again */
	scoped_guard(spinlock_irqsave, &wdata->state.lock)
		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
    ...
	wiimote_modules_unload(wdata);
    ...
}

If a user unbinds the driver or disconnects the controller while rumble is
active, wiimote_modules_unload() invokes wiimod_rumble_remove() to send
a command to turn the rumble off.

Will this check in wiimote_queue() unexpectedly drop the rumble-off
packet, leaving the physical controller vibrating indefinitely?

>  
>  	memcpy(wdata->queue.outq[wdata->queue.head].data, buffer, count);
>  	wdata->queue.outq[wdata->queue.head].size = count;

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