On Thu, Jul 21, 2016 at 9:10 AM, Jason Gerecke <killertofu@xxxxxxxxx> wrote:
> If a touchscreen contains both multitouch and single-touch reports in its
> descriptor in that order, the driver may overwrite information it saved
> about the format of the multitouch report. This can cause the report
> processing code to get tripped up and send an incorrect event stream to
> userspace.
>
> In particular, this can cause last_slot_field to be overwritten with the
> result that the driver prematurely assumes it has finished processing a
> slot and sending the ABS_MT_SLOT event at the wrong point in time,
> associating events for the current contact with the following contact
> instead.
>
> To prevent this from occurring, we update the value of last_slot_field
> durring the pre_report phase to ensure that it is correct for the report
> that is to be processed.
Nice job, Jason! I like this approach. The patch is:
Reviewed-by: Ping Cheng <pingc@xxxxxxxxx>
Cheers,
Ping
> Signed-off-by: Jason Gerecke <jason.gerecke@xxxxxxxxx>
> ---
> Changes from v1:
> * The v1 patch cut off processing by wacom_wac_finger_usage_mapping once
> it saw a HID_DG_CONTACTCOUNT usage in any report. This method of
> handling the bug has two potential problems: 1) reports which place
> the HID_DG_CONTACTCOUNT usage near the beginning of the packet will
> not properly map the rest of the usages, and 2) devices which have
> multiple reports with the HID_DG_CONTACTCOUNT usage will only send
> reports formatted like the first discovered. Neither of these should
> cause issues for currently available hardware, but to maximize forward
> compatibility, the revised scheme contained here was devised.
>
> drivers/hid/wacom_wac.c | 62 +++++++++++++++++++++----------------------------
> drivers/hid/wacom_wac.h | 2 +-
> 2 files changed, 27 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index fcf2264..d2611f3 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1556,13 +1556,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
> {
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> - struct wacom_features *features = &wacom_wac->features;
> struct input_dev *input = wacom_wac->touch_input;
> unsigned touch_max = wacom_wac->features.touch_max;
>
> switch (usage->hid) {
> case HID_GD_X:
> - features->last_slot_field = usage->hid;
> if (touch_max == 1)
> wacom_map_usage(input, usage, field, EV_ABS, ABS_X, 4);
> else
> @@ -1570,7 +1568,6 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
> ABS_MT_POSITION_X, 4);
> break;
> case HID_GD_Y:
> - features->last_slot_field = usage->hid;
> if (touch_max == 1)
> wacom_map_usage(input, usage, field, EV_ABS, ABS_Y, 4);
> else
> @@ -1579,22 +1576,11 @@ static void wacom_wac_finger_usage_mapping(struct hid_device *hdev,
> break;
> case HID_DG_WIDTH:
> case HID_DG_HEIGHT:
> - features->last_slot_field = usage->hid;
> wacom_map_usage(input, usage, field, EV_ABS, ABS_MT_TOUCH_MAJOR, 0);
> wacom_map_usage(input, usage, field, EV_ABS, ABS_MT_TOUCH_MINOR, 0);
> input_set_abs_params(input, ABS_MT_ORIENTATION, 0, 1, 0, 0);
> break;
> - case HID_DG_CONTACTID:
> - features->last_slot_field = usage->hid;
> - break;
> - case HID_DG_INRANGE:
> - features->last_slot_field = usage->hid;
> - break;
> - case HID_DG_INVERT:
> - features->last_slot_field = usage->hid;
> - break;
> case HID_DG_TIPSWITCH:
> - features->last_slot_field = usage->hid;
> wacom_map_usage(input, usage, field, EV_KEY, BTN_TOUCH, 0);
> break;
> case HID_DG_CONTACTCOUNT:
> @@ -1672,7 +1658,7 @@ static int wacom_wac_finger_event(struct hid_device *hdev,
>
>
> if (usage->usage_index + 1 == field->report_count) {
> - if (usage->hid == wacom_wac->features.last_slot_field)
> + if (usage->hid == wacom_wac->hid_data.last_slot_field)
> wacom_wac_finger_slot(wacom_wac, wacom_wac->touch_input);
> }
>
> @@ -1685,31 +1671,35 @@ static void wacom_wac_finger_pre_report(struct hid_device *hdev,
> struct wacom *wacom = hid_get_drvdata(hdev);
> struct wacom_wac *wacom_wac = &wacom->wacom_wac;
> struct hid_data* hid_data = &wacom_wac->hid_data;
> + int i;
>
> - if (hid_data->cc_report != 0 &&
> - hid_data->cc_report != report->id) {
> - int i;
> -
> - hid_data->cc_report = report->id;
> - hid_data->cc_index = -1;
> - hid_data->cc_value_index = -1;
> -
> - for (i = 0; i < report->maxfield; i++) {
> - struct hid_field *field = report->field[i];
> - int j;
> -
> - for (j = 0; j < field->maxusage; j++) {
> - if (field->usage[j].hid == HID_DG_CONTACTCOUNT) {
> - hid_data->cc_index = i;
> - hid_data->cc_value_index = j;
> -
> - /* break */
> - i = report->maxfield;
> - j = field->maxusage;
> - }
> + for (i = 0; i < report->maxfield; i++) {
> + struct hid_field *field = report->field[i];
> + int j;
> +
> + for (j = 0; j < field->maxusage; j++) {
> + struct hid_usage *usage = &field->usage[j];
> +
> + switch (usage->hid) {
> + case HID_GD_X:
> + case HID_GD_Y:
> + case HID_DG_WIDTH:
> + case HID_DG_HEIGHT:
> + case HID_DG_CONTACTID:
> + case HID_DG_INRANGE:
> + case HID_DG_INVERT:
> + case HID_DG_TIPSWITCH:
> + hid_data->last_slot_field = usage->hid;
> + break;
> + case HID_DG_CONTACTCOUNT:
> + hid_data->cc_report = report->id;
> + hid_data->cc_index = i;
> + hid_data->cc_value_index = j;
> + break;
> }
> }
> }
> +
> if (hid_data->cc_report != 0 &&
> hid_data->cc_index >= 0) {
> struct hid_field *field = report->field[hid_data->cc_index];
> diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
> index 8a8974c..267025c 100644
> --- a/drivers/hid/wacom_wac.h
> +++ b/drivers/hid/wacom_wac.h
> @@ -185,7 +185,6 @@ struct wacom_features {
> int pktlen;
> bool check_for_hid_type;
> int hid_type;
> - int last_slot_field;
> };
>
> struct wacom_shared {
> @@ -214,6 +213,7 @@ struct hid_data {
> int cc_report;
> int cc_index;
> int cc_value_index;
> + int last_slot_field;
> int num_expected;
> int num_received;
> };
> --
> 2.9.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
