linux-kernel - Re: [PATCH] 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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <54D121C8.80206@bitmath.org>
Date:	Tue, 03 Feb 2015 20:30:16 +0100
From:	Henrik Rydberg <rydberg@...math.org>
To:	Benjamin Tissoires <benjamin.tissoires@...hat.com>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
CC:	Hans de Goede <hdegoede@...hat.com>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input - mt: Fix input_mt_get_slot_by_key

Hi Benjamin,

> 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().

Good catch! However...

> @@ -453,7 +456,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;
>  		}
> 

Here, you are changing the preconditions of the function without explicit
reference to all its users. For one, it is now assumed that input_mt_is_used()
is up-to-date, which requires either input_mt_drop_unused() or
input_mt_sync_frame(), which does not seem to be true for all users of
input_mt_get_slot_by_key(). After a couple of iterations with
input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true
for all slots, and the driver will stop working.

How about defering the deassignments until the end of the loop instead? That
would remove possible reuse.

Thanks,
Henrik

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