[PATCH libevdev 2/5] Dynamically allocate the slot values
Benjamin Tissoires
benjamin.tissoires at gmail.com
Mon Mar 3 21:19:43 PST 2014
On Tue, Mar 4, 2014 at 12:16 AM, Peter Hutterer
<peter.hutterer at who-t.net> wrote:
> 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.
>
yeah me too. I still hope that the compiler would be smart enough to
remove the tests regarding the axis (against a plain value used in the
loops and in the tests), I am a little less sure about the slot, but
gcc is so powerful :)
>> > 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.
the defines are fine. Just don't use ABS_MT_MIN if you want ABS_MT_POSITION_X :)
Cheers,
Benjamin
>
> 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