| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20150825150421.GB12421@mail.corp.redhat.com>
Date: Tue, 25 Aug 2015 11:04:21 -0400
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: David Herrmann <dh.herrmann@...il.com>
Cc: Dmitry Torokhov <dmitry.torokhov@...il.com>,
Peter Hutterer <peter.hutterer@...-t.net>,
"open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] Input: uinput - add new UINPUT_DEV_SETUP and
UI_ABS_SETUP ioctl
On Aug 24 2015 or thereabouts, David Herrmann wrote:
> Hi
>
> On Fri, Aug 21, 2015 at 4:36 PM, Benjamin Tissoires
> <benjamin.tissoires@...hat.com> wrote:
> > This adds two new ioctls, UINPUT_DEV_SETUP and UI_ABS_SETUP, that
> > replaces the old device setup method (by write()'ing "struct
> > uinput_user_dev" to the node). The old method is not easily extendable
> > and requires huge payloads. Furthermore, overloading write() without
> > properly versioned objects is error-prone.
> >
> > Therefore, we introduce two new ioctls to replace the old method.
> > These ioctls support all features of the old method, plus a "resolution"
> > field for absinfo. Furthermore, it's properly forward-compatible to new
> > ABS codes and a growing "struct input_absinfo" structure.
> >
> > UI_ABS_SETUP also allows user-space to skip unknown axes if not set.
> > There is no need to copy the whole array temporarily into the kernel,
> > but instead the caller issues several ioctl where we copy each value
> > manually.
> >
> > Signed-off-by: David Herrmann <dh.herrmann@...il.com>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> > ---
> >
> > Hi,
> >
> > this is the v2 of the patch David submitted last year. I tried to take
> > Dmitry's remarks into account and came out with this.
> >
> > Cheers,
> > Benjamin
> >
> > drivers/input/misc/uinput.c | 83 +++++++++++++++++++++++++++++++++++++++++++--
> > include/linux/uinput.h | 5 +++
> > include/uapi/linux/uinput.h | 83 +++++++++++++++++++++++++++++++++++++++++++--
> > 3 files changed, 165 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > index 345df9b..afbcc41 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -370,8 +370,76 @@ static int uinput_allocate_device(struct uinput_device *udev)
> > return 0;
> > }
> >
> > -static int uinput_setup_device(struct uinput_device *udev,
> > - const char __user *buffer, size_t count)
> > +static int uinput_dev_setup(struct uinput_device *udev,
> > + struct uinput_setup __user *arg)
> > +{
> > + struct uinput_setup setup;
> > + struct input_dev *dev;
> > + int retval;
> > +
> > + if (udev->state == UIST_CREATED)
> > + return -EINVAL;
> > + if (copy_from_user(&setup, arg, sizeof(setup)))
> > + return -EFAULT;
> > + if (!setup.name[0])
> > + return -EINVAL;
> > +
> > + dev = udev->dev;
> > + dev->id = setup.id;
> > + udev->ff_effects_max = setup.ff_effects_max;
> > +
> > + kfree(dev->name);
> > + dev->name = kstrndup(setup.name, UINPUT_MAX_NAME_SIZE, GFP_KERNEL);
> > + if (!dev->name)
> > + return -ENOMEM;
> > +
> > + retval = uinput_validate_absbits(dev);
> > + if (retval < 0)
> > + return retval;
> > +
> > + udev->state = UIST_SETUP_COMPLETE;
> > + return 0;
> > +}
> > +
> > +static int uinput_abs_setup(struct uinput_device *udev,
> > + struct uinput_setup __user *arg, size_t size)
> > +{
> > + struct uinput_abs_setup setup;
> > + struct input_dev *dev;
> > +
> > + memset(&setup, 0, sizeof(setup));
>
> This could be simplified as:
>
> struct uinput_abs_setup setup = {};
>
> > +
> > + if (size > sizeof(setup))
> > + return -EINVAL;
>
> Maybe use something more specific here. This is something user-space
> may want to match for if the ioctl struct gets extended. E2BIG?
>
> > + if (udev->state == UIST_CREATED)
> > + return -EINVAL;
> > + if (copy_from_user(&setup, arg, size))
> > + return -EFAULT;
> > + if (setup.code > ABS_MAX)
> > + return -ERANGE;
> > +
> > + dev = udev->dev;
> > +
> > + input_alloc_absinfo(dev);
> > + if (!dev->absinfo)
> > + return -ENOMEM;
> > +
> > + set_bit(setup.code, dev->absbit);
> > +
>
> If you do a second round of this patch, maybe drop that newline?
>
> > + dev->absinfo[setup.code] = setup.absinfo;
> > +
> > + /*
> > + * We restore the state to UIST_NEW_DEVICE because the user has to call
> > + * UI_DEV_SETUP in the last place before UI_DEV_CREATE to check the
> > + * validity of the absbits.
> > + */
> > + udev->state = UIST_NEW_DEVICE;
> > + return 0;
> > +}
> > +
> > +/* legacy setup via write() */
> > +static int uinput_setup_device_legacy(struct uinput_device *udev,
> > + const char __user *buffer, size_t count)
> > {
> > struct uinput_user_dev *user_dev;
> > struct input_dev *dev;
> > @@ -474,7 +542,7 @@ static ssize_t uinput_write(struct file *file, const char __user *buffer,
> >
> > retval = udev->state == UIST_CREATED ?
> > uinput_inject_events(udev, buffer, count) :
> > - uinput_setup_device(udev, buffer, count);
> > + uinput_setup_device_legacy(udev, buffer, count);
> >
> > mutex_unlock(&udev->mutex);
> >
> > @@ -735,6 +803,12 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > uinput_destroy_device(udev);
> > goto out;
> >
> > + case UI_DEV_SETUP:
> > + retval = uinput_dev_setup(udev, p);
> > + goto out;
> > +
> > + /* UI_ABS_SETUP is handled in the variable size ioctls */
> > +
> > case UI_SET_EVBIT:
> > retval = uinput_set_bit(arg, evbit, EV_MAX);
> > goto out;
> > @@ -879,6 +953,9 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > name = dev_name(&udev->dev->dev);
> > retval = uinput_str_to_user(p, name, size);
> > goto out;
> > + case UI_ABS_SETUP & ~IOCSIZE_MASK:
> > + retval = uinput_abs_setup(udev, p, size);
> > + goto out;
> > }
> >
> > retval = -EINVAL;
> > diff --git a/include/linux/uinput.h b/include/linux/uinput.h
> > index 0994c0d..75de43d 100644
> > --- a/include/linux/uinput.h
> > +++ b/include/linux/uinput.h
> > @@ -20,6 +20,11 @@
> > * Author: Aristeu Sergio Rozanski Filho <aris@...hedrallabs.org>
> > *
> > * Changes/Revisions:
> > + * 0.5 08/13/2015 (David Herrmann <dh.herrmann@...il.com> &
> > + * Benjamin Tissoires <benjamin.tissoires@...hat.com>)
> > + * - add UI_DEV_SETUP ioctl
> > + * - add UI_ABS_SETUP ioctl
> > + * - add UI_GET_VERSION ioctl
> > * 0.4 01/09/2014 (Benjamin Tissoires <benjamin.tissoires@...hat.com>)
> > * - add UI_GET_SYSNAME ioctl
> > * 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > index 013c9d8..ef6c9f5 100644
> > --- a/include/uapi/linux/uinput.h
> > +++ b/include/uapi/linux/uinput.h
> > @@ -20,6 +20,11 @@
> > * Author: Aristeu Sergio Rozanski Filho <aris@...hedrallabs.org>
> > *
> > * Changes/Revisions:
> > + * 0.5 08/13/2015 (David Herrmann <dh.herrmann@...il.com> &
> > + * Benjamin Tissoires <benjamin.tissoires@...hat.com>)
> > + * - add UI_DEV_SETUP ioctl
> > + * - add UI_ABS_SETUP ioctl
> > + * - add UI_GET_VERSION ioctl
> > * 0.4 01/09/2014 (Benjamin Tissoires <benjamin.tissoires@...hat.com>)
> > * - add UI_GET_SYSNAME ioctl
> > * 0.3 24/05/2006 (Anssi Hannula <anssi.hannulagmail.com>)
> > @@ -37,8 +42,8 @@
> > #include <linux/types.h>
> > #include <linux/input.h>
> >
> > -#define UINPUT_VERSION 4
> > -
> > +#define UINPUT_VERSION 5
> > +#define UINPUT_MAX_NAME_SIZE 80
> >
> > struct uinput_ff_upload {
> > __u32 request_id;
> > @@ -58,6 +63,79 @@ struct uinput_ff_erase {
> > #define UI_DEV_CREATE _IO(UINPUT_IOCTL_BASE, 1)
> > #define UI_DEV_DESTROY _IO(UINPUT_IOCTL_BASE, 2)
> >
> > +struct uinput_setup {
> > + struct input_id id;
> > + char name[UINPUT_MAX_NAME_SIZE];
> > + __u32 ff_effects_max;
> > +};
> > +
> > +/**
> > + * UI_DEV_SETUP - Set device parameters for setup
> > + *
> > + * This ioctl sets parameters for the input-device to be created. It must be
> > + * issued *before* calling UI_DEV_CREATE or it will fail. This ioctl supersedes
> > + * the old "struct uinput_user_dev" method, which wrote this data via write().
> > + * To actually set the absolute axes, you also need to call the ioctl
> > + * UI_ABS_SETUP *before* calling this ioctl.
> > + *
> > + * This ioctl takes a "struct uinput_setup" object as argument. The fields of
> > + * this object are as follows:
> > + * id: See the description of "struct input_id". This field is
> > + * copied unchanged into the new device.
> > + * name: This is used unchanged as name for the new device.
> > + * ff_effects_max: This limits the maximum numbers of force-feedback effects.
> > + * See below for a description of FF with uinput.
> > + *
> > + * This ioctl can be called multiple times and will overwrite previous values.
> > + * If this ioctl fails with -EINVAL, you're recommended to use the old
> > + * "uinput_user_dev" method via write() as fallback, in case you run on an old
> > + * kernel that does not support this ioctl.
> > + *
> > + * This ioctl may fail with -EINVAL if it is not supported or if you passed
> > + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
> > + * passed uinput_setup object cannot be read/written.
> > + * If this call fails, partial data may have already been applied to the
> > + * internal device.
> > + */
> > +#define UI_DEV_SETUP _IOW(UINPUT_IOCTL_BASE, 3, struct uinput_setup)
> > +
> > +struct uinput_abs_setup {
> > + __u16 code; /* axis code */
> > + /* __u16 filler; */
> > + struct input_absinfo absinfo;
> > +};
> > +
> > +/**
> > + * UI_ABS_SETUP - Set absolute axis information for the device to setup
> > + *
> > + * This ioctl sets one absolute axis information for the input-device to be
> > + * created. It must be issued *before* calling UI_DEV_SETUP and UI_DEV_CREATE
> > + * for every absolute axis the device exports.
> > + * This ioctl supersedes the old "struct uinput_user_dev" method, which wrote
> > + * part of this data and the content of UI_DEV_SETUP via write().
> > + *
> > + * This ioctl takes a "struct uinput_abs_setup" object as argument. The fields
> > + * of this object are as follows:
> > + * code: The corresponding input code associated with this axis
> > + * (ABS_X, ABS_Y, etc...)
> > + * absinfo: See "struct input_absinfo" for a description of this field.
> > + * This field is copied unchanged into the kernel for the
> > + * specified axis. If the axis is not enabled via
> > + * UI_SET_ABSBIT, this ioctl will enable it.
> > + *
> > + * This ioctl can be called multiple times and will overwrite previous values.
> > + * If this ioctl fails with -EINVAL, you're recommended to use the old
> > + * "uinput_user_dev" method via write() as fallback, in case you run on an old
> > + * kernel that does not support this ioctl.
> > + *
> > + * This ioctl may fail with -EINVAL if it is not supported or if you passed
> > + * incorrect values, -ENOMEM if the kernel runs out of memory or -EFAULT if the
> > + * passed uinput_setup object cannot be read/written.
> > + * If this call fails, partial data may have already been applied to the
> > + * internal device.
> > + */
> > +#define UI_ABS_SETUP _IOW(UINPUT_IOCTL_BASE, 4, struct uinput_abs_setup)
> > +
> > #define UI_SET_EVBIT _IOW(UINPUT_IOCTL_BASE, 100, int)
> > #define UI_SET_KEYBIT _IOW(UINPUT_IOCTL_BASE, 101, int)
> > #define UI_SET_RELBIT _IOW(UINPUT_IOCTL_BASE, 102, int)
> > @@ -144,7 +222,6 @@ struct uinput_ff_erase {
> > #define UI_FF_UPLOAD 1
> > #define UI_FF_ERASE 2
> >
> > -#define UINPUT_MAX_NAME_SIZE 80
> > struct uinput_user_dev {
> > char name[UINPUT_MAX_NAME_SIZE];
> > struct input_id id;
>
> Looks good to me! Thanks for picking it up! Feel free to append:
>
> Reviewed-by: David Herrmann <dh.herrmann@...il.com>
Thanks for your comments David. I'll send a new version right away with
your suggestions.
Cheers,
Benjamin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists