Re: [PATCH v2] 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 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