Re: [PATCH v3] HID: multitouch: Fix stale MT slots when contact count drops to zero

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

Pre-existing issues:
- [High] Preexisting teardown race condition in mt_remove() where the sticky fingers release timer can be re-armed after deletion, leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v3] HID: multitouch: Fix stale MT slots when contact count drops to zero ---
commit de5f919b17036295856b898ddbe48949c82b0650
Author: Dave Carey <[email protected]>

HID: multitouch: Fix stale MT slots when contact count drops to zero

The INGENIC 17EF:6161 touchscreen reports a contact count of 0 in the frame
immediately following the last finger lift. This commit fixes an issue where
the driver left num_expected at its previous value, preventing stale slots
from being released. It resolves this by explicitly resetting num_expected to
0 for a new multi-report frame.

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index edb37b4c867e4..418925b03691b 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1321,21 +1321,18 @@ static void mt_touch_report(struct hid_device *hid,
>  	 * Includes multi-packet support where subsequent
>  	 * packets are sent with zero contactcount.
>  	 */
> -	if (contact_count >= 0) {
> +	if (contact_count > 0)
> +		app->num_expected = contact_count;

[ ... ]

> +		app->num_expected = 0;
>  	}
>  	app->prev_scantime = scantime;
>  

[Severity: High]
This is a pre-existing issue, but while looking at mt_touch_report(), I noticed
a potential race condition during device teardown that might lead to a
use-after-free.

In mt_remove(), the release timer is deleted before stopping the hardware:

drivers/hid/hid-multitouch.c:mt_remove() {
    ...
	timer_delete_sync(&td->release_timer);

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

Since hid_hw_stop() hasn't been called yet, the hardware is still active and
can generate events. If an incoming HID report is processed concurrently by
mt_touch_report(), it can re-arm the timer:

drivers/hid/hid-multitouch.c:mt_touch_report() {
    ...
	if (app->quirks & MT_QUIRK_STICKY_FINGERS) {
		if (!bitmap_empty(td->active_slots, td->maxcontacts))
			mod_timer(&td->release_timer,
				  jiffies + msecs_to_jiffies(100));
    ...
}

After mt_remove() completes, the devm-managed mt_device structure (td) is freed.
If the re-armed timer expires 100ms later, could mt_expired_timeout() execute
on the freed td structure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1