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

[sashiko-bot@kernel.org]

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