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

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

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