[PATCH libevdev 2/5] Dynamically allocate the slot values

[Peter Hutterer]

On Tue, Mar 04, 2014 at 12:19:43AM -0500, Benjamin Tissoires wrote:
> 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 :)

for the archive, we've looked at the assembly output of the code with the
current conditions and it looks like most of them are dropped. This should
have almost no performance impact. And, fwiw, having the statements in there
already found two bugs in our current implementation, so I take that as a
definitive argument that we should leave them in.

> >> > 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 :)

I've thought about this again and it really doesn't matter what we use here,
since the only goal is to provide some pointer to data that won't segfault.
ABS_MT_MIN is always available and it's also 0 (since few devices have
TOUCH_MAJOR), so that will just do fine.

So the only changes I have to this patch is to wrap the two conditions in
slot_value() into unlikely() to even further reduce any performance impact.

Cheers,
   Peter