Re: [PATCH] HID: sony: fix UAF of ghl_poke_timer / ghl_urb at driver unbind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the patch fails to correctly fix the use-after-free race condition because it uses `usb_kill_urb()` instead of `usb_poison_urb()` or a reversed teardown sequence. `usb_kill_urb()` does not permanently prevent URB submissions; it allows the pending timer to submit the URB immediately after it returns.
--

--- Patch [1]: [PATCH] HID: sony: fix UAF of ghl_poke_timer / ghl_urb at driver unbind ---
commit 922f81b4c171a572e240262070ec65197e21cd83
Author: Doruk Tan Ozturk <doruk@0sec.ai>

HID: sony: fix UAF of ghl_poke_timer / ghl_urb at driver unbind

This commit changes the driver teardown sequence for Guitar Hero Live
dongles to fix a use-after-free during driver unbind. It attempts to kill
the in-flight URB before shutting down the timer to prevent the URB
completion from re-arming the timer on freed memory.

Fixes: cc894ac55360 ("HID: sony: support for ghlive ps3/wii u dongles")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index e75246d29e16d..b7b9b705b9e1c 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -2424,7 +2424,19 @@ static void sony_remove(struct hid_device *hdev)
>  	struct sony_sc *sc = hid_get_drvdata(hdev);
>  
>  	if (sc->quirks & (GHL_GUITAR_PS3WIIU | GHL_GUITAR_PS4)) {
> -		timer_delete_sync(&sc->ghl_poke_timer);
> +		/*
> +		 * ghl_magic_poke() resubmits sc->ghl_urb from the poke timer
> +		 * and its completion ghl_magic_poke_cb() re-arms the timer, so
> +		 * an in-flight URB can re-arm the timer after a plain
> +		 * timer_delete_sync() (a no-op while the URB is in flight and
> +		 * the timer is not pending).  sc is devm-allocated and freed
> +		 * once sony_remove() returns, so the re-armed timer would fire
> +		 * on freed memory.  Kill the URB first so no completion can
> +		 * re-arm the timer, then timer_shutdown_sync() to drain it and
> +		 * permanently block further re-arming.
> +		 */
> +		usb_kill_urb(sc->ghl_urb);
> +		timer_shutdown_sync(&sc->ghl_poke_timer);
> +		usb_free_urb(sc->ghl_urb);

[Severity: Critical]
This is a pre-existing issue, but does this teardown order still allow a
use-after-free race condition?

Because usb_kill_urb() does not permanently prevent URB submissions, it allows
the pending timer to submit the URB immediately after it returns.

Could this sequence occur if a user unbinds the driver while the URB is not
in flight and the ghl_poke_timer is pending:

1. usb_kill_urb() executes and returns immediately, decrementing urb->reject
   back to 0.

2. Before timer_shutdown_sync() executes, the timer expires and its callback
   ghl_magic_poke() successfully submits the URB.

3. timer_shutdown_sync() then waits for the timer callback to finish and marks
   the timer as shutdown, but the URB is now in flight.

4. The driver teardown finishes and frees sc.

5. Later, the URB completes and triggers the use-after-free by accessing the
   freed sc.

Would it be safer to use usb_poison_urb() instead to permanently block
submissions?

>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724134236.34304-1-doruk@0sec.ai?part=1