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

[Benjamin Tissoires <bentiss@kernel.org>]

On Jul 23 2026, Your Name wrote:
> 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.

FWIW, this (and your next patch which should have been tied with this
one as a series) is already fixed upstream through 0a3fe972a7cb ("HID:
core: Mitigate potential OOB by removing bogus memset()") - applied on
July 2 according to the logs.

Please always test your patches against the latest HID tree.

Cheers,
Benjamin

> 
> 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
> 
>