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

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 <[email protected]>

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/[email protected]?part=4