[PATCH libevdev 2/5] Dynamically allocate the slot values
Peter Hutterer
peter.hutterer at who-t.net
Mon Mar 3 21:16:24 PST 2014
On Mon, Mar 03, 2014 at 11:41:37PM -0500, Benjamin Tissoires wrote:
> On Mon, Mar 3, 2014 at 10:19 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > On Mon, Mar 03, 2014 at 10:37:50AM -0500, Benjamin Tissoires wrote:
> >> On Thu, Feb 27, 2014 at 1:27 AM, Peter Hutterer
> >> <peter.hutterer at who-t.net> wrote:
> >> > Instead of relying on a static MAX_SLOTS array, allocated it based on the
> >> > number of slots we have on the device. The previous checks for MAX_SLOTS were
> >> > incomplete, causing out-of-bound reads.
> >> >
> >> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> >> > ---
> >> > libevdev/libevdev-int.h | 2 +-
> >> > libevdev/libevdev.c | 41 +++++++++++++++++++++++++++++++----------
> >> > 2 files changed, 32 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/libevdev/libevdev-int.h b/libevdev/libevdev-int.h
> >> > index 847fe56..42141ca 100644
> >> > --- a/libevdev/libevdev-int.h
> >> > +++ b/libevdev/libevdev-int.h
> >> > @@ -95,7 +95,7 @@ struct libevdev {
> >> > unsigned long led_values[NLONGS(LED_CNT)];
> >> > unsigned long sw_values[NLONGS(SW_CNT)];
> >> > struct input_absinfo abs_info[ABS_CNT];
> >> > - int mt_slot_vals[MAX_SLOTS][ABS_MT_CNT];
> >> > + int *mt_slot_vals; /* [num_slots * ABS_MT_CNT] */
> >> > int num_slots; /**< valid slots in mt_slot_vals */
> >> > int current_slot;
> >> > int rep_values[REP_CNT];
> >> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> >> > index 36359d4..4f5062d 100644
> >> > --- a/libevdev/libevdev.c
> >> > +++ b/libevdev/libevdev.c
> >> > @@ -38,6 +38,21 @@
> >> >
> >> > static int sync_mt_state(struct libevdev *dev, int create_events);
> >> >
> >> > +static inline int*
> >> > +slot_value(const struct libevdev *dev, int slot, int axis)
> >> > +{
> >> > + if (slot > dev->num_slots) {
> >> > + log_bug("Slot %d exceeds number of slots (%d)\n", slot, dev->num_slots);
> >> > + slot = 0;
> >> > + }
> >> > + if (axis < ABS_MT_MIN || axis > ABS_MT_MAX) {
> >> > + log_bug("MT axis %d is outside the valid range [%d,%d]\n",
> >> > + axis, ABS_MT_MIN, ABS_MT_MAX);
> >> > + axis = ABS_MT_MIN;
> >> > + }
> >> > + return &dev->mt_slot_vals[slot * ABS_MT_CNT + axis - ABS_MT_MIN];
> >> > +}
> >> > +
> >>
> >> I am a little bit puzzled by this one.
> >> If the slot is not valid or if the axis is not valid, we return a
> >> random address, which may be used to overwrite actual data.
> >> Also, in some places later, the code actually checks if slot is valid,
> >> which means that we do not have the log_bug() message there.
> >>
> >> How about splitting this function in 2 (or 3), 1 (or 2) to check the
> >> validity of slots and axis, and the last would return the pointer,
> >> (maybe return NULL if there is an error in slots or axis)?
> >
> > the reason for this approach is that this is a function used exclusively
> > internally and I don't want to have error checks around every instance, only
> > where we need to worry about user input. elsewhere, we can assume that the
> > input data is correct (after the basic checks we need anyway).
>
> Ok, but with this implementation, you do run the checks every call,
> while 98% of them are not required.
>
> >
> > so in theory, this should never happen. when it does, return something that
> > we can work with nonetheless (as opposed to potentially bringing your
> > compositor down), log a bug and continue, albeit slightly wobbly now. this
> > is a case where having a unlikely() compiler macro would be useful since we
> > don't expect this to ever be in the hot path.
>
> Agree for unlikely(). I may say something dumb, but can't we just skip
> the tests in slot_value(), put big warnings around it (comments), and
> put extra care where there is no guarantee that the slots or axis are
> not correct.
> I know tests do not cost that much, but still, in sync_mt_state in the
> worse case, you are making (3 * 3 * ABS_MT_CNT * min(dev->num_slots,
> MAX_SLOTS)) = 8100 (ok, there is no mt device with 60 touches which
> exports all the axes and the axes need to be changed all at once). Or
> maybe we count on the compiler to prefetch the branches?
the main reason is that this is otherwise unchecked memory that we're
writing to. if we forget a check somewhere where we should (yes yes, tests,
blah blah) then that opens up a potential attack vector, and a quite trivial
one as well since you have a pretty nicely indexed array to write whatever
you want wherever you want. I think unlikely() is the best approach here.
it'd be interesting to see what the real performance impact is here though.
> > fwiw, when it comes to returning something random, ABS_MT_POSITION_X in slot
> > 0 is the safest to return, it'll exist on almost every touch device.
>
> so why do you return ABS_MT_TOUCH_MAJOR? :)
> The code set ABS_MT_MIN as the axis, so this gives us the first
> MT_AXIS in the slot offset, and the first one is ABS_MT_TOUCH_MAJOR,
> not ABS_MT_POSITION_X...
whoah, good point, that's not quite what I wanted. mixed up my #defines
here.
Cheers,
Peter
> >> > init_event_queue(struct libevdev *dev)
> >> > {
> >> > @@ -136,6 +151,7 @@ libevdev_reset(struct libevdev *dev)
> >> > free(dev->name);
> >> > free(dev->phys);
> >> > free(dev->uniq);
> >> > + free(dev->mt_slot_vals);
> >> > memset(dev, 0, sizeof(*dev));
> >> > dev->fd = -1;
> >> > dev->initialized = false;
> >> > @@ -187,6 +203,7 @@ libevdev_free(struct libevdev *dev)
> >> > free(dev->name);
> >> > free(dev->phys);
> >> > free(dev->uniq);
> >> > + free(dev->mt_slot_vals);
> >> > queue_free(dev);
> >> > free(dev);
> >> > }
> >> > @@ -369,6 +386,11 @@ libevdev_set_fd(struct libevdev* dev, int fd)
> >> > if (i == ABS_MT_SLOT &&
> >> > !libevdev_has_event_code(dev, EV_ABS, ABS_MT_SLOT - 1)) {
> >> > dev->num_slots = abs_info.maximum + 1;
> >> > + dev->mt_slot_vals = calloc(dev->num_slots * ABS_MT_CNT, sizeof(int));
> >> > + if (!dev->mt_slot_vals) {
> >> > + rc = -ENOMEM;
> >> > + goto out;
> >> > + }
> >> > dev->current_slot = abs_info.value;
> >> > }
> >> >
> >> > @@ -579,14 +601,14 @@ sync_mt_state(struct libevdev *dev, int create_events)
> >> > if (!libevdev_has_event_code(dev, EV_ABS, j))
> >> > continue;
> >> >
> >> > - if (dev->mt_slot_vals[i][jdx] == mt_state[jdx].val[i])
> >> > + if (*slot_value(dev, i, j) == mt_state[jdx].val[i])
> >> > continue;
> >> >
> >> > if (create_events) {
> >> > ev = queue_push(dev);
> >> > init_event(dev, ev, EV_ABS, j, mt_state[jdx].val[i]);
> >> > }
> >> > - dev->mt_slot_vals[i][jdx] = mt_state[jdx].val[i];
> >> > + *slot_value(dev, i, j) = mt_state[jdx].val[i];
> >> > }
> >> > }
> >> >
> >> > @@ -667,14 +689,14 @@ update_mt_state(struct libevdev *dev, const struct input_event *e)
> >> > /* sync abs_info with the current slot values */
> >> > for (i = ABS_MT_SLOT + 1; i <= ABS_MT_MAX; i++) {
> >> > if (libevdev_has_event_code(dev, EV_ABS, i))
> >> > - dev->abs_info[i].value = dev->mt_slot_vals[dev->current_slot][i - ABS_MT_MIN];
> >> > + dev->abs_info[i].value = *slot_value(dev, dev->current_slot, i);
> >> > }
> >> >
> >> > return 0;
> >> > } else if (dev->current_slot == -1)
> >> > return 1;
> >> >
> >> > - dev->mt_slot_vals[dev->current_slot][e->code - ABS_MT_MIN] = e->value;
> >> > + *slot_value(dev, dev->current_slot, e->code) = e->value;
> >> >
> >> > return 0;
> >> > }
> >> > @@ -1070,13 +1092,13 @@ libevdev_get_slot_value(const struct libevdev *dev, unsigned int slot, unsigned
> >> > if (!libevdev_has_event_type(dev, EV_ABS) || !libevdev_has_event_code(dev, EV_ABS, code))
> >> > return 0;
> >> >
> >> > - if (dev->num_slots < 0 || slot >= (unsigned int)dev->num_slots || slot >= MAX_SLOTS)
> >> > + if (dev->num_slots < 0 || slot >= (unsigned int)dev->num_slots)
> >> > return 0;
> >> >
> >> > if (code > ABS_MT_MAX || code < ABS_MT_MIN)
> >> > return 0;
> >> >
> >> > - return dev->mt_slot_vals[slot][code - ABS_MT_MIN];
> >> > + return *slot_value(dev, slot, code);
> >> > }
> >> >
> >> > LIBEVDEV_EXPORT int
> >> > @@ -1085,7 +1107,7 @@ libevdev_set_slot_value(struct libevdev *dev, unsigned int slot, unsigned int co
> >> > if (!libevdev_has_event_type(dev, EV_ABS) || !libevdev_has_event_code(dev, EV_ABS, code))
> >> > return -1;
> >> >
> >> > - if (dev->num_slots == -1 || slot >= (unsigned int)dev->num_slots || slot >= MAX_SLOTS)
> >> > + if (dev->num_slots == -1 || slot >= (unsigned int)dev->num_slots)
> >> > return -1;
> >> >
> >> > if (code > ABS_MT_MAX || code < ABS_MT_MIN)
> >> > @@ -1097,8 +1119,7 @@ libevdev_set_slot_value(struct libevdev *dev, unsigned int slot, unsigned int co
> >> > dev->current_slot = value;
> >> > }
> >> >
> >> > - dev->mt_slot_vals[slot][code - ABS_MT_MIN] = value;
> >> > -
> >> > + *slot_value(dev, slot, code) = value;
> >> >
> >> > return 0;
> >> > }
> >> > @@ -1109,7 +1130,7 @@ libevdev_fetch_slot_value(const struct libevdev *dev, unsigned int slot, unsigne
> >> > if (libevdev_has_event_type(dev, EV_ABS) &&
> >> > libevdev_has_event_code(dev, EV_ABS, code) &&
> >> > dev->num_slots >= 0 &&
> >> > - slot < (unsigned int)dev->num_slots && slot < MAX_SLOTS) {
> >> > + slot < (unsigned int)dev->num_slots) {
> >> > *value = libevdev_get_slot_value(dev, slot, code);
> >> > return 1;
> >> > } else
> >> > --
> >> > 1.8.4.2
> >> >
> >> > _______________________________________________
> >> > 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