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

[sashiko-bot@kernel.org]

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