Re: [PATCH] HID: letsketch: 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:
- [High] Missing timer cleanup on the probe error path leads to a Use-After-Free.
- [Medium] Unnecessary spinlock and state flag introduced instead of using `timer_shutdown_sync()`.
--

commit 2d88d7acbcf7e811559cfcb0df7e33f55178711b
Author: Manish Khadka <maskmemanish@gmail.com>

HID: letsketch: fix UAF on inrange_timer at driver unbind

This commit adds a remove callback to the letsketch driver to clean up
the inrange_timer during device unbind. It introduces a spinlock and a
removing flag to prevent the timer from being re-armed by in-flight
events while teardown is occurring.

> diff --git a/drivers/hid/hid-letsketch.c b/drivers/hid/hid-letsketch.c
> index 11e21f9887232..0dc9496d05f80 100644
> --- a/drivers/hid/hid-letsketch.c
> +++ b/drivers/hid/hid-letsketch.c

[ ... ]

> @@ -305,6 +315,23 @@ static int letsketch_probe(struct hid_device *hdev, const struct hid_device_id *
>  	return hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>  }

While not introduced by this patch, if hid_hw_start() fails and returns an
error, does this leave a pending timer that will fire on freed memory?

Looking at hid_hw_start(), it enables hardware I/O before attempting to
connect. If the connection fails, it stops I/O and returns an error:

hid_hw_start() {
        hdev->ll_driver->start(hdev);
        if (hid_connect(hdev, connect_mask)) {
                hdev->ll_driver->stop(hdev);
                return error;
        }
}

During the window where I/O is active, an incoming event could trigger
letsketch_raw_event() and arm the timer.

When letsketch_probe() directly returns this error, devres immediately frees
the letsketch_data structure without draining the timer.

> +static void letsketch_remove(struct hid_device *hdev)
> +{
> +	struct letsketch_data *data = hid_get_drvdata(hdev);
> +
> +	/*
> +	 * Block raw_event from arming inrange_timer during teardown so
> +	 * timer_delete_sync() below cannot race with a fresh mod_timer()
> +	 * issued from a URB completion handler still in flight while
> +	 * hid_hw_stop() is running.
> +	 */
> +	scoped_guard(spinlock_irqsave, &data->lock)
> +		data->removing = true;
> +
> +	timer_delete_sync(&data->inrange_timer);
> +	hid_hw_stop(hdev);
> +}

Could this synchronization be achieved without adding a new spinlock and state
flag by using timer_shutdown_sync() instead?

The timer_shutdown_sync() API permanently disables the timer, ensuring that
any subsequent mod_timer() calls are silent no-ops.

Alternatively, if hid_hw_stop() were called before cleaning up the timer, it
would wait for all URBs to complete, which structurally guarantees no new
events can arrive to re-arm the timer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515153551.76162-1-maskmemanish@gmail.com?part=1