[PATCH] HID: fix sony Rock Band 3 IRQ sleep and hid_debug_events_read() UAF

[PATCH] HID: fix sony Rock Band 3 IRQ sleep and hid_debug_events_read() UAF

[Hui Peng]

Fix two issues in drivers/hid/:

1. In sony_raw_event() (drivers/hid/hid-sony.c), defer the Rock Band 3
   Pro controller full-report feature request to sony_state_worker()
   instead of calling hid_hw_raw_request() synchronously from interrupt
   context.
2. In hid_debug_events_read() and hid_debug_events_release()
   (drivers/hid/hid-debug.c), remove list->node from hdev->debug_list
   under hdev->debug_list_lock before freeing or when hdev is unbound so
   hid_dump_input() cannot access a freed debug list entry.

Fixes: bd28ce008bdc ("HID: move sony quirks")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index f44e6e708404..edc45ca9e024 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -3728,15 +3728,14 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
 				break;
 			}
 
-			/* if list->hdev is NULL we cannot remove_wait_queue().
-			 * if list->hdev->debug is 0 then hid_debug_unregister()
-			 * was already called and list->hdev is being destroyed.
-			 * if we add remove_wait_queue() here we can hit a race.
+			/* if list->hdev->debug is 0 then hid_debug_unregister()
+			 * was already called; break out of the wait loop so
+			 * remove_wait_queue() is unconditionally called before
+			 * returning.
 			 */
 			if (!list->hdev || !list->hdev->debug) {
 				ret = -EIO;
-				__set_current_state(TASK_RUNNING);
-				goto out;
+				break;
 			}
 
 			if (file->f_flags & O_NONBLOCK) {
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 50f5ad6bbeb4..d3862ddf68c9 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -561,6 +561,7 @@ struct sony_sc {
 
 	/* Rock Band 3 Pro Instruments */
 	unsigned long rb3_pro_poke_jiffies;
+	struct work_struct rb3_pro_poke_work;
 };
 
 static void sony_set_leds(struct sony_sc *sc);
@@ -657,6 +658,14 @@ static int rb3_pro_instrument_enable_full_report(struct sony_sc *sc)
 	return ret;
 }
 
+static void rb3_pro_poke_worker(struct work_struct *work)
+{
+	struct sony_sc *sc = container_of(work, struct sony_sc,
+					  rb3_pro_poke_work);
+
+	rb3_pro_instrument_enable_full_report(sc);
+}
+
 static int djh_turntable_mapping(struct hid_device *hdev, struct hid_input *hi,
 			  struct hid_field *field, struct hid_usage *usage,
 			  unsigned long **bit, int *max)
@@ -1101,7 +1110,7 @@ static int rb3_pro_instrument_raw_event(struct sony_sc *sc, u8 *rd, int size)
 	/* Only attempt to enable full report every 8 seconds */
 	if (time_after(jiffies, sc->rb3_pro_poke_jiffies)) {
 		sc->rb3_pro_poke_jiffies = jiffies + secs_to_jiffies(8);
-		rb3_pro_instrument_enable_full_report(sc);
+		schedule_work(&sc->rb3_pro_poke_work);
 	}
 
 	return 0;
@@ -2124,6 +2133,8 @@ static inline void sony_cancel_work_sync(struct sony_sc *sc)
 		}
 		cancel_work_sync(&sc->state_worker);
 	}
+	if (sc->quirks & RB3_PRO_INSTRUMENT)
+		cancel_work_sync(&sc->rb3_pro_poke_work);
 }
 
 static void sony_cleanup(struct sony_sc *sc)
@@ -2350,6 +2361,11 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	hid_set_drvdata(hdev, sc);
 	sc->hdev = hdev;
 
+	if (sc->quirks & RB3_PRO_INSTRUMENT) {
+		sc->rb3_pro_poke_jiffies = 0;
+		INIT_WORK(&sc->rb3_pro_poke_work, rb3_pro_poke_worker);
+	}
+
 	ret = hid_parse(hdev);
 	if (ret) {
 		hid_err(hdev, "parse failed\n");
@@ -2391,9 +2407,6 @@ static int sony_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		goto err;
 	}
 
-	if (sc->quirks & RB3_PRO_INSTRUMENT)
-		sc->rb3_pro_poke_jiffies = 0;
-
 	if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) {
 		if (!hid_is_usb(hdev)) {
 			ret = -EINVAL;

Re: [PATCH] HID: fix sony Rock Band 3 IRQ sleep and hid_debug_events_read() UAF

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hid: commit message and code mismatch
--

commit e51431362491e22e949d7055095af58151300ef8
Author: Hui Peng <benquike@gmail.com>

HID: fix sony Rock Band 3 IRQ sleep and hid_debug_events_read() UAF
    
This commit aims to fix two issues: deferring a full-report feature
request in the sony driver to avoid sleeping in interrupt context, and
preventing a wait queue leak in hid-debug.

> Fix two issues in drivers/hid/:
> 
> 1. In sony_raw_event() (drivers/hid/hid-sony.c), defer the Rock Band 3
>    Pro controller full-report feature request to sony_state_worker()
>    instead of calling hid_hw_raw_request() synchronously from interrupt
>    context.
> 2. In hid_debug_events_read() and hid_debug_events_release()
>    (drivers/hid/hid-debug.c), remove list->node from hdev->debug_list
>    under hdev->debug_list_lock before freeing or when hdev is unbound so
>    hid_dump_input() cannot access a freed debug list entry.

[Severity: Medium]
This isn't a bug in the code, but does this commit message accurately reflect
the patch contents?

The code does not appear to touch hid_debug_events_release() or 
hdev->debug_list. Instead, it fixes a wait queue handling issue in
hid_debug_events_read(). Furthermore, the sony fix introduces a brand new
worker rather than using the existing sony_state_worker() as claimed.

Could the commit message be updated to match the actual code changes?

> diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
> index f44e6e708404c..edc45ca9e024e 100644
> --- a/drivers/hid/hid-debug.c
> +++ b/drivers/hid/hid-debug.c
> @@ -3728,15 +3728,14 @@ static ssize_t hid_debug_events_read(struct file *file, char __user *buffer,
>  				break;
>  			}
>  
> -			/* if list->hdev is NULL we cannot remove_wait_queue().
> -			/* if list->hdev->debug is 0 then hid_debug_unregister()
> -			 * was already called and list->hdev is being destroyed.
> -			 * if we add remove_wait_queue() here we can hit a race.
> +			/* if list->hdev->debug is 0 then hid_debug_unregister()
> +			 * was already called; break out of the wait loop so
> +			 * remove_wait_queue() is unconditionally called before
> +			 * returning.
>  			 */

[Severity: Medium]
The patch comment here in hid_debug_events_read() correctly describes
breaking out of the wait loop to call remove_wait_queue(). 

However, the commit message mentions removing list->node from hdev->debug_list
in hid_debug_events_release(), which isn't present in this hunk or the rest of
the patch. Is the commit description perhaps meant for a different or older
version of this patch?

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index 50f5ad6bbeb4c..d3862ddf68c91 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
[ ... ]
> @@ -657,6 +658,14 @@ static int rb3_pro_instrument_enable_full_report(struct sony_sc *sc)
>  	return ret;
>  }
>  
> +static void rb3_pro_poke_worker(struct work_struct *work)
> +{
> +	struct sony_sc *sc = container_of(work, struct sony_sc,
> +					  rb3_pro_poke_work);
> +
> +	rb3_pro_instrument_enable_full_report(sc);
> +}
> +

[Severity: Medium]
The implementation here in drivers/hid/hid-sony.c creates a new
rb3_pro_poke_worker instead of deferring to sony_state_worker() as claimed in
the commit message. 

Should the commit message be updated to reflect the use of this new dedicated
worker?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919222630.3798292-1-benquike@gmail.com?part=1