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

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

[Rafael Passos]

Hi,
This patchset contains one feature change, two cleanup patches + 1 uaf fix.

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.
I also used the __free scope cleanup in the wiimote and LED probe functions.
The wiimote_probe required a new state tracker bitmask. Trivial for the LED.
Lastly, I fixed a pre-existing uaf pointed out by sashiko in V1, using
the driver for the playstation controller as a reference.

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
      Patch used for testing this:
      https://lore.kernel.org/linux-input/20260715213513.3929001-1-rafael@rcpassos.me/
    (4/4) *new patch* :
    - sashiko found a pre-existing uaf. Unlikely, but correct.
      implemented using the playstation driver as an inspiration

V2: https://lore.kernel.org/linux-input/20260710153456.2093889-1-rafael@rcpassos.me/
Changes from v2:
    (2/4):
    - join the last two locks into a single scoped_guard lock in wiimote_modules_load

Notes on Sashiko reviews for V2:
    - controller state on driver unload: it would be funny if the
      controller would stay vibrating as suggested. I forced this case
      dropping the connection from kernel in the dirty state, but the
      controller just shuts down.
    - mixed goto/scoped cleanup: there is scoped locking and goto, not
      scoped cleanup. I think this is fine.
    - integer/u8 truncation in player_id: would need 256+ controllers.
      Not realistic. Even Bluetooth would refuse this.


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    | 339 ++++++++++++++++--------------
 drivers/hid/hid-wiimote-debug.c   |  50 ++---
 drivers/hid/hid-wiimote-modules.c |  24 +--
 drivers/hid/hid-wiimote.h         |   2 +
 4 files changed, 216 insertions(+), 199 deletions(-)

-- 
2.53.0

[PATCH v3 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 63c4fa8fbb9b6..48830f2ffcb50 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);
+
+	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]);
+	}
+
 	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 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;
-- 
2.53.0

[PATCH v3 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    | 224 +++++++++++++-----------------
 drivers/hid/hid-wiimote-debug.c   |  50 +++----
 drivers/hid/hid-wiimote-modules.c |   7 +-
 3 files changed, 121 insertions(+), 160 deletions(-)

diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
index 48830f2ffcb50..762b3c383194e 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)
@@ -675,11 +673,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
 			goto error;
 	}
 
-	spin_lock_irq(&wdata->state.lock);
-	wdata->state.devtype = devtype;
-	spin_unlock_irq(&wdata->state.lock);
-
 	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
+		wdata->state.devtype = devtype;
 		/* after loading modules, set the Player ID LED cycling from 1 to 4*/
 		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
 	}
@@ -703,13 +698,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 +729,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 +740,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 +761,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 +772,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 +785,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 +872,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 +897,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 +949,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 +966,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 +991,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 +1017,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 +1047,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 +1106,15 @@ static void wiimote_init_hotplug(struct wiimote_data *wdata)
 
 	wiimote_cmd_acquire_noint(wdata);
 
-	spin_lock_irq(&wdata->state.lock);
+	scoped_guard(spinlock_irq, &wdata->state.lock) {
 
-	/* get state snapshot that we will then work on */
-	flags = wdata->state.flags;
+	    /* get state snapshot that we will then work on */
+	    flags = wdata->state.flags;
 
-	/* disable event forwarding temporarily */
-	wdata->state.flags &= ~WIIPROTO_FLAG_EXT_ACTIVE;
-	wdata->state.flags &= ~WIIPROTO_FLAG_MP_ACTIVE;
-
-	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 +1137,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,28 +1176,26 @@ 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);
 	}
 
-	/* request status report for hotplug state updates */
-	wiiproto_req_status(wdata);
-
-	spin_unlock_irq(&wdata->state.lock);
-
 	hid_dbg(wdata->hdev, "detected extensions: MP: %d EXT: %d\n",
 		wdata->state.mp, wdata->state.exttype);
 }
@@ -1244,11 +1226,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 +1617,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 +1624,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 +1643,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 +1694,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 +1747,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 5f74917781f28..df6f830ee791c 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 dccb78bb3afd6..3cd6144667404 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 v3 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 762b3c383194e..31ee86affc553 100644
--- a/drivers/hid/hid-wiimote-core.c
+++ b/drivers/hid/hid-wiimote-core.c
@@ -1772,16 +1772,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;
@@ -1790,68 +1814,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 3cd6144667404..47fa6a8ecdaef 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 a53f72d5077ef..6812efa589c93 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 v3 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 31ee86affc553..067db8b8a56d1 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

Re: [PATCH v3 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] Mixing `goto` error handling with `scoped_guard` cleanup helpers in the same function.
- [Medium] Incomplete refactoring and unfulfilled commit message claims regarding lock replacements.

Pre-existing issues:
- [High] Race condition in teardown sequence allows re-scheduling of canceled work, leading to Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/4] HID: wiimote: replace spinlock pairs with scoped_guard ---
commit 20054d5c329a400384a729c4fe3021c525d804b5
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.

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.

[Severity: Medium]
Is this statement accurate? It appears that drivers/hid/hid-wiimote-modules.c
was mostly skipped during this refactoring, leaving over 30 manual
spin_lock_irqsave() and spin_lock_irq() calls on wdata->state.lock in
functions like wiimod_rumble_worker() and wiimod_battery_get_property().

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 48830f2ffcb50..762b3c383194e 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
[ ... ]
> @@ -675,11 +673,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  			goto error;
>  	}
>  
> -	spin_lock_irq(&wdata->state.lock);
> -	wdata->state.devtype = devtype;
> -	spin_unlock_irq(&wdata->state.lock);
> -
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock) {
> +		wdata->state.devtype = devtype;
>  		/* 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]
Does this violate the cleanup helper guidelines? This function uses a
"goto error" label for manual error handling earlier in the function,
but now introduces a scoped_guard(). The subsystem guidelines typically
forbid mixing goto-based error handling and scope-based cleanup helpers
in the same function to avoid confusing ownership semantics. This also
happens in wiimote_init_detect() and wiimote_init_check().

[ ... ]
> @@ -1774,14 +1747,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 is a pre-existing issue, but does the teardown sequence later in this
function have a race condition? 

In wiimote_destroy(), cancel_work_sync() is called on wdata->queue.worker
before the hardware transport is stopped via hid_hw_stop():

	cancel_work_sync(&wdata->queue.worker);
	hid_hw_close(wdata->hdev);
	hid_hw_stop(wdata->hdev);
	kfree(wdata);

If an incoming hardware event arrives in the window after cancel_work_sync()
but before hid_hw_stop(), it can be processed by wiimote_hid_event(). This
could trigger a status reply that queues data and re-schedules the worker via
schedule_work(&wdata->queue.worker).

Since the worker can be re-enqueued after it was intended to be canceled,
could it execute after kfree(wdata) and cause a use-after-free? Should
the hardware source be stopped before canceling works that the hardware
can schedule?

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

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

[sashiko-bot]

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 <rafael@rcpassos.me>

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.

> 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;

[ ... ]

> 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

[ ... ]

> @@ -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?

[ ... ]

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

[ ... ]

> @@ -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

Re: [PATCH v3 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 `__wiimote_probe_cleanup` scoped cleanup function fails to cancel asynchronous work queues (`init_worker`, `queue.worker`), shut down timers, and unload modules before freeing `wdata`, leading to a use-after-free and memory leaks on the probe error path.
- [High] An out-of-bounds array read occurs in `wiimote_modules_load()` due to integer promotion of the uninitialized `player_id`, causing the index calculation to yield -1.
--

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

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

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 762b3c383194e..31ee86affc553 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c

[ ... ]

> @@ -1772,16 +1772,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 missing cancellation cause a use-after-free or memory leak if the
probe fails later?

During wiimote_hid_probe(), after hid_hw_start() and hid_hw_open() succeed,
the hardware is active and can generate HID events. These events are
processed by wiimote_hid_event(), which can asynchronously schedule
wdata->init_worker and wdata->queue.worker.

If a subsequent probe step like ida_alloc_min() fails, this cleanup function
runs and frees wdata without cancelling the work queues (cancel_work_sync()),
shutting down timers (timer_shutdown_sync()), or unloading modules.

If a worker accesses wdata after it has been freed, could it trigger a kernel
crash?

> +}
> +
> +DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
> +	__wiimote_probe_cleanup(_T))
> +

[ ... ]

> @@ -1790,68 +1814,54 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>  	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;

[Severity: High]
Can an event arriving here before player_id is initialized cause an
out-of-bounds read?

If the device sends a status report or hotplug event immediately after
hid_hw_open() completes, wiimote_hid_event() triggers and schedules
wdata->init_worker. 

The worker calls wiimote_init_detect() -> wiimote_init_set_type() ->
wiimote_modules_load().

Because wdata->player_id is only populated later in this function, it is
still 0 when wiimote_modules_load() is called.

In wiimote_modules_load(), the calculation (wdata->player_id - 1) promotes to
int and evaluates to -1. The modulo operator (-1 % 4) yields -1, leading to
an out-of-bounds access on the player_leds array:

drivers/hid/hid-wiimote-core.c:wiimote_modules_load() {
    ...
    wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
    ...
}

Could this lead to an infoleak or kernel oops?

>  
>  	ret = device_create_file(&hdev->dev, &dev_attr_extension);

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch prevents essential hardware cleanup commands (such as turning off the rumble motor) from being sent to the controller during driver unbind.
- [Medium] Skipping `wiimote_cmd_abort()` when the controller is exiting causes concurrent synchronous callers to block for a 1-second timeout, needlessly delaying device teardown.
--

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

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

This patch aims to prevent use-after-free races during device teardown by
dropping new outgoing events in wiimote_queue() if the controller is in the
exiting state.

> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 31ee86affc553..067db8b8a56d1 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: High]
Will this unconditionally drop all hardware cleanup commands during driver
unbind?

When wiimote_destroy() runs, it sets WIIPROTO_FLAG_EXITING before calling
wiimote_modules_unload(). The module remove handlers, such as
wiimod_rumble_remove(), attempt to gracefully shut down hardware features
like turning off the rumble motor via wiiproto_req_rumble().

Because this check now drops all packets when EXITING is set, doesn't this
prevent those cleanup commands from reaching the hardware, potentially leaving
the rumble motor running indefinitely?

[Severity: Medium]
Does skipping the out_error label here introduce a teardown delay?

If a concurrent synchronous caller like wdata->init_worker is running when
wiimote_destroy() sets the EXITING flag, its packets will be dropped here.
By jumping directly to out_unlock, this bypasses wiimote_cmd_abort() at
the out_error label.

Without the abort to wake up waiters, won't wiimote_cmd_wait_noint() block
for its full 1-second timeout (HZ) while wiimote_destroy() waits in
cancel_work_sync(&wdata->init_worker)?

>  
>  	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/20260729164928.1138468-1-rafael@rcpassos.me?part=4