Re: [PATCH 1/4] Input: uinput: add full absinfo support

From: Peter Hutterer
Date: Wed Dec 18 2013 - 17:44:59 EST

Next message: Andy Lutomirski: "Re: [RFC PATCH 00/13] nohz: Use sysidle detection to let thetimekeeper sleep"
Previous message: David Miller: "Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c"
In reply to: David Herrmann: "[PATCH 1/4] Input: uinput: add full absinfo support"
Next in thread: David Herrmann: "[PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>

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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Andy Lutomirski: "Re: [RFC PATCH 00/13] nohz: Use sysidle detection to let thetimekeeper sleep"
Previous message: David Miller: "Re: [PATCH] drivers: ptp: Include new header file in ptp_pch.c"
In reply to: David Herrmann: "[PATCH 1/4] Input: uinput: add full absinfo support"
Next in thread: David Herrmann: "[PATCH 2/4] Input: introduce ABS_MAX2/CNT2 and friends"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]