Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl | linux-input

Re: [PATCH v3] Input: uinput - add new UINPUT_DEV_SETUP and UI_ABS_SETUP ioctl

From: David Herrmann <hidden>
Date: 2015-10-25 09:39:14
Also in: lkml
Subsystem: input (keyboard, mouse, joystick, touchscreen) drivers, the rest · Maintainers: Dmitry Torokhov, Linus Torvalds

Hi

On Sun, Oct 25, 2015 at 12:53 AM, Dmitry Torokhov
[off-list ref] wrote:

Hi Benjamin,

On Tue, Aug 25, 2015 at 11:12:59AM -0400, Benjamin Tissoires wrote:

quoted

+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;
+
+     if (size > sizeof(setup))
+             return -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);
+     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.
+      */

Do we have to be this strict? It seems to me that with the new IOCTLs
you naturally want to use the new ioctl to setup the device, then adjust
various axes and bits and then validate everything.

Indeed, we now force the order to be abs-setup first, then
device-setup as last step. Appended is a follow-up patch to cleanup
ABS handling in uinput. It is untested. Benjamin, care to give it a
spin?

Thanks
David

----

From 2568f83bcc5c4b8aeb149be2c5fc5a743812f0fe Mon Sep 17 00:00:00 2001

From: David Herrmann <redacted>
Date: Sun, 25 Oct 2015 10:34:13 +0100
Subject: [PATCH] Input: uinput - rework ABS validation

Rework the uinput ABS validation to check passed absinfo data
immediately, but do ABS initialization as last step in UI_DEV_CREATE. The
behavior observed by user-space is not changed, as ABS initialization was
never checked for errors.

With this in place, the order of device-initialization and
abs-configuration is no longer fixed. User-space can initialize the
device and afterwards set absinfo just fine.

Signed-off-by: David Herrmann <redacted>
---
 drivers/input/misc/uinput.c | 89 +++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index baac903..1d93037 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c

@@ -256,13 +256,22 @@ static void uinput_destroy_device(...)
 static int uinput_create_device(struct uinput_device *udev)
 {
  struct input_dev *dev = udev->dev;
- int error;
+ int error, nslot;

  if (udev->state != UIST_SETUP_COMPLETE) {
  printk(KERN_DEBUG "%s: write device info first\n", UINPUT_NAME);
  return -EINVAL;
  }

+ if (test_bit(ABS_MT_SLOT, dev->absbit)) {
+ nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
+ error = input_mt_init_slots(dev, nslot, 0);
+ if (error)
+ goto fail1;
+ } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
+ input_set_events_per_packet(dev, 60);
+ }
+
  if (udev->ff_effects_max) {
  error = input_ff_create(dev, udev->ff_effects_max);
  if (error)

@@ -308,10 +317,35 @@ static int uinput_open(...)
  return 0;
 }

+static int uinput_validate_absinfo(struct input_dev *dev, unsigned int code,
+   const struct input_absinfo *abs)
+{
+ int min, max;
+
+ min = abs->minimum;
+ max = abs->maximum;
+
+ if ((min != 0 || max != 0) && max <= min) {
+ printk(KERN_DEBUG
+       "%s: invalid abs[%02x] min:%d max:%d\n",
+       UINPUT_NAME, code, min, max);
+ return -EINVAL;
+ }
+
+ if (abs->flat > max - min) {
+ printk(KERN_DEBUG
+       "%s: abs_flat #%02x out of range: %d (min:%d/max:%d)\n",
+       UINPUT_NAME, code, abs->flat, min, max);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
 static int uinput_validate_absbits(struct input_dev *dev)
 {
  unsigned int cnt;
- int nslot;
+ int error;

  if (!test_bit(EV_ABS, dev->evbit))
  return 0;

@@ -321,38 +355,12 @@ static int uinput_validate_absbits(struct input_dev *dev)
  */

  for_each_set_bit(cnt, dev->absbit, ABS_CNT) {
- int min, max;
-
- min = input_abs_get_min(dev, cnt);
- max = input_abs_get_max(dev, cnt);
-
- if ((min != 0 || max != 0) && max <= min) {
- printk(KERN_DEBUG
- "%s: invalid abs[%02x] min:%d max:%d\n",
- UINPUT_NAME, cnt,
- input_abs_get_min(dev, cnt),
- input_abs_get_max(dev, cnt));
+ if (!dev->absinfo)
  return -EINVAL;
- }
-
- if (input_abs_get_flat(dev, cnt) >
-    input_abs_get_max(dev, cnt) - input_abs_get_min(dev, cnt)) {
- printk(KERN_DEBUG
- "%s: abs_flat #%02x out of range: %d "
- "(min:%d/max:%d)\n",
- UINPUT_NAME, cnt,
- input_abs_get_flat(dev, cnt),
- input_abs_get_min(dev, cnt),
- input_abs_get_max(dev, cnt));
- return -EINVAL;
- }
- }

- if (test_bit(ABS_MT_SLOT, dev->absbit)) {
- nslot = input_abs_get_max(dev, ABS_MT_SLOT) + 1;
- input_mt_init_slots(dev, nslot, 0);
- } else if (test_bit(ABS_MT_POSITION_X, dev->absbit)) {
- input_set_events_per_packet(dev, 60);
+ error = uinput_validate_absinfo(dev, cnt, &dev->absinfo[cnt]);
+ if (error)
+ return error;
  }

  return 0;

@@ -375,7 +383,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
 {
  struct uinput_setup setup;
  struct input_dev *dev;
- int retval;

  if (udev->state == UIST_CREATED)
  return -EINVAL;

@@ -393,10 +400,6 @@ static int uinput_dev_setup(struct uinput_device *udev,
  if (!dev->name)
  return -ENOMEM;

- retval = uinput_validate_absbits(dev);
- if (retval < 0)
- return retval;
-
  udev->state = UIST_SETUP_COMPLETE;
  return 0;
 }

@@ -406,6 +409,7 @@ static int uinput_abs_setup(struct uinput_device *udev,
 {
  struct uinput_abs_setup setup = {};
  struct input_dev *dev;
+ int error;

  if (size > sizeof(setup))
  return -E2BIG;

@@ -418,19 +422,16 @@ static int uinput_abs_setup(struct uinput_device *udev,

  dev = udev->dev;

+ error = uinput_validate_absinfo(dev, setup.code, &setup.absinfo);
+ if (error)
+ return error;
+
  input_alloc_absinfo(dev);
  if (!dev->absinfo)
  return -ENOMEM;

  set_bit(setup.code, dev->absbit);
  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;
 }

-- 
2.6.1

Attachments

0001-Input-uinput-rework-ABS-validation.patch [text/x-patch] 4943 bytes · preview

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help