[PATCH] HID: multitouch: stop the release timer from being rearmed on remove

[PATCH] HID: multitouch: stop the release timer from being rearmed on remove

[Aldo Ariel Panzardo]

mt_remove() quiesces the sticky-finger timer before stopping the
hardware:

    timer_delete_sync(&td->release_timer);

    sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
    hid_hw_stop(hdev);

timer_delete_sync() waits for a running callback and dequeues the timer,
but it does not stop the timer from being armed again. The transport is
still delivering reports at that point, and the report path rearms it:

    if (app->quirks & MT_QUIRK_STICKY_FINGERS) {
        if (td->mt_io_flags & MT_IO_SLOTS_MASK)
            mod_timer(&td->release_timer,
                      jiffies + msecs_to_jiffies(100));

A report that arrives after timer_delete_sync() has returned therefore
leaves the timer queued. td is allocated with devm_kzalloc() against
hdev->dev, so it is freed when the driver is unbound, after mt_remove()
returns. When the timer fires afterwards, mt_expired_timeout()
dereferences the freed td:

    struct mt_device *td = timer_container_of(td, t, release_timer);
    struct hid_device *hdev = td->hdev;

    if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))

Simply moving the teardown after hid_hw_stop() does not fix this on its
own, because mt_expired_timeout() calls mt_release_contacts(), which
walks hdev->inputs; the timer still has to be quiesced before
hid_hw_stop() tears the input devices down.

Use timer_shutdown_sync() instead, which additionally makes any later
mod_timer() a no-op, so neither ordering constraint has to be traded off
against the other. This is the final-teardown pattern the function was
introduced for, and hid-wiimote already uses it for the same reason.

Fixes: 4f4001bc76fd ("HID: multitouch: fix rare Win 8 cases when the touch up event gets missing")
Cc: stable@vger.kernel.org
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260723224211.613112-1-you@example.com?part=1
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
Found by code inspection after Sashiko AI review flagged the teardown
ordering while reviewing an unrelated patch of mine. I have not
reproduced the use-after-free at runtime: it needs a report to land in
the window between timer_delete_sync() returning and the device being
unbound, which I have no way to drive reliably on the hardware I have.
The window and the rearm path are visible in the code, and the fix does
not depend on the race being hit.

 drivers/hid/hid-multitouch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0495152091e3..f25065b9ec66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -2233,7 +2233,7 @@ static void mt_remove(struct hid_device *hdev)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
 
-	timer_delete_sync(&td->release_timer);
+	timer_shutdown_sync(&td->release_timer);
 
 	sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
 	hid_hw_stop(hdev);
-- 
2.43.0

Re: [PATCH] HID: multitouch: stop the release timer from being rearmed on remove

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Out-of-bounds write via set_bit() leading to memory corruption by malicious HID devices.
- [High] Use-After-Free of report buffers in hid_haptic_destroy during device teardown.
- [High] Race condition during probe leading to NULL pointer dereference in mt_sync_frame.
- [High] Sleeping in atomic context when processing unmapped HID reports.
- [Medium] Data race modifying td->applications list without synchronization.
--

--- Patch [1]: [PATCH] HID: multitouch: stop the release timer from being rearmed on remove ---
commit b618d8ab09924436537bc4c4c724c79da532672e
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
Subject: HID: multitouch: stop the release timer from being rearmed on remove

mt_remove() quiesces the sticky-finger timer before stopping the
hardware. timer_delete_sync() waits for a running callback and
dequeues the timer, but it does not stop the timer from being armed
again. Use timer_shutdown_sync() instead.

Cc: stable@vger.kernel.org
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/patchset/20260723224211.613112-1-you@example.com?part=1
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 0495152091e3b..f25065b9ec66e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -2233,7 +2233,7 @@ static void mt_remove(struct hid_device *hdev)
>  {
>  	struct mt_device *td = hid_get_drvdata(hdev);
>  
> -	timer_delete_sync(&td->release_timer);
> +	timer_shutdown_sync(&td->release_timer);
>  
>  	sysfs_remove_group(&hdev->dev.kobj, &mt_attribute_group);
>  	hid_hw_stop(hdev);
>  }

[ ... ]

drivers/hid/hid-haptic.c:hid_haptic_destroy() {
    ...
	kfree(haptic->stop_effect.report_buf);
	haptic->stop_effect.report_buf = NULL;

	if (haptic->effect) {
		for (r = 0; r < ff->max_effects; r++)
			kfree(haptic->effect[r].report_buf);
		kfree(haptic->effect);
	}
	haptic->effect = NULL;

	destroy_workqueue(haptic->wq);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a
use-after-free during device teardown?

When an input device is destroyed, input_ff_flush() queues
stop_effect.work on the workqueue. If the work is still pending,
destroy_workqueue() will wait for it to finish. However, the report
buffers are freed before the workqueue is drained.

When haptic_work_handler() executes, it calls play_effect() which uses
the freed effect->report_buf.

Should destroy_workqueue() be called before freeing the report buffers?

drivers/hid/hid-multitouch.c:mt_probe() {
    ...
	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
	if (ret)
		return ret;
    ...
	if (td->is_haptic_touchpad) {
		if (hid_haptic_init(hdev, &td->haptic)) {
			dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
				 hdev->name);
			td->is_haptic_touchpad = false;
			devm_kfree(&hdev->dev, td->haptic);
		}
    ...
}

[Severity: High]
This is a pre-existing issue, but does this create a race condition if
hid_haptic_init() fails?

Since hid_hw_start() enables hardware interrupts earlier in mt_probe(),
an interrupt could arrive after hid_haptic_init() fails (setting td->haptic
to NULL) but before td->is_haptic_touchpad is set to false.

If mt_sync_frame() runs in this window, it will see td->is_haptic_touchpad
as true and call hid_haptic_pressure_reset(td->haptic), dereferencing a
NULL pointer.

Should the interrupt enablement or error handling order be adjusted?

drivers/hid/hid-multitouch.c:struct mt_device {
    ...
	unsigned long mt_io_flags;
	__u8 inputmode_value;
    ...
	struct list_head applications;
    ...
}

drivers/hid/hid-multitouch.c:mt_process_slot() {
    ...
	set_bit(slotnum, &td->mt_io_flags);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does this set_bit() call allow an
out-of-bounds write?

A malicious USB HID device could declare a contact max up to 250, resulting
in a slotnum up to 249. Since mt_io_flags is a single unsigned long, calling
set_bit(249, ...) overflows the variable by up to 31 bytes.

This would corrupt adjacent fields in the mt_device struct, including the
inputmode_value and the applications list pointers.

Can the slot index be constrained or the storage for mt_io_flags be expanded?

drivers/hid/hid-multitouch.c:mt_allocate_report_data() {
    ...
	rdata = devm_kzalloc(&td->hdev->dev, sizeof(*rdata), GFP_KERNEL);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but can this sleep in atomic
context?

If a device sends an unmapped report ID dynamically over the interrupt
endpoint, the URB completion handler runs in atomic context and calls
hid_report_raw_event(), which eventually calls mt_report().

mt_report() then calls mt_find_report_data() and mt_allocate_report_data().
Since devm_kzalloc() is called with GFP_KERNEL here, it could fault when
processing these unmapped reports in atomic context.

Should this use GFP_ATOMIC instead?

drivers/hid/hid-multitouch.c:mt_set_quirks() {
    ...
	list_for_each_entry(application, &td->applications, list) {
    ...
}

drivers/hid/hid-multitouch.c:mt_allocate_application() {
    ...
	list_add_tail(&mt_application->list, &td->applications);
    ...
}

[Severity: Medium]
This is a pre-existing issue, but is there a data race on the
td->applications list?

mt_set_quirks() iterates over td->applications without locking.
Concurrently, if the device receives an unmapped report, mt_report() can
call mt_allocate_application() from the interrupt path, which adds to the
same list using list_add_tail().

Could this concurrent modification without synchronization corrupt the
linked list?

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