[PATCH libevdev 1/6] Cap slot values to the announced maximum
Peter Hutterer
peter.hutterer at who-t.net
Thu Mar 6 16:23:44 PST 2014
On Thu, Mar 06, 2014 at 05:57:24PM -0500, Benjamin Tissoires wrote:
> On Wed, Mar 5, 2014 at 11:44 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > A malicious device may announce N slots but then send a slot index >= N. The
> > slot state is almost always allocated (definitely the case in libevdev and
> > true for most callers), so providing a slot number higher than the announced
> > maximum is likely to lead to invalid dereferences. Don't allow that.
> > Likewise, don't allow negative slot numbers.
> >
> > Note that the kernel filters these events anyway, the only way to trigger this
> > is to change the device fd to something outside the kernel's control.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> > libevdev/libevdev.c | 24 ++++++++++++++
> > libevdev/libevdev.h | 7 +++-
> > test/test-libevdev-events.c | 78 +++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 108 insertions(+), 1 deletion(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index a77c466..d9e2517 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -809,6 +809,26 @@ read_more_events(struct libevdev *dev)
> > return 0;
> > }
> >
> > +/**
> > + * Sanitize/modify events where needed.
> > + * @return 0 if untouched, 1 if modified.
> > + */
> > +static inline int
> > +sanitize_event(const struct libevdev *dev, struct input_event *ev)
> > +{
> > + if (unlikely(dev->num_slots > -1 &&
> > + libevdev_event_is_code(ev, EV_ABS, ABS_MT_SLOT) &&
> > + (ev->value < 0 || ev->value >= dev->num_slots))) {
> > + log_bug("Device %s received an invalid slot index %d."
> > + "Capping to announced max slot number %d.\n",
> > + dev->name, ev->value, dev->num_slots - 1);
> > + ev->value = dev->num_slots - 1;
>
> just nitpicking here, if the slot is < 0, I would have used 0 instead
> of dev->num_slots - 1. But it does not matters that much.
I thought about that, but really, it's just extra code that will hardly ever
get triggered anyway and either way returns a value that's already wrong
anyway, so I don't think we need to put any effort in here.
Cheers,
Peter
>
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires at gmail.com>
>
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > LIBEVDEV_EXPORT int
> > libevdev_next_event(struct libevdev *dev, unsigned int flags, struct input_event *ev)
> > {
> > @@ -875,6 +895,7 @@ 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 we disabled a code, get the next event instead */
> > @@ -1072,6 +1093,9 @@ libevdev_set_event_value(struct libevdev *dev, unsigned int type, unsigned int c
> > e.code = code;
> > e.value = value;
> >
> > + if (sanitize_event(dev, &e))
> > + return -1;
> > +
> > switch(type) {
> > case EV_ABS: rc = update_abs_state(dev, &e); break;
> > case EV_KEY: rc = update_key_state(dev, &e); break;
> > diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> > index eab02ba..1855cd3 100644
> > --- a/libevdev/libevdev.h
> > +++ b/libevdev/libevdev.h
> > @@ -1111,6 +1111,10 @@ int libevdev_get_event_value(const struct libevdev *dev, unsigned int type, unsi
> > * event code is the value of the currently active slot. You should use
> > * libevdev_set_slot_value() instead.
> > *
> > + * If the device supports ABS_MT_SLOT and the type is EV_ABS and the code is
> > + * ABS_MT_SLOT, the value must be a positive number less then the number of
> > + * slots on the device. Otherwise, libevdev_set_event_value() returns -1.
> > + *
> > * @param dev The evdev device, already initialized with libevdev_set_fd()
> > * @param type The event type for the code to query (EV_SYN, EV_REL, etc.)
> > * @param code The event code to set the value for, one of ABS_X, LED_NUML, etc.
> > @@ -1118,7 +1122,8 @@ int libevdev_get_event_value(const struct libevdev *dev, unsigned int type, unsi
> > *
> > * @return 0 on success, or -1 on failure.
> > * @retval -1 the device does not have the event type or code enabled, or the code is outside the
> > - * allowed limits for the given type, or the type cannot be set.
> > + * allowed limits for the given type, or the type cannot be set, or the
> > + * value is not permitted for the given code.
> > *
> > * @see libevdev_set_slot_value
> > * @see libevdev_get_event_value
> > diff --git a/test/test-libevdev-events.c b/test/test-libevdev-events.c
> > index c9ceb33..8d1bd8b 100644
> > --- a/test/test-libevdev-events.c
> > +++ b/test/test-libevdev-events.c
> > @@ -1143,6 +1143,83 @@ START_TEST(test_mt_event_values_invalid)
> > }
> > END_TEST
> >
> > +START_TEST(test_mt_slot_ranges_invalid)
> > +{
> > + struct uinput_device* uidev;
> > + struct libevdev *dev;
> > + struct input_event ev[2];
> > + int rc;
> > + struct input_absinfo abs[5];
> > + int num_slots = 2;
> > + int pipefd[2];
> > +
> > + 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 = num_slots - 1;
> > +
> > + rc = test_create_abs_device(&uidev, &dev,
> > + 5, abs,
> > + EV_SYN, SYN_REPORT,
> > + -1);
> > + ck_assert_msg(rc == 0, "Failed to create device: %s", strerror(-rc));
> > +
> > + rc = pipe2(pipefd, O_NONBLOCK);
> > + ck_assert_int_eq(rc, 0);
> > + libevdev_change_fd(dev, pipefd[0]);
> > +
> > + ev[0].type = EV_ABS;
> > + ev[0].code = ABS_MT_SLOT;
> > + ev[0].value = num_slots;
> > + ev[1].type = EV_SYN;
> > + ev[1].code = SYN_REPORT;
> > + ev[1].value = 0;
> > + rc = write(pipefd[1], ev, sizeof(ev));
> > + ck_assert_int_eq(rc, sizeof(ev));
> > +
> > + libevdev_set_log_function(test_logfunc_ignore_error, NULL);
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, ev);
> > + ck_assert(libevdev_event_is_code(ev, EV_ABS, ABS_MT_SLOT));
> > + ck_assert_int_eq(ev[0].value, num_slots - 1);
> > +
> > + /* drain the EV_SYN */
> > + libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, ev);
> > +
> > + ev[0].type = EV_ABS;
> > + ev[0].code = ABS_MT_SLOT;
> > + ev[0].value = -1;
> > + ev[1].type = EV_SYN;
> > + ev[1].code = SYN_REPORT;
> > + ev[1].value = 0;
> > + rc = write(pipefd[1], ev, sizeof(ev));
> > + ck_assert_int_eq(rc, sizeof(ev));
> > +
> > + rc = libevdev_next_event(dev, LIBEVDEV_READ_FLAG_NORMAL, ev);
> > + ck_assert(libevdev_event_is_code(ev, EV_ABS, ABS_MT_SLOT));
> > + ck_assert_int_eq(ev[0].value, num_slots - 1);
> > +
> > + ck_assert_int_eq(libevdev_get_current_slot(dev), num_slots - 1);
> > +
> > + ck_assert_int_eq(libevdev_set_event_value(dev, EV_ABS, ABS_MT_SLOT, num_slots), -1);
> > + ck_assert_int_eq(libevdev_set_event_value(dev, EV_ABS, ABS_MT_SLOT, -1), -1);
> > +
> > + 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;
> > @@ -1447,6 +1524,7 @@ libevdev_events(void)
> > tcase_add_test(tc, test_event_values_invalid);
> > 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_ev_rep_values);
> > suite_add_tcase(s, tc);
> >
> > --
> > 1.8.5.3
> >
> > _______________________________________________
> > 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