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

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

[Doruk Tan Ozturk]

For GHL (Guitar Hero Live) dongles, sony_probe() arms a periodic timer:
ghl_magic_poke() (the timer callback) submits sc->ghl_urb, and the URB
completion ghl_magic_poke_cb() re-arms the timer with mod_timer().

sony_remove() drained the timer with timer_delete_sync() and then freed
the URB with usb_free_urb():

	timer_delete_sync(&sc->ghl_poke_timer);
	usb_free_urb(sc->ghl_urb);

timer_delete_sync() does not block re-arming, and while the URB is in
flight the timer is not pending, so the sync delete is a no-op. A URB
completion that runs after the delete re-arms the timer, and usb_free_urb()
only drops a reference -- it does not kill an in-flight URB. sc is
allocated with devm_kzalloc() and freed once sony_remove() returns, so the
re-armed ghl_poke_timer (embedded in sc) then fires on freed memory, a
use-after-free from timer softirq. This is a disconnect/rmmod race.

Poison the URB first, then shut the timer down, before freeing the URB.
usb_poison_urb() kills any in-flight URB and permanently rejects further
submissions, so a poke timer that is still pending cannot re-submit the
URB from ghl_magic_poke() in the window before timer_shutdown_sync() runs.
usb_kill_urb() would not suffice: it only cancels the in-flight URB and
leaves it submittable once it returns, so the pending timer could
re-submit it and put a fresh URB in flight over the freed sc.
timer_shutdown_sync() then drains any last callback and blocks re-arming.
The probe error path is unaffected: it is only reached before the timer
is armed.

Reproduced under KASAN on next-20260710 via dummy_hcd + raw-gadget
emulation of the GHL PS4 dongle (VID 0x1430 / PID 0x07bb): hid-sony binds
and arms the poke timer, the poke URB is held in flight, the driver is
unbound (freeing sc), then the URB is released. The completion re-arms the
timer on the freed sc, and the re-armed timer fires ~8 s later:

  BUG: KASAN: slab-use-after-free in ghl_magic_poke+0x98/0xb0
  Read of size 8 at addr ffff88810b02fd50 by task swapper/0/0
   ghl_magic_poke+0x98/0xb0
   call_timer_fn+0x35/0x2b0
   __run_timers+0x69c/0x9a0
   run_timer_softirq+0x173/0x2a0
  Allocated by task 169:  sony_probe
  Freed by task 338:      devres_release_group <- hid_device_remove (sony_remove)

Found by 0sec (https://0sec.ai) using automated source analysis.

Fixes: cc894ac55360 ("HID: sony: support for ghlive ps3/wii u dongles")
Cc: stable@vger.kernel.org
Assisted-by: 0sec:multi-model
Signed-off-by: Doruk Tan Ozturk <doruk@0sec.ai>
---
v2: use usb_poison_urb() instead of usb_kill_urb(). usb_kill_urb() only
    cancels the in-flight URB and leaves it submittable once it returns,
    so a still-pending poke timer could re-submit between the kill and
    timer_shutdown_sync() and race the free (Sashiko review). Inline
    comment kept terse; rationale in the log.
 drivers/hid/hid-sony.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 253fff4066eb..112d1deaf3f3 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -2431,7 +2431,9 @@ 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);
+		/* poison, not kill: a pending timer must not re-submit during teardown */
+		usb_poison_urb(sc->ghl_urb);
+		timer_shutdown_sync(&sc->ghl_poke_timer);
 		usb_free_urb(sc->ghl_urb);
 	}
 
-- 
2.43.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Error paths in `sony_probe()` fail to remove the device context from `sony_device_list`, leading to a global use-after-free.
- [High] System suspend breaks the periodic keep-alive timer, permanently disconnecting the GHL guitar dongle.
- [Low] URB DMA buffers are allocated using `devm_kzalloc()`, violating DMA API cache-line alignment requirements.
--

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

    HID: sony: fix UAF of ghl_poke_timer / ghl_urb at driver unbind
    
    For GHL (Guitar Hero Live) dongles, sony_probe() arms a periodic timer
    that submits a URB, which on completion re-arms the timer.
    
    sony_remove() drained the timer with timer_delete_sync() and then freed
    the URB, which did not block re-arming or kill in-flight URBs. This allowed
    the timer to re-arm on freed memory, causing a use-after-free.

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index e75246d29e16d..77ab30303080e 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -2424,7 +2424,9 @@ 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);
> +		/* poison, not kill: a pending timer must not re-submit during teardown */
> +		usb_poison_urb(sc->ghl_urb);
> +		timer_shutdown_sync(&sc->ghl_poke_timer);
>  		usb_free_urb(sc->ghl_urb);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but error paths in sony_probe()
fail to remove the device context from sony_device_list, leading to a global
use-after-free.

When hid_hw_start() triggers sony_input_configured(), the device context sc
is added to the global sony_device_list. If a subsequent operation in
sony_probe() fails (for example, usb_alloc_urb() returns NULL), the function
jumps to the err label:

drivers/hid/hid-sony.c:sony_probe() {
    ...
err:
	usb_free_urb(sc->ghl_urb);

	hid_hw_stop(hdev);
	return ret;
}

Can this lead to a crash when the devm subsystem frees sc upon returning
the error? If the next Sony HID device is plugged in, it seems
sony_check_add() will iterate over sony_device_list and dereference the
freed sc pointer. Should this error path call sony_remove_dev_list(sc)
and sony_release_device_id(sc)?


[Severity: High]
This isn't a bug introduced by this patch, but system suspend breaks the
periodic keep-alive timer, permanently disconnecting the GHL guitar dongle.

During system suspend, usb_submit_urb() fails synchronously. When this
happens, the timer callback exits without re-arming the timer:

drivers/hid/hid-sony.c:ghl_magic_poke() {
    ...
	ret = usb_submit_urb(sc->ghl_urb, GFP_ATOMIC);
	if (ret < 0)
		hid_err(sc->hdev, "usb_submit_urb failed: %d", ret);
    ...
}

Neither sony_suspend() nor sony_resume() seem to manage ghl_poke_timer:

drivers/hid/hid-sony.c:
static int sony_suspend(struct hid_device *hdev, pm_message_t message)
{
    ...
}

static int sony_resume(struct hid_device *hdev)
{
    ...
}

Once the system resumes, will the timer remain dead, effectively dropping
the device connection permanently? Does the suspend handler need to pause
the timer and the resume handler restart it?


[Severity: Low]
This isn't a bug introduced by this patch, but URB DMA buffers are allocated
using devm_kzalloc(), which violates DMA API cache-line alignment
requirements.

Looking at ghl_init_urb():

	cr = devm_kzalloc(&sc->hdev->dev, sizeof(*cr), GFP_ATOMIC);
	... 
	databuf = devm_kzalloc(&sc->hdev->dev, poke_size, GFP_ATOMIC);

And in sony_allocate_output_report():

		sc->output_report_dmabuf =
			devm_kmalloc(&sc->hdev->dev,
				sizeof(union sixaxis_output_report_01),
				GFP_KERNEL);

Since the devm subsystem prepends a devres_node header, the resulting
pointers are offset from ARCH_DMA_MINALIGN. Is it safe to map these unaligned
buffers for DMA by the USB core?

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