[PATCH] HID: sony: fix use-after-free when removing GHL devices

[PATCH] HID: sony: fix use-after-free when removing GHL devices

[Fan Wu]

The GHL poke timer and its control URB re-arm each other: the timer
callback submits the URB, and the URB completion re-arms the timer.

sony_remove() only called timer_delete_sync(), which waits for a running
callback but does not stop a URB the callback already submitted and does
not stop the completion from re-arming the timer.  The sony_sc structure
is devm-allocated and freed after sony_remove() returns, so on removal
the completion or the re-armed timer can run on freed memory.

Use timer_shutdown_sync() instead, which also detaches the callback so
the completion can no longer re-arm the timer, and add usb_kill_urb()
before usb_free_urb() to drain any URB the callback submitted.  The URB
is driver-private and not anchored, so hid_hw_stop() does not touch it.

Found by a static analysis tool.

Fixes: fb1a79a6b6e1 ("HID: sony: fix freeze when inserting ghlive ps3/wii dongles")
Cc: stable@vger.kernel.org
Cc: Pascal Giard <pascal.giard@etsmtl.ca>
Assisted-by: Codex:gpt-5.6
Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
---
 drivers/hid/hid-sony.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index b966e4044238..bbd95adfbed6 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -2164,7 +2164,8 @@ 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);
+		timer_shutdown_sync(&sc->ghl_poke_timer);
+		usb_kill_urb(sc->ghl_urb);
 		usb_free_urb(sc->ghl_urb);
 	}
 
-- 
2.34.1

Re: [PATCH] HID: sony: fix use-after-free when removing GHL devices

[sashiko-bot]

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

Pre-existing issues:
- [High] In the `sony_probe()` error path, driver-specific resources registered during `sony_input_configured()` are not cleaned up when probe fails after `hid_hw_start()`, leading to a Use-After-Free.
- [High] The GHL poke timer and its URB are not paused during system suspend, causing the poke loop to permanently stop and the device to freeze upon resume.
--

--- Patch [1]: [PATCH] HID: sony: fix use-after-free when removing GHL devices ---
commit 60eeab13afba312995f6d575044d513de06674d1
Author: Fan Wu <fanwu01@zju.edu.cn>

HID: sony: fix use-after-free when removing GHL devices

This commit fixes a use-after-free issue during the removal of GHL devices
by using timer_shutdown_sync() to prevent timer re-arming. It also adds
usb_kill_urb() to drain any URB the callback submitted before freeing it.

> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
> index e75246d29e16d..f7929a7848be4 100644
> --- a/drivers/hid/hid-sony.c
> +++ b/drivers/hid/hid-sony.c
> @@ -2424,7 +2424,8 @@ 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);
> +		timer_shutdown_sync(&sc->ghl_poke_timer);
> +		usb_kill_urb(sc->ghl_urb);
>  		usb_free_urb(sc->ghl_urb);
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but looking at how the GHL poke
timer is managed, is the timer properly paused during system suspend?

If the system enters suspend and the USB bus is suspended, what happens if the
timer fires? It looks like usb_submit_urb() in ghl_magic_poke() will return an
error and fail to re-arm 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() appear to pause and restart this
timer loop. Does this mean the poke loop silently dies upon suspend, causing
the GHL device to freeze when the system resumes?

[Severity: High]
This is also a pre-existing issue, but seeing the teardown sequence in
sony_remove() raises a question about the error path in sony_probe().

When hid_hw_start() succeeds, it internally calls the driver's
sony_input_configured() callback, which allocates resources and adds the
device to the global sony_device_list.

If a subsequent check in sony_probe() fails, it 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;
}

This error path does not appear to call sony_remove_dev_list(sc) or
sony_release_device_id(sc) like sony_remove() does. Since sc is managed by
devm_kzalloc(), the memory is freed when probe returns, leaving a dangling
pointer in sony_device_list. Can this lead to a use-after-free the next time
any Sony device is probed and the list is traversed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807052328.11404-1-fanwu01@zju.edu.cn?part=1