| 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
| ||
|
Date: Fri, 18 May 2012 21:14:08 +0000
From: "Drews, Paul" <paul.drews@...el.com>
To: Benjamin Tissoires <benjamin.tissoires@...il.com>,
Henrik Rydberg <rydberg@...omail.se>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Jiri Kosina <jkosina@...e.cz>,
Stephane Chatty <chatty@...c.fr>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
Subject: RE: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
> -----Original Message-----
> From: linux-kernel-owner@...r.kernel.org [mailto:linux-kernel-
> owner@...r.kernel.org] On Behalf Of Benjamin Tissoires
> Sent: Wednesday, May 09, 2012 12:04 PM
> To: Henrik Rydberg
> Cc: Dmitry Torokhov; Jiri Kosina; Stephane Chatty; linux-input@...r.kernel.org;
> linux-kernel@...r.kernel.org
> Subject: Re: [PATCH 1/5] HID: hid-multitouch: fix wrong protocol detection
>
> On Sun, May 6, 2012 at 9:01 PM, Henrik Rydberg <rydberg@...omail.se> wrote:
> > Hi Benjamin,
> >
> >> The previous implementation introduced a randomness in the splitting
> >> of the different touches reported by the device. This version is more
> >> robust as we don't rely on hi->input->absbit, but on our own structure.
> >>
> >> +
> >> struct mt_device {
> >> struct mt_slot curdata; /* placeholder of incoming data */
> >> struct mt_class mtclass; /* our mt device class */
> >> + struct mt_fields *fields; /* temporary placeholder for storing the
> >> + multitouch fields */
> >
> > Why not skip the pointer here?
>
> well, the idea was to keep the memory footprint low. As these values
> are only needed at init, then I freed them once I finished using them.
> I can of course skip the pointer, but in that case, wouldn't the
> struct declaration be worthless?
>
> >
> >>
> >> +static void mt_post_parse(struct mt_device *td)
> >> +{
> >> + struct mt_fields *f = td->fields;
> >> +
> >> + if (td->touches_by_report > 0) {
> >> + int field_count_per_touch = f->length / td->touches_by_report;
> >> + td->last_slot_field = f->usages[field_count_per_touch - 1]->hid;
> >> + }
> >> +}
> >> +
It sounds as though:
() Reviewers are a little uncomfortable with the memory footprint and
allocation/free
() The patch as it stands relies on the pattern of "usage" values repeating
for each touch, and deeming the last one in the repetition pattern to
be the last-slot-field marker.
If this is the case, how about avoiding storing all the slot-field values
and just detecting the point of repetition to use the most-recently-seen
usage value as the last-slot-field marker. I have been successfully using
the patch below based on this notion. It took the failure rate from about
1-per-10 boots to 250+ boots with no failures on an Atmel MaXTouch.
I don't have others to try it with, including the "buggy" one that led
to all this trouble in the first place.
Patch follows:
==========================================================
>From 242f6773babe0fc0215764abbeeeff6510f3ca92 Mon Sep 17 00:00:00 2001
From: Paul Drews <paul.drews@...el.com>
Date: Wed, 16 May 2012 11:15:00 -0700
Subject: [PATCH] Repair detection of last slot in multitouch reports
The logic for detecting the last per-touch slot in a
multitouch report erroneously used hid usage values (large
numbers such as 0xd0032) as indices into the smaller absbit
bitmap (with bit indexes up to 0x3f). This caused
intermittent failures in the configuration of the last-slot
value leading to stale x,y coordinates being reported in
multi-touch input events. It also carried the risk of a
segmentation fault due to the out-of-range bitmap index.
This patch takes a different approach of detecting the last
per-touch slot: when the hid usage value wraps around to
the first hid usage value we have seen already, we must be
looking at the slots for the next touch of a multi-touch
report, so the last hid usage value we have seen so far must
be the last per-touch value.
Issue: AIA-446
Change-Id: Ic1872998502874298bb60705df9bd2fc70de1738
Signed-off-by: Paul Drews <paul.drews@...el.com>
---
drivers/hid/hid-multitouch.c | 39 ++++++++++++++++++++++++++-------------
1 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2e6d187..226f828 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -75,6 +75,9 @@ struct mt_device {
struct mt_class mtclass; /* our mt device class */
unsigned last_field_index; /* last field index of the report */
unsigned last_slot_field; /* the last field of a slot */
+ bool last_slot_field_found; /* last_slot_field has full init */
+ unsigned first_slot_field;
+ bool first_slot_field_found; /* for detecting wrap to next touch */
__s8 inputmode; /* InputMode HID feature, -1 if non-existent */
__s8 maxcontact_report_id; /* Maximum Contact Number HID feature,
-1 if non-existent */
@@ -275,11 +278,21 @@ static void set_abs(struct input_dev *input, unsigned int code,
input_set_abs_params(input, code, fmin, fmax, fuzz, 0);
}
-static void set_last_slot_field(struct hid_usage *usage, struct mt_device *td,
- struct hid_input *hi)
+static void update_last_slot_field(struct hid_usage *usage,
+ struct mt_device *td)
{
- if (!test_bit(usage->hid, hi->input->absbit))
- td->last_slot_field = usage->hid;
+ if (!td->last_slot_field_found) {
+ if (td->first_slot_field_found) {
+ if (td->last_slot_field == usage->hid)
+ td->last_slot_field_found = true; /* wrapped */
+ else
+ td->last_slot_field = usage->hid;
+ } else {
+ td->first_slot_field = usage->hid;
+ td->first_slot_field_found = true;
+ td->last_slot_field = usage->hid;
+ }
+ }
}
static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
@@ -340,7 +353,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
cls->sn_move);
/* touchscreen emulation */
set_abs(hi->input, ABS_X, field, cls->sn_move);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_GD_Y:
@@ -350,7 +363,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
cls->sn_move);
/* touchscreen emulation */
set_abs(hi->input, ABS_Y, field, cls->sn_move);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
}
@@ -359,24 +372,24 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
case HID_UP_DIGITIZER:
switch (usage->hid) {
case HID_DG_INRANGE:
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_CONFIDENCE:
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_TIPSWITCH:
hid_map_usage(hi, usage, bit, max, EV_KEY, BTN_TOUCH);
input_set_capability(hi->input, EV_KEY, BTN_TOUCH);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_CONTACTID:
if (!td->maxcontacts)
td->maxcontacts = MT_DEFAULT_MAXCONTACT;
input_mt_init_slots(hi->input, td->maxcontacts);
- td->last_slot_field = usage->hid;
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
td->touches_by_report++;
return 1;
@@ -385,7 +398,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
EV_ABS, ABS_MT_TOUCH_MAJOR);
set_abs(hi->input, ABS_MT_TOUCH_MAJOR, field,
cls->sn_width);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_HEIGHT:
@@ -395,7 +408,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
cls->sn_height);
input_set_abs_params(hi->input,
ABS_MT_ORIENTATION, 0, 1, 0, 0);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_TIPPRESSURE:
@@ -406,7 +419,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
/* touchscreen emulation */
set_abs(hi->input, ABS_PRESSURE, field,
cls->sn_pressure);
- set_last_slot_field(usage, td, hi);
+ update_last_slot_field(usage, td);
td->last_field_index = field->index;
return 1;
case HID_DG_CONTACTCOUNT:
--
1.7.3.4
==========================================================
--
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/
Powered by blists - more mailing lists