[PATCH libevdev 2/5] Dynamically allocate the slot values
Peter Hutterer
peter.hutterer at who-t.net
Mon Mar 3 19:19:31 PST 2014
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).
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.
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.
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