[PATCH] HID: multitouch: bound the slot index before touching mt_io_flags

[PATCH] HID: multitouch: bound the slot index before touching mt_io_flags

[Your Name]

From: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

td->mt_io_flags is a single unsigned long.  Its first MT_IO_SLOTS_BITS
bits track per-slot state, read back through MT_IO_SLOTS_MASK, while bit
32 holds MT_IO_FLAGS_RUNNING, which the report path and the sticky-finger
timer take and release with test_and_set_bit_lock() and
clear_bit_unlock().

set_bit()/clear_bit() are called on that word with the slot number as the
bit index:

	set_bit(slotnum, &td->mt_io_flags);

slotnum is bounded only by td->maxcontacts, which is taken from the HID
CONTACTMAX feature value and can reach MT_MAX_MAXCONTACT (250).  Two
things go wrong once a device reports a slot number of 8 or more:

 - a slot number of 32 sets MT_IO_FLAGS_RUNNING from the data path.
   mt_expired_timeout() then finds the flag already set and returns
   early, so sticky fingers are never released, and the
   clear_bit_unlock() in the other path drops a lock it does not hold.
   Several in-tree classes declare .maxcontacts of 40 and 60, so this is
   reachable with ordinary hardware.

 - a slot number of BITS_PER_LONG or more writes past the end of the word
   altogether, corrupting the struct mt_device fields that follow it.

The same unchecked index is used in mt_release_pending_palms(), where
slotnum comes from a for_each_set_bit() bounded by td->maxcontacts, and
in mt_release_contacts(), where it is bounded by mt->num_slots.

Bound the index to the reserved slot range in one place and use that for
every set_bit()/clear_bit() on mt_io_flags.  Dropping the out-of-range
slots is not a functional change: both readers of the slot state mask
with MT_IO_SLOTS_MASK, so bits at or above MT_IO_SLOTS_BITS were never
observable to begin with.

Also correct the comment on the field, which claimed that eight bits were
sufficient because at most 250 slots are supported.

Fixes: 46f781e0d151 ("HID: multitouch: fix sticky fingers")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 drivers/hid/hid-multitouch.c | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0495152091e3..463eab9f73cd 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -98,6 +98,7 @@ enum report_mode {
 };
 
 #define MT_IO_SLOTS_MASK		GENMASK(7, 0) /* reserve first 8 bits for slot tracking */
+#define MT_IO_SLOTS_BITS		8 /* bits covered by MT_IO_SLOTS_MASK */
 #define MT_IO_FLAGS_RUNNING		32
 
 static const bool mtrue = true;		/* default for true */
@@ -175,9 +176,10 @@ struct mt_device {
 	struct hid_haptic_device *haptic;	/* haptic related configuration */
 	struct hid_device *hdev;	/* hid_device we're attached to */
 	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_RUNNING)
-					 * first 8 bits are reserved for keeping the slot
-					 * states, this is fine because we only support up
-					 * to 250 slots (MT_MAX_MAXCONTACT)
+					 * the first MT_IO_SLOTS_BITS bits are reserved
+					 * for keeping the slot states; higher slot
+					 * numbers are not tracked here, see
+					 * mt_io_slot_set()
 					 */
 	__u8 inputmode_value;	/* InputMode HID feature value */
 	__u8 maxcontacts;
@@ -1027,6 +1029,25 @@ static int mt_compute_slot(struct mt_dev
 	return input_mt_get_slot_by_key(input, *slot->contactid);
 }
 
+/*
+ * Only the first MT_IO_SLOTS_BITS bits of mt_io_flags track slot state; the
+ * rest of the word holds unrelated flags such as MT_IO_FLAGS_RUNNING.  Slot
+ * numbers are bounded by td->maxcontacts, which is taken from the HID
+ * CONTACTMAX feature and can be much larger, so the index has to be checked
+ * before touching the word.
+ */
+static void mt_io_slot_set(struct mt_device *td, int slotnum)
+{
+	if (slotnum < MT_IO_SLOTS_BITS)
+		set_bit(slotnum, &td->mt_io_flags);
+}
+
+static void mt_io_slot_clear(struct mt_device *td, int slotnum)
+{
+	if (slotnum < MT_IO_SLOTS_BITS)
+		clear_bit(slotnum, &td->mt_io_flags);
+}
+
 static void mt_release_pending_palms(struct mt_device *td,
 				     struct mt_application *app,
 				     struct input_dev *input)
@@ -1036,7 +1057,7 @@ static void mt_release_pending_palms(str
 
 	for_each_set_bit(slotnum, app->pending_palm_slots, td->maxcontacts) {
 		clear_bit(slotnum, app->pending_palm_slots);
-		clear_bit(slotnum, &td->mt_io_flags);
+		mt_io_slot_clear(td, slotnum);
 
 		input_mt_slot(input, slotnum);
 		input_mt_report_slot_inactive(input);
@@ -1247,9 +1268,9 @@ static int mt_process_slot(struct mt_dev
 		input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
 		input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
 
-		set_bit(slotnum, &td->mt_io_flags);
+		mt_io_slot_set(td, slotnum);
 	} else {
-		clear_bit(slotnum, &td->mt_io_flags);
+		mt_io_slot_clear(td, slotnum);
 	}
 
 	return 0;
@@ -2062,7 +2083,7 @@ static void mt_release_contacts(struct h
 			for (i = 0; i < mt->num_slots; i++) {
 				input_mt_slot(input_dev, i);
 				input_mt_report_slot_inactive(input_dev);
-				clear_bit(i, &td->mt_io_flags);
+				mt_io_slot_clear(td, i);
 			}
 			input_mt_sync_frame(input_dev);
 			input_sync(input_dev);
-- 
2.43.0

Re: [PATCH] HID: multitouch: bound the slot index before touching mt_io_flags

[sashiko-bot]

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

Pre-existing issues:
- [High] Out-of-bounds memory access / State corruption on 32-bit platforms due to `MT_IO_FLAGS_RUNNING` being 32.
- [High] Use-After-Free (UAF) race condition in `mt_remove` due to incorrect timer teardown ordering.
--

--- Patch [1]: [PATCH] HID: multitouch: bound the slot index before touching mt_io_flags ---
commit 876ccafb8bbdeb9e57250cb530c345252c4de7d6
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

HID: multitouch: bound the slot index before touching mt_io_flags

This commit prevents out-of-bounds writes to mt_io_flags by checking that
the slot index is within the reserved bit range. It introduces mt_io_slot_set()
and mt_io_slot_clear() to enforce this bound before calling set_bit() or
clear_bit().

Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 0495152091e3b..463eab9f73cdb 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -98,6 +98,7 @@ enum report_mode {
>  };
>  
>  #define MT_IO_SLOTS_MASK		GENMASK(7, 0) /* reserve first 8 bits for slot tracking */
> +#define MT_IO_SLOTS_BITS		8 /* bits covered by MT_IO_SLOTS_MASK */
>  #define MT_IO_FLAGS_RUNNING		32

[Severity: High]
This is a pre-existing issue, but does using bit index 32 cause out-of-bounds
memory access on 32-bit platforms?

Because mt_io_flags is a single unsigned long, valid bit indices are 0-31
on 32-bit architectures.

Calling test_and_set_bit_lock() with bit 32 will read and write to the memory
immediately following mt_io_flags.

Could this silently corrupt adjacent device state fields like inputmode_value
and maxcontacts in struct mt_device?

>  static const bool mtrue = true;		/* default for true */
> @@ -175,9 +176,10 @@ struct mt_device {
>  	struct hid_haptic_device *haptic;	/* haptic related configuration */
>  	struct hid_device *hdev;	/* hid_device we're attached to */
>  	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_RUNNING)
> -					 * first 8 bits are reserved for keeping the slot
> -					 * states, this is fine because we only support up
> -					 * to 250 slots (MT_MAX_MAXCONTACT)
> +					 * the first MT_IO_SLOTS_BITS bits are reserved
> +					 * for keeping the slot states; higher slot
> +					 * numbers are not tracked here, see
> +					 * mt_io_slot_set()
>  					 */
>  	__u8 inputmode_value;	/* InputMode HID feature value */
>  	__u8 maxcontacts;

[ ... ]

[Severity: High]
This is another pre-existing issue not introduced by this patch, but is there
a use-after-free race condition during device removal?

In mt_remove(), timer_delete_sync() is called before the hardware is stopped:

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 the hardware transport is still active, could an incoming HID report
trigger mt_touch_report() and re-arm the timer?

drivers/hid/hid-multitouch.c:mt_touch_report() {
    ...
			mod_timer(&td->release_timer,
				  jiffies + msecs_to_jiffies(100));
    ...
}

If the timer is re-armed after timer_delete_sync(), mt_remove() will complete
and free the devm-managed td structure.

When the timer later fires, won't mt_expired_timeout() access the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723224211.613112-1-you@example.com?part=1