Re: [PATCH] HID: uclogic: fix UAF on inrange_timer at driver unbind

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Moving timer_shutdown_sync() after hid_hw_stop() introduces a Use-After-Free of drvdata->pen_input during driver unbind.

Pre-existing issues:
- [Critical] Missing timer cancellation in the uclogic_probe() error path leads to a Use-After-Free if initialization fails after I/O has started.
--

--- Patch [1]: [PATCH] HID: uclogic: fix UAF on inrange_timer at driver unbind ---
commit 0de50e5b91e3a9978c96d85e0aab45d43b0d98d6
Author: Doruk Tan Ozturk <doruk@0sec.ai>

HID: uclogic: fix UAF on inrange_timer at driver unbind

uclogic_remove() drained the in-range emulation timer before calling
hid_hw_stop(), leaving a window where a completing pen report could
re-arm the timer. This commit reorders the operations to call
hid_hw_stop() first, preventing re-arming before shutting down the timer.

> diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
> index b73f09d26688a..c440013a609fd 100644
> --- a/drivers/hid/hid-uclogic-core.c
> +++ b/drivers/hid/hid-uclogic-core.c
> @@ -548,8 +548,8 @@ static void uclogic_remove(struct hid_device *hdev)
>  {
>  	struct uclogic_drvdata *drvdata = hid_get_drvdata(hdev);
>  
> -	timer_delete_sync(&drvdata->inrange_timer);
>  	hid_hw_stop(hdev);
> +	timer_shutdown_sync(&drvdata->inrange_timer);

[Severity: Critical]
Can moving timer_shutdown_sync() after hid_hw_stop() introduce a
use-after-free on drvdata->pen_input?

hid_hw_stop() synchronously unregisters and frees the standard input
devices, which frees the memory pointed to by drvdata->pen_input.

If an incoming report arms the timer just before hid_hw_stop() is called,
the timer will remain active.

When it fires, the callback will unconditionally dereference the freed
pointer:

drivers/hid/hid-uclogic-core.c:uclogic_inrange_timeout()
	struct input_dev *input = drvdata->pen_input;

	if (input == NULL)
		return;
	input_report_abs(input, ABS_PRESSURE, 0);


[Severity: Critical]
This is a pre-existing issue, but can the uclogic_probe() error path
also lead to a use-after-free if initialization fails?

If hid_hw_start() briefly starts hardware I/O but subsequently fails
internally, an incoming report could trigger uclogic_raw_event_pen() and
arm the timer before the error path is taken:

drivers/hid/hid-uclogic-core.c:uclogic_probe()
	rc = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
	if (rc) {
		hid_err(hdev, "hw start failed\n");
		goto failure;
	}

	return 0;
failure:
	/* Assume "remove" might not be called if "probe" failed */
	if (params_initialized)
		uclogic_params_cleanup(&drvdata->params);
	return rc;

Because this cleanup block does not cancel the inrange_timer, the devm
framework will free the drvdata structure upon probe return.

This leaves the timer armed to execute on freed memory.

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

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