Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt <steve@sk2.org>]

On Tue, 11 Aug 2009 00:26:53 -0700, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Tue, Aug 11, 2009 at 08:20:32AM +0200, Stephen Kitt wrote:
> > Here goes! I'm not too familiar with kernel memory handling, I'll send the
> > fixes for the FIXMEs later on as a separate patch once I've figured things
> > out; I'm thinking along these lines:
> > * vmalloc() a buffer
> > * return -ENOMEM if no memory is available
> > * copy_from_user the data into the temporary buffer
> > * validate the data
> > * on error vfree() the buffer and return -EINVAL
> > * copy the data from the temporary buffer to the destination array
> > * vfree() the buffer
> > 
> 
> Yep, exactly, except that don't bother with vmalloc, kmalloc will do
> nicely since the amout of memory needed is relatively small.

The following does just that (it applies on top of the previous patch).

The change to JSIOCGBTNMAP and JSIOCGAXMAP revealed a bug in the Debian and
Ubuntu versions of jscal (they reacted to any non-zero return value of
ioctl() as an error, not just negative values), but it will be fixed by the
time this makes it into the kernel... I checked any other code I could find
using those two ioctl requests, and I didn't find any other instance of the
bug.

Regards,

Stephen

 Input: validate axis and button maps before overwriting the driver's version

 Up to now axis and button map validation was done after the user-supplied
 values were copied over the driver's map. This patch copies the
 user-supplied values into temporary buffers and validated them before
 overwriting the driver's permanent maps.

 Signed-off-by: Stephen Kitt <steve@sk2.org>

 ---

 drivers/input/joydev.c |   52 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 15 deletions(-)


diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index c4a5e7b..ceaeb7b 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -459,6 +459,8 @@ static int joydev_ioctl_common(struct joydev *joydev,
 	size_t len;
 	int i, j;
 	const char *name;
+	__u8 *tmpabspam;
+	__u16 *tmpkeypam;
 
 	/* Process fixed-sized commands. */
 	switch (cmd) {
@@ -511,16 +513,26 @@ static int joydev_ioctl_common(struct joydev *joydev,
 
 	case (JSIOCSAXMAP & ~IOCSIZE_MASK):
 		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->abspam));
-		/*
-		 * FIXME: we should not copy into our axis map before
-		 * validating the data.
-		 */
-		if (copy_from_user(joydev->abspam, argp, len))
-			return -EFAULT;
 
+		/* Validate the map. */
+		tmpabspam = kmalloc(len, GFP_KERNEL);
+		if (!tmpabspam)
+			return -ENOMEM;
+		if (copy_from_user(tmpabspam, argp, len)) {
+			kfree(tmpabspam);
+			return -EFAULT;
+		}
 		for (i = 0; i < joydev->nabs; i++) {
-			if (joydev->abspam[i] > ABS_MAX)
+			if (tmpabspam[i] > ABS_MAX) {
+				kfree(tmpabspam);
 				return -EINVAL;
+			}
+		}
+
+		memcpy(joydev->abspam, tmpabspam, len);
+		kfree(tmpabspam);
+
+		for (i = 0; i < joydev->nabs; i++) {
 			joydev->absmap[joydev->abspam[i]] = i;
 		}
 		return 0;
@@ -531,17 +543,27 @@ static int joydev_ioctl_common(struct joydev *joydev,
 
 	case (JSIOCSBTNMAP & ~IOCSIZE_MASK):
 		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
-		/*
-		 * FIXME: we should not copy into our keymap before
-		 * validating the data.
-		 */
-		if (copy_from_user(joydev->keypam, argp, len))
-			return -EFAULT;
 
+		/* Validate the map. */
+		tmpkeypam = kmalloc(len, GFP_KERNEL);
+		if (!tmpkeypam)
+			return -ENOMEM;
+		if (copy_from_user(tmpkeypam, argp, len)) {
+			kfree(tmpkeypam);
+			return -EFAULT;
+		}
 		for (i = 0; i < joydev->nkey; i++) {
-			if (joydev->keypam[i] > KEY_MAX ||
-			    joydev->keypam[i] < BTN_MISC)
+			if (tmpkeypam[i] > KEY_MAX ||
+			    tmpkeypam[i] < BTN_MISC) {
+				kfree(tmpkeypam);
 				return -EINVAL;
+			}
+		}
+
+		memcpy(joydev->keypam, tmpkeypam, len);
+		kfree(tmpkeypam);
+
+		for (i = 0; i < joydev->nkey; i++) {
 			joydev->keymap[joydev->keypam[i] - BTN_MISC] = i;
 		}