Re: [PATCH v2] HID: uclogic: fix use-after-free of inrange_timer on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Multiple HID drivers suffer from race conditions during device removal due to incorrect or missing timer teardown.
- [High] In `hid-sony.c`, `sony_remove()` fails to synchronously kill the `ghl_urb` before freeing it, leading to a Use-After-Free in the URB callback.
--

--- Patch [1]: [PATCH v2] HID: uclogic: fix use-after-free of inrange_timer on remove ---
commit b8f98aaaf048d9d412a83d45b98e8c080f6ef05c
Author: Ibrahim Hashimov <security@auditcode.ai>

HID: uclogic: fix use-after-free of inrange_timer on remove

uclogic_remove() cancels the pen in-range timer and then stops the
device. A report landing between the timer_delete_sync() call and the transport
teardown in hid_hw_stop() re-arms inrange_timer after it was cancelled.

> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index b73f09d26688a..0b8a83fa6c5be 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -548,7 +548,17 @@ static void uclogic_remove(struct hid_device *hdev)
>  {
>  	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
>  
> -	timer_delete_sync(&drvdata->inrange_timer);
> +	/*
> +	 * Shut the in-range timer down before stopping the device.
> +	 * uclogic_raw_event_pen() re-arms inrange_timer on every pen report
> +	 * and keeps running until hid_hw_stop() stops the transport, so a
> +	 * plain timer_delete_sync() here can be undone by a report landing in
> +	 * the window before hid_hw_stop().  timer_shutdown_sync() cancels the
> +	 * timer and makes any later re-arm a no-op, so it is provably dead
> +	 * before hid_hw_stop() frees the input device drvdata->pen_input
> +	 * points at.
> +	 */
> +	timer_shutdown_sync(&drvdata->inrange_timer);
>  	hid_hw_stop(hdev);

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this
fix for the teardown race in uclogic, I noticed multiple other HID drivers
suffer from similar pre-existing race conditions during device removal.

For instance, drivers/hid/hid-letsketch.c lacks a .remove callback entirely
in letsketch_driver:

static struct hid_driver letsketch_driver = {
	.name = "letsketch",
	.id_table = letsketch_devices,
	.probe = letsketch_probe,
	.raw_event = letsketch_raw_event,
};

This means the inrange_timer is never explicitly canceled and can outlive
the driver's devres cleanup.

In drivers/hid/hid-multitouch.c mt_remove() and drivers/hid/hid-prodikeys.c
pk_remove(), the code calls timer_delete_sync() before hid_hw_stop(), leaving
the exact same window where incoming reports can re-arm the timer.

Additionally, in drivers/hid/hid-appleir.c appleir_remove() calls
hid_hw_stop() before timer_delete_sync(). Because hid_hw_stop() frees the
hid-input device, this can cause a use-after-free if the timer fires in this
window.

Would it make sense to apply this same timer_shutdown_sync() pattern and
add the missing remove callbacks to these other HID drivers as well?

[Severity: High]
This is also a pre-existing issue, but while looking at these removal paths,
I spotted another teardown race in drivers/hid/hid-sony.c during sony_remove().

The cleanup sequence is:

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

If ghl_poke_timer fires right before it is canceled, it submits ghl_urb.
The usb_free_urb() call only decrements the reference count; it does not
cancel an in-flight transfer.

When the URB later completes, ghl_magic_poke_cb() will dereference the
freed sc pointer to re-arm the timer, leading to a use-after-free.

Should this code be using usb_kill_urb() prior to freeing the URB to
ensure any in-flight transfers are synchronously terminated?

>  	kfree(drvdata->desc_ptr);
>  	uclogic_params_cleanup(&drvdata->params);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714173946.73773-1-security@auditcode.ai?part=1