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

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:

quoted

+
+	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.

quoted

+	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.

-- 
Dmitry