[PATCH libevdev 6/6] Drop invalid ABS_MT_TRACKING_ID changes
Peter Hutterer
peter.hutterer at who-t.net
Wed Apr 2 19:23:27 PDT 2014
On Wed, Apr 02, 2014 at 07:33:37PM -0400, Benjamin Tissoires wrote:
> On Tue, Apr 1, 2014 at 10:17 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > Follow-up to
> > commit 41334b5b40cd5456f5f584b55d8888aaafa1f26e
> > Author: Peter Hutterer <peter.hutterer at who-t.net>
> > Date: Thu Mar 6 11:54:00 2014 +1000
> >
> > If the tracking ID changes during SYN_DROPPED, terminate the touch first
> >
> > In normal mode, we may get double tracking ID events in the same slot, but
> > only if we either have a user-generated event sequence (uinput) or a malicious
> > device that tries to send data on a slot > dev->num_slots.
> > Since the client is unlikely to be able to handle these events, discard the
> > ABS_MT_TRACKING_ID completely. This is a bug somewhere in the stack, so
> > complain and hobble on along.
> >
> > Note: the kernel doesn't allow that, but we cap to num_slots anyway, see
> > 66fee1bec4c4b021e1b54adcd775cf6e2aa84869.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > libevdev/libevdev.c | 31 +++++++++++++-----
> > test/test-libevdev-events.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 100 insertions(+), 7 deletions(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index 2262284..b035607 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -883,10 +883,12 @@ read_more_events(struct libevdev *dev)
> >
> > /**
> > * Sanitize/modify events where needed.
> > - * @return 0 if untouched, 1 if modified.
> > + * @return 0 if untouched, 1 if modified, -1 if to be discarded
>
> Seems OK, but I really hope you will not add more return codes to
> sanitize events. It's going to be very complicate to understand it
> from a caller point of view.
It's an internal API that's only used in three places. If we add more codes
to that we'll just switch it to an enum but I think for now this will do.
>
> > */
> > static inline int
> > -sanitize_event(const struct libevdev *dev, struct input_event *ev)
> > +sanitize_event(const struct libevdev *dev,
> > + struct input_event *ev,
> > + enum SyncState sync_state)
> > {
> > if (unlikely(dev->num_slots > -1 &&
> > libevdev_event_is_code(ev, EV_ABS, ABS_MT_SLOT) &&
> > @@ -896,6 +898,21 @@ sanitize_event(const struct libevdev *dev, struct input_event *ev)
> > dev->name, ev->value, dev->num_slots - 1);
> > ev->value = dev->num_slots - 1;
> > return 1;
> > +
> > + /* Drop any invalid tracking IDs, they are only supposed to go from
> > + N to -1 or from -1 to N. Never from -1 to -1, or N to M. Very
> > + unlikely to ever happen from a real device.
> > + */
> > + } else if (unlikely(sync_state == SYNC_NONE &&
> > + dev->num_slots > -1 &&
> > + libevdev_event_is_code(ev, EV_ABS, ABS_MT_TRACKING_ID) &&
> > + ((ev->value == -1 &&
> > + *slot_value(dev, dev->current_slot, ABS_MT_TRACKING_ID) == -1) ||
> > + (ev->value != -1 &&
> > + *slot_value(dev, dev->current_slot, ABS_MT_TRACKING_ID) != -1)))) {
> > + log_bug("Device \"%s\" received a double tracking ID %d in slot %d.\n",
> > + dev->name, ev->value, dev->current_slot);
> > + return -1;
> > }
> >
> > return 0;
> > @@ -937,8 +954,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> > of the device too */
> > while (queue_shift(dev, &e) == 0) {
> > dev->queue_nsync--;
> > - sanitize_event(dev, &e);
> > - update_state(dev, &e);
> > + if (sanitize_event(dev, &e, dev->sync_state) != -1)
> > + update_state(dev, &e);
> > }
> >
> > dev->sync_state = SYNC_NONE;
> > @@ -968,8 +985,8 @@ libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event
> > if (queue_shift(dev, ev) != 0)
> > return -EAGAIN;
> >
> > - sanitize_event(dev, ev);
> > - update_state(dev, ev);
> > + if (sanitize_event(dev, ev, dev->sync_state) != -1)
> > + update_state(dev, ev);
> >
> > /* if we disabled a code, get the next event instead */
> > } while(!libevdev_has_event_code(dev, ev->type, ev->code));
> > @@ -1166,7 +1183,7 @@ libevdev_set_event_value(struct libevdev *dev, unsigned int type, unsigned int c
> > e.code = code;
> > e.value = value;
> >
> > - if (sanitize_event(dev, &e))
> > + if (sanitize_event(dev, &e, SYNC_NONE))
> > return -1;
> >
> > switch(type) {
> > diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> > index fda7617..921eda6 100644
> > --- a/test/test-libevdev-events.c
> > +++ b/test/test-libevdev-events.c
> > @@ -1424,6 +1424,81 @@ START_TEST(test_mt_slot_ranges_invalid)
> > }
> > END_TEST
> >
> > +START_TEST(test_mt_tracking_id_discard)
> > +{
> > + struct uinput_device* uidev;
> > + struct libevdev *dev;
> > + int rc;
> > + struct input_event ev;
> > + struct input_absinfo abs[6];
> > +
> > + memset(abs, 0, sizeof(abs));
> > + abs[0].value = ABS_X;
> > + abs[0].maximum = 1000;
> > + abs[1].value = ABS_MT_POSITION_X;
> > + abs[1].maximum = 1000;
> > +
> > + abs[2].value = ABS_Y;
> > + abs[2].maximum = 1000;
> > + abs[3].value = ABS_MT_POSITION_Y;
> > + abs[3].maximum = 1000;
> > +
> > + abs[4].value = ABS_MT_SLOT;
> > + abs[4].maximum = 10;
> > + abs[5].value = ABS_MT_TRACKING_ID;
> > + abs[5].maximum = 500;
> > +
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 6, abs,
> > + EV_SYN, SYN_REPORT,
> > + -1);
> > +
> > + uinput_device_event(uidev, EV_ABS, ABS_MT_SLOT, 1);
> > + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 1);
> > + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> > +
> > + /* second tracking ID on same slot */
> > + uinput_device_event(uidev, EV_ABS, ABS_MT_TRACKING_ID, 2);
> > + uinput_device_event(uidev, EV_SYN, SYN_REPORT, 0);
> > +
> > + libevdev_set_log_function(test_logfunc_ignore_error, NULL);
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> > + ck_assert_int_eq(ev.type, EV_ABS);
> > + ck_assert_int_eq(ev.code, ABS_MT_SLOT);
> > + ck_assert_int_eq(ev.value, 1);
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> > + ck_assert_int_eq(ev.type, EV_ABS);
> > + ck_assert_int_eq(ev.code, ABS_MT_TRACKING_ID);
> > + ck_assert_int_eq(ev.value, 1);
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> > + ck_assert_int_eq(ev.type, EV_SYN);
> > + ck_assert_int_eq(ev.code, SYN_REPORT);
> > + ck_assert_int_eq(ev.value, 0);
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> > +
> > + /* expect tracking ID discarded */
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > + ck_assert_int_eq(rc, LIBEVDEV_READ_STATUS_SUCCESS);
> > + ck_assert_int_eq(ev.type, EV_SYN);
> > + ck_assert_int_eq(ev.code, SYN_REPORT);
> > + ck_assert_int_eq(ev.value, 0);
>
> shouldn't we also test for two consecutive ABS_MT_TRACKING_ID == -1?
> The code checks for it, and it will also be a problem if a device
> closes twice a slot.
good point, I'll add that as a follow-up patch though since it's a bit more
involved and requires the pipe() trickery (otherwise the kernel filters it).
Cheers,
Peter
> > +
> > + libevdev_set_log_function(test_logfunc_abort_on_error, NULL);
> > +
> > + uinput_device_free(uidev);
> > + libevdev_free(dev);
> > +}
> > +END_TEST
> > +
> > START_TEST(test_ev_rep_values)
> > {
> > struct uinput_device* uidev;
> > @@ -1719,6 +1794,7 @@ libevdev_events(void)
> > tcase_add_test(tc, test_mt_event_values);
> > tcase_add_test(tc, test_mt_event_values_invalid);
> > tcase_add_test(tc, test_mt_slot_ranges_invalid);
> > + tcase_add_test(tc, test_mt_tracking_id_discard);
> > tcase_add_test(tc, test_ev_rep_values);
> > suite_add_tcase(s, tc);
> >
> > --
> > 1.9.0
> >
> > _______________________________________________
> > Input-tools mailing list
> > Input-tools at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/input-tools
More information about the Input-tools
mailing list