Bug 191811
| Summary: | uinput crashes with misconfigured ABS axes | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Rodrigo Rivas Costa (rodrigorivascosta) |
| Component: | Input Devices | Assignee: | Dmitry Torokhov (dmitry.torokhov) |
| Status: | NEW --- | ||
| Severity: | normal | CC: | dh.herrmann, dmitry.torokhov |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 4.8 | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
|
Description
Rodrigo Rivas Costa
2017-01-03 12:12:56 UTC
It appears you are mixing the old and new way of creating uinput device. Do you mind listing all operations that you do in order to set up your uinput device? Sorry, my bad, I thought that it was easier to reproduce.
I managed to crash my system with this code:
#include <fcntl.h>
#include <unistd.h>
#include <string.h>
#include <sys/ioctl.h>
#include <linux/uinput.h>
int main()
{
int ui = open("/dev/uinput", O_RDWR | O_CLOEXEC);
struct uinput_setup us = {0};
us.id.bustype = BUS_VIRTUAL;
us.id.version = 1;
strcpy(us.name, "Test");
ioctl(ui, UI_DEV_SETUP, &us);
ioctl(ui, UI_SET_PHYS, "Test");
ioctl(ui, UI_SET_EVBIT, EV_ABS);
ioctl(ui, UI_SET_ABSBIT, ABS_X);
ioctl(ui, UI_SET_ABSBIT, ABS_Y);
ioctl(ui, UI_DEV_CREATE, 0);
struct input_event e = {0};
e.type = EV_ABS;
e.code = ABS_Y;
e.value = 100;
write(ui, &e, sizeof(e));
}
I thought that the "new way" was to use UI_DEV_SETUP instead of write() the "struct uinput_setup", isn't it? I'm trying to use the new style, but I forgot to do the UI_ABS_SETUP. Curiously, if I write() a ABS_X event, it will not crash.
Is UI_ABS_SETUP really mandatory? If UI_ABS_SETUP does an implicit UI_SET_ABSBIT, and a UI_SET_ABSBIT cannot be used without a UI_ABS_SETUP, then UI_SET_ABSBIT is useless! Or is UI_SET_ABSBIT part of the "old way"? If that is the case it should be marked as such in uinput.h.
Yeah, you mix the new ioctls with the old. You cannot use UI_DEV_SETUP with UI_SET_ABSBIT. You need to specify the ABS parameters, and this is either done via UI_ABS_SETUP or the old write() method.
So just replace your UI_SET_ABSBIT calls with UI_ABS_SETUP and it should work fine (but pass `struct uinput_abs_setup` as argument). Btw., the libevdev project contains a lot of helpers to deal with uinput as well as evdev. Even if you don't end up using it, always worth a look to see how things were designed to be used.
We still need to fix the NULL-deref, though. How about this:
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 92595b98e7ed..dd765ab1a332 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -832,7 +832,11 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
goto out;
case UI_SET_ABSBIT:
- retval = uinput_set_bit(arg, absbit, ABS_MAX);
+ input_alloc_absinfo(udev->dev);
+ if (!udev->dev->absinfo)
+ retval = -ENOMEM;
+ else
+ retval = uinput_set_bit(arg, absbit, ABS_MAX);
goto out;
case UI_SET_MSCBIT:
Yeah, my code was badly written and easy to fix. Thanks for the libevdev point (you cannot have too many references). Until now I was using the kernel source as reference, because the documentation for uinput is somewhat missing... Your patch will certainly fix the NULL reference. I would have implemented it by calling input_set_capability(), just in case in the future other invariants arise: case UI_SET_RELBIT: - retval = uinput_set_bit(arg, relbit, REL_MAX); + input_set_capability(udev->deev, EV_REL, arg); goto out; case UI_SET_ABSBIT: - retval = uinput_set_bit(arg, absbit, ABS_MAX); + input_set_capability(udev->deev, EV_ABS, arg); goto out; And so on for all the other bits. But then you'll miss the retval, so you'll need a few more changes, and I don't know if it is worth it... |