linux-kernel - [PATCH v2] Input - mt: Fix input_mt_get_slot_by

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1427741655-4142-1-git-send-email-benjamin.tissoires@redhat.com>
Date:	Mon, 30 Mar 2015 14:54:15 -0400
From:	Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:	Jiri Kosina <jkosina@...e.cz>,
	Henrik Rydberg <rydberg@...math.org>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key

The case occurred recently with a touchscreen using twice a slot during a
single EV_SYN event:

E: 0.288415 0000 0000 0000      # ------------ SYN_REPORT (0) ----------
E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
E: 0.296207 0003 0039 -001      # EV_ABS / ABS_MT_TRACKING_ID   -1
E: 0.296207 0003 002f 0001      # EV_ABS / ABS_MT_SLOT          1
E: 0.296207 0003 0035 0908      # EV_ABS / ABS_MT_POSITION_X    908
E: 0.296207 0003 0036 1062      # EV_ABS / ABS_MT_POSITION_Y    1062
E: 0.296207 0003 002f 0000      # EV_ABS / ABS_MT_SLOT          0
E: 0.296207 0003 0039 8787      # EV_ABS / ABS_MT_TRACKING_ID   8787
E: 0.296207 0003 0035 1566      # EV_ABS / ABS_MT_POSITION_X    1566
E: 0.296207 0003 0036 0861      # EV_ABS / ABS_MT_POSITION_Y    861
E: 0.296207 0003 0000 0908      # EV_ABS / ABS_X                908
E: 0.296207 0003 0001 1062      # EV_ABS / ABS_Y                1062
E: 0.296207 0000 0000 0000      # ------------ SYN_REPORT (0) ----------

This occurred because while having already slots 0 and 1 assigned, the
touchscreen sent:

0.293377 Tip Switch: 0 | Contact Id: 0 | X:  539 | Y: 1960 | Contact Count: 3
0.294783 Tip Switch: 1 | Contact Id: 1 | X:  908 | Y: 1062 | Contact Count: 0
0.296187 Tip Switch: 1 | Contact Id: 2 | X: 1566 | Y:  861 | Contact Count: 0

Slot 0 is released correclty, but when we look for Contact ID 2, the slot
0 is then picked up again because it is marked as inactive (trackingID < 0).

This is wrong, and we should not reuse a slot in the same frame.
The test should also check for input_mt_is_used().

In addition, we need to initialize mt->frame to an other value than 0.
With mt->frame being 0, all slots are tags as currently used, and so
input_mt_get_slot_by_key() would return -1 for all requests.

Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=88903

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
---

Changes in v2:
- add note in input_mt_get_slot_by_key that input_mt_sync_frame() is required

Hi Dmitry, Henrik, Jiri,

With https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-4.1/wacom&id=9a1c001298fd567c0f0776ab54ab9965eeb9019f
in Jiri's tree, scheduled for 4.1, this patch should not break any existing
driver. I'd like us to stage it somewhere so that it doesn't get forgotten.

Henrik's previous concerns were that input_mt_sync_frame() may not be called
by a driver using input_mt_get_slot_by_key(), and now, no driver should be in
this case.

Cheers,
Benjamin

 drivers/input/input-mt.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index cb150a1..34feb3e 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -88,10 +88,13 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 			goto err_mem;
 	}
 
-	/* Mark slots as 'unused' */
+	/* Mark slots as 'inactive' */
 	for (i = 0; i < num_slots; i++)
 		input_mt_set_value(&mt->slots[i], ABS_MT_TRACKING_ID, -1);
 
+	/* Mark slots as 'unused' */
+	mt->frame = 1;
+
 	dev->mt = mt;
 	return 0;
 err_mem:
@@ -439,6 +442,8 @@ EXPORT_SYMBOL(input_mt_assign_slots);
  * set the key on the first unused slot and return.
  *
  * If no available slot can be found, -1 is returned.
+ * Note that for this function to work properly, input_mt_sync_frame() has
+ * to be called at each frame.
  */
 int input_mt_get_slot_by_key(struct input_dev *dev, int key)
 {
@@ -453,7 +458,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int key)
 			return s - mt->slots;
 
 	for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
-		if (!input_mt_is_active(s)) {
+		if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
 			s->key = key;
 			return s - mt->slots;
 		}
-- 
2.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/