| Message ID | 1387295334-1744-2-git-send-email-dh.herrmann@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote: > We currently lack support for abs-resolution and abs-value parameters > during uinput ABS initialization. Furthermore, our parsers don't allow > growing ABS_CNT values. Therefore, introduce uinput_user_dev2. > > User-space is free to write uinput_user_dev2 objects instead of > uinput_user_dev legacy objects now. If the kernel lacks support for it, > our comparison for "count != sizeof(struct uinput_user_dev)" will catch > this and return EINVAL. User-space shall retry with the legacy mode then. > > Internally, we transform the legacy object into uinput_user_dev2 and then > handle both the same way. > > The new uinput_user_dev2 object has multiple new features: > - abs payload now has "value" and "resolution" parameters as part of the > switch to "struct input_absinfo". We simply copy these over. > - Our parser allows growing ABS_CNT. We automatically detect the payload > size of the caller, thus, calculating the ABS_CNT the program was > compiled with. > - A "version" field to denote the uinput-version used. This is required > to properly support changing "struct input_user_dev2" changes in the > future. Due to the dynamic ABS_CNT support, we cannot simply add new > fields, as we cannot deduce the structure size from the user-space > given size. Thus, we need the "version" field to allow changing the > object and properly detecting it in our write() handler. > > Signed-off-by: David Herrmann <dh.herrmann@gmail.com> > --- > drivers/input/misc/uinput.c | 142 ++++++++++++++++++++++++++++++++------------ > include/uapi/linux/uinput.h | 9 +++ > 2 files changed, 114 insertions(+), 37 deletions(-) > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c > index 7728359..927ad9a 100644 > --- a/drivers/input/misc/uinput.c > +++ b/drivers/input/misc/uinput.c > @@ -358,14 +358,16 @@ static int uinput_allocate_device(struct uinput_device *udev) > } > > static int uinput_setup_device(struct uinput_device *udev, > - const char __user *buffer, size_t count) > + struct uinput_user_dev2 *user_dev2, > + size_t abscnt) > { > - struct uinput_user_dev *user_dev; > struct input_dev *dev; > int i; > int retval; > + struct input_absinfo *abs; > > - if (count != sizeof(struct uinput_user_dev)) > + /* Ensure name is filled in */ > + if (!user_dev2->name[0]) > return -EINVAL; > > if (!udev->dev) { > @@ -375,37 +377,27 @@ static int uinput_setup_device(struct uinput_device *udev, > } > > dev = udev->dev; > - > - user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev)); > - if (IS_ERR(user_dev)) > - return PTR_ERR(user_dev); > - > - udev->ff_effects_max = user_dev->ff_effects_max; > - > - /* Ensure name is filled in */ > - if (!user_dev->name[0]) { > - retval = -EINVAL; > - goto exit; > - } > + udev->ff_effects_max = user_dev2->ff_effects_max; > > kfree(dev->name); > - dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE, > + dev->name = kstrndup(user_dev2->name, UINPUT_MAX_NAME_SIZE, > GFP_KERNEL); > - if (!dev->name) { > - retval = -ENOMEM; > - goto exit; > - } > - > - dev->id.bustype = user_dev->id.bustype; > - dev->id.vendor = user_dev->id.vendor; > - dev->id.product = user_dev->id.product; > - dev->id.version = user_dev->id.version; > + if (!dev->name) > + return -ENOMEM; > > - for (i = 0; i < ABS_CNT; i++) { > - input_abs_set_max(dev, i, user_dev->absmax[i]); > - input_abs_set_min(dev, i, user_dev->absmin[i]); > - input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]); > - input_abs_set_flat(dev, i, user_dev->absflat[i]); > + dev->id.bustype = user_dev2->id.bustype; > + dev->id.vendor = user_dev2->id.vendor; > + dev->id.product = user_dev2->id.product; > + dev->id.version = user_dev2->id.version; > + > + for (i = 0; i < abscnt; i++) { > + abs = &user_dev2->abs[i]; minor nit: I'd just declare abs here. > + input_abs_set_val(dev, i, abs->value); > + input_abs_set_max(dev, i, abs->maximum); > + input_abs_set_min(dev, i, abs->minimum); > + input_abs_set_fuzz(dev, i, abs->fuzz); > + input_abs_set_flat(dev, i, abs->flat); > + input_abs_set_res(dev, i, abs->resolution); > } > > /* check if absmin/absmax/absfuzz/absflat are filled as > @@ -413,7 +405,7 @@ static int uinput_setup_device(struct uinput_device *udev, > if (test_bit(EV_ABS, dev->evbit)) { > retval = uinput_validate_absbits(dev); > if (retval < 0) > - goto exit; > + return retval; > if (test_bit(ABS_MT_SLOT, dev->absbit)) { > int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1; > input_mt_init_slots(dev, nslot, 0); > @@ -423,11 +415,84 @@ static int uinput_setup_device(struct uinput_device *udev, > } > > udev->state = UIST_SETUP_COMPLETE; > - retval = count; > + return 0; > +} > + > +static int uinput_setup_device1(struct uinput_device *udev, > + const char __user *buffer, size_t count) > +{ > + struct uinput_user_dev *user_dev; > + struct uinput_user_dev2 *user_dev2; > + int i; > + int retval; > + > + if (count != sizeof(struct uinput_user_dev)) > + return -EINVAL; > + > + user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev)); > + if (IS_ERR(user_dev)) > + return PTR_ERR(user_dev); > + > + user_dev2 = kmalloc(sizeof(*user_dev2), GFP_KERNEL); > + if (!user_dev2) { > + kfree(user_dev); > + return -ENOMEM; > + } > + > + user_dev2->version = UINPUT_VERSION; > + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE); > + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id)); you copy the id bits one-by-one into the input_dev but you memcpy it here. is this intentional? > + user_dev2->ff_effects_max = user_dev->ff_effects_max; > + > + for (i = 0; i < ABS_CNT; ++i) { > + user_dev2->abs[i].value = 0; > + user_dev2->abs[i].maximum = user_dev->absmax[i]; > + user_dev2->abs[i].minimum = user_dev->absmin[i]; > + user_dev2->abs[i].fuzz = user_dev->absfuzz[i]; > + user_dev2->abs[i].flat = user_dev->absflat[i]; > + user_dev2->abs[i].resolution = 0; > + } > + > + retval = uinput_setup_device(udev, user_dev2, ABS_CNT); > > - exit: > kfree(user_dev); > - return retval; > + kfree(user_dev2); > + > + return retval ? retval : count; > +} > + > +static int uinput_setup_device2(struct uinput_device *udev, > + const char __user *buffer, size_t count) > +{ > + struct uinput_user_dev2 *user_dev2; > + int retval; > + size_t off, abscnt, max; > + > + /* The first revision of "uinput_user_dev2" is bigger than > + * "uinput_user_dev" and growing. Disallow any smaller payloads. */ > + if (count <= sizeof(struct uinput_user_dev)) > + return -EINVAL; > + > + /* rough check to avoid huge kernel space allocations */ > + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2); > + if (count > max) > + return -EINVAL; > + > + user_dev2 = memdup_user(buffer, count); > + if (IS_ERR(user_dev2)) > + return PTR_ERR(user_dev2); > + > + if (user_dev2->version > UINPUT_VERSION) { > + retval = -EINVAL; > + } else { > + off = offsetof(struct uinput_user_dev2, abs); > + abscnt = (count - off) / sizeof(*user_dev2->abs); > + retval = uinput_setup_device(udev, user_dev2, abscnt); > + } > + I really wish uinput would be a bit easier to debug than just returning -EINVAL when it's not happy. having said that, the only errno that would remotely make sense is -ERANGE for count > max and even that is a bit meh. Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net> Cheers, Peter > + kfree(user_dev2); > + > + return retval ? retval : count; > } > > static ssize_t uinput_inject_events(struct uinput_device *udev, > @@ -469,9 +534,12 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, > if (retval) > return retval; > > - retval = udev->state == UIST_CREATED ? > - uinput_inject_events(udev, buffer, count) : > - uinput_setup_device(udev, buffer, count); > + if (udev->state == UIST_CREATED) > + retval = uinput_inject_events(udev, buffer, count); > + else if (count <= sizeof(struct uinput_user_dev)) > + retval = uinput_setup_device1(udev, buffer, count); > + else > + retval = uinput_setup_device2(udev, buffer, count); > > mutex_unlock(&udev->mutex); > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h > index fe46431..c2e8710 100644 > --- a/include/uapi/linux/uinput.h > +++ b/include/uapi/linux/uinput.h > @@ -134,4 +134,13 @@ struct uinput_user_dev { > __s32 absfuzz[ABS_CNT]; > __s32 absflat[ABS_CNT]; > }; > + > +struct uinput_user_dev2 { > + __u8 version; > + char name[UINPUT_MAX_NAME_SIZE]; > + struct input_id id; > + __u32 ff_effects_max; > + struct input_absinfo abs[ABS_CNT]; > +}; > + > #endif /* _UAPI__UINPUT_H_ */ > -- > 1.8.5.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote: > + > +struct uinput_user_dev2 { > + __u8 version; It does not make sense to have version u8 since we going to have padding (1 byte I believe) padding between name and id. > + char name[UINPUT_MAX_NAME_SIZE]; > + struct input_id id; > + __u32 ff_effects_max; > + struct input_absinfo abs[ABS_CNT]; > +}; > + > #endif /* _UAPI__UINPUT_H_ */ > -- > 1.8.5.1 >
On Thu, Dec 19, 2013 at 08:27:32AM +1000, Peter Hutterer wrote: > On Tue, Dec 17, 2013 at 04:48:51PM +0100, David Herrmann wrote: > > + > > + user_dev2->version = UINPUT_VERSION; > > + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE); > > + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id)); > > you copy the id bits one-by-one into the input_dev but you memcpy it here. > is this intentional? That should simply be: user_dev2->id = user_dev->id; and in othe rplace as well I think. > > > + user_dev2->ff_effects_max = user_dev->ff_effects_max; > > + > > + for (i = 0; i < ABS_CNT; ++i) { > > + user_dev2->abs[i].value = 0; > > + user_dev2->abs[i].maximum = user_dev->absmax[i]; > > + user_dev2->abs[i].minimum = user_dev->absmin[i]; > > + user_dev2->abs[i].fuzz = user_dev->absfuzz[i]; > > + user_dev2->abs[i].flat = user_dev->absflat[i]; > > + user_dev2->abs[i].resolution = 0; > > + } > > + > > + retval = uinput_setup_device(udev, user_dev2, ABS_CNT); > > > > - exit: > > kfree(user_dev); > > - return retval; > > + kfree(user_dev2); > > + > > + return retval ? retval : count; > > +} > > + > > +static int uinput_setup_device2(struct uinput_device *udev, > > + const char __user *buffer, size_t count) > > +{ > > + struct uinput_user_dev2 *user_dev2; > > + int retval; > > + size_t off, abscnt, max; > > + > > + /* The first revision of "uinput_user_dev2" is bigger than > > + * "uinput_user_dev" and growing. Disallow any smaller payloads. */ > > + if (count <= sizeof(struct uinput_user_dev)) > > + return -EINVAL; > > + > > + /* rough check to avoid huge kernel space allocations */ > > + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2); > > + if (count > max) > > + return -EINVAL; > > + > > + user_dev2 = memdup_user(buffer, count); > > + if (IS_ERR(user_dev2)) > > + return PTR_ERR(user_dev2); > > + > > + if (user_dev2->version > UINPUT_VERSION) { > > + retval = -EINVAL; > > + } else { > > + off = offsetof(struct uinput_user_dev2, abs); > > + abscnt = (count - off) / sizeof(*user_dev2->abs); > > + retval = uinput_setup_device(udev, user_dev2, abscnt); > > + } > > + > > I really wish uinput would be a bit easier to debug than just returning > -EINVAL when it's not happy. having said that, the only errno that would > remotely make sense is -ERANGE for count > max and even that is a bit meh. Maybe we should add a few dev_dbg() and rely on having dynamic debug to activate logging on demand? Thanks.
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c index 7728359..927ad9a 100644 --- a/drivers/input/misc/uinput.c +++ b/drivers/input/misc/uinput.c @@ -358,14 +358,16 @@ static int uinput_allocate_device(struct uinput_device *udev) } static int uinput_setup_device(struct uinput_device *udev, - const char __user *buffer, size_t count) + struct uinput_user_dev2 *user_dev2, + size_t abscnt) { - struct uinput_user_dev *user_dev; struct input_dev *dev; int i; int retval; + struct input_absinfo *abs; - if (count != sizeof(struct uinput_user_dev)) + /* Ensure name is filled in */ + if (!user_dev2->name[0]) return -EINVAL; if (!udev->dev) { @@ -375,37 +377,27 @@ static int uinput_setup_device(struct uinput_device *udev, } dev = udev->dev; - - user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev)); - if (IS_ERR(user_dev)) - return PTR_ERR(user_dev); - - udev->ff_effects_max = user_dev->ff_effects_max; - - /* Ensure name is filled in */ - if (!user_dev->name[0]) { - retval = -EINVAL; - goto exit; - } + udev->ff_effects_max = user_dev2->ff_effects_max; kfree(dev->name); - dev->name = kstrndup(user_dev->name, UINPUT_MAX_NAME_SIZE, + dev->name = kstrndup(user_dev2->name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL); - if (!dev->name) { - retval = -ENOMEM; - goto exit; - } - - dev->id.bustype = user_dev->id.bustype; - dev->id.vendor = user_dev->id.vendor; - dev->id.product = user_dev->id.product; - dev->id.version = user_dev->id.version; + if (!dev->name) + return -ENOMEM; - for (i = 0; i < ABS_CNT; i++) { - input_abs_set_max(dev, i, user_dev->absmax[i]); - input_abs_set_min(dev, i, user_dev->absmin[i]); - input_abs_set_fuzz(dev, i, user_dev->absfuzz[i]); - input_abs_set_flat(dev, i, user_dev->absflat[i]); + dev->id.bustype = user_dev2->id.bustype; + dev->id.vendor = user_dev2->id.vendor; + dev->id.product = user_dev2->id.product; + dev->id.version = user_dev2->id.version; + + for (i = 0; i < abscnt; i++) { + abs = &user_dev2->abs[i]; + input_abs_set_val(dev, i, abs->value); + input_abs_set_max(dev, i, abs->maximum); + input_abs_set_min(dev, i, abs->minimum); + input_abs_set_fuzz(dev, i, abs->fuzz); + input_abs_set_flat(dev, i, abs->flat); + input_abs_set_res(dev, i, abs->resolution); } /* check if absmin/absmax/absfuzz/absflat are filled as @@ -413,7 +405,7 @@ static int uinput_setup_device(struct uinput_device *udev, if (test_bit(EV_ABS, dev->evbit)) { retval = uinput_validate_absbits(dev); if (retval < 0) - goto exit; + return retval; if (test_bit(ABS_MT_SLOT, dev->absbit)) { int nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1; input_mt_init_slots(dev, nslot, 0); @@ -423,11 +415,84 @@ static int uinput_setup_device(struct uinput_device *udev, } udev->state = UIST_SETUP_COMPLETE; - retval = count; + return 0; +} + +static int uinput_setup_device1(struct uinput_device *udev, + const char __user *buffer, size_t count) +{ + struct uinput_user_dev *user_dev; + struct uinput_user_dev2 *user_dev2; + int i; + int retval; + + if (count != sizeof(struct uinput_user_dev)) + return -EINVAL; + + user_dev = memdup_user(buffer, sizeof(struct uinput_user_dev)); + if (IS_ERR(user_dev)) + return PTR_ERR(user_dev); + + user_dev2 = kmalloc(sizeof(*user_dev2), GFP_KERNEL); + if (!user_dev2) { + kfree(user_dev); + return -ENOMEM; + } + + user_dev2->version = UINPUT_VERSION; + memcpy(user_dev2->name, user_dev->name, UINPUT_MAX_NAME_SIZE); + memcpy(&user_dev2->id, &user_dev->id, sizeof(struct input_id)); + user_dev2->ff_effects_max = user_dev->ff_effects_max; + + for (i = 0; i < ABS_CNT; ++i) { + user_dev2->abs[i].value = 0; + user_dev2->abs[i].maximum = user_dev->absmax[i]; + user_dev2->abs[i].minimum = user_dev->absmin[i]; + user_dev2->abs[i].fuzz = user_dev->absfuzz[i]; + user_dev2->abs[i].flat = user_dev->absflat[i]; + user_dev2->abs[i].resolution = 0; + } + + retval = uinput_setup_device(udev, user_dev2, ABS_CNT); - exit: kfree(user_dev); - return retval; + kfree(user_dev2); + + return retval ? retval : count; +} + +static int uinput_setup_device2(struct uinput_device *udev, + const char __user *buffer, size_t count) +{ + struct uinput_user_dev2 *user_dev2; + int retval; + size_t off, abscnt, max; + + /* The first revision of "uinput_user_dev2" is bigger than + * "uinput_user_dev" and growing. Disallow any smaller payloads. */ + if (count <= sizeof(struct uinput_user_dev)) + return -EINVAL; + + /* rough check to avoid huge kernel space allocations */ + max = ABS_CNT * sizeof(*user_dev2->abs) + sizeof(*user_dev2); + if (count > max) + return -EINVAL; + + user_dev2 = memdup_user(buffer, count); + if (IS_ERR(user_dev2)) + return PTR_ERR(user_dev2); + + if (user_dev2->version > UINPUT_VERSION) { + retval = -EINVAL; + } else { + off = offsetof(struct uinput_user_dev2, abs); + abscnt = (count - off) / sizeof(*user_dev2->abs); + retval = uinput_setup_device(udev, user_dev2, abscnt); + } + + kfree(user_dev2); + + return retval ? retval : count; } static ssize_t uinput_inject_events(struct uinput_device *udev, @@ -469,9 +534,12 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer, if (retval) return retval; - retval = udev->state == UIST_CREATED ? - uinput_inject_events(udev, buffer, count) : - uinput_setup_device(udev, buffer, count); + if (udev->state == UIST_CREATED) + retval = uinput_inject_events(udev, buffer, count); + else if (count <= sizeof(struct uinput_user_dev)) + retval = uinput_setup_device1(udev, buffer, count); + else + retval = uinput_setup_device2(udev, buffer, count); mutex_unlock(&udev->mutex); diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h index fe46431..c2e8710 100644 --- a/include/uapi/linux/uinput.h +++ b/include/uapi/linux/uinput.h @@ -134,4 +134,13 @@ struct uinput_user_dev { __s32 absfuzz[ABS_CNT]; __s32 absflat[ABS_CNT]; }; + +struct uinput_user_dev2 { + __u8 version; + char name[UINPUT_MAX_NAME_SIZE]; + struct input_id id; + __u32 ff_effects_max; + struct input_absinfo abs[ABS_CNT]; +}; + #endif /* _UAPI__UINPUT_H_ */
We currently lack support for abs-resolution and abs-value parameters during uinput ABS initialization. Furthermore, our parsers don't allow growing ABS_CNT values. Therefore, introduce uinput_user_dev2. User-space is free to write uinput_user_dev2 objects instead of uinput_user_dev legacy objects now. If the kernel lacks support for it, our comparison for "count != sizeof(struct uinput_user_dev)" will catch this and return EINVAL. User-space shall retry with the legacy mode then. Internally, we transform the legacy object into uinput_user_dev2 and then handle both the same way. The new uinput_user_dev2 object has multiple new features: - abs payload now has "value" and "resolution" parameters as part of the switch to "struct input_absinfo". We simply copy these over. - Our parser allows growing ABS_CNT. We automatically detect the payload size of the caller, thus, calculating the ABS_CNT the program was compiled with. - A "version" field to denote the uinput-version used. This is required to properly support changing "struct input_user_dev2" changes in the future. Due to the dynamic ABS_CNT support, we cannot simply add new fields, as we cannot deduce the structure size from the user-space given size. Thus, we need the "version" field to allow changing the object and properly detecting it in our write() handler. Signed-off-by: David Herrmann <dh.herrmann@gmail.com> --- drivers/input/misc/uinput.c | 142 ++++++++++++++++++++++++++++++++------------ include/uapi/linux/uinput.h | 9 +++ 2 files changed, 114 insertions(+), 37 deletions(-)