Restoring joydev BTNMAP ioctl compatibility

Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

Hi,

The KEY_MAX change in 2.6.28 changed the definitions of the button map
joydev ioctls (JSIOCSBTNMAP and JSIOCGBTNMAP). Thus software built using
pre-2.6.28 headers fails when retrieving or setting the button map on 2.6.28
and greater kernels.

The attached patch reintroduced the old ioctl definitions to restore
compatibility. It only copies as much information as was supported in
previous versions, but since this still allows for devices with 256 buttons I
doubt there's much chance of losing information, hence the lack of a printk()
warning.

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

 joydev.c |   23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..764700e 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -35,6 +35,11 @@ MODULE_LICENSE("GPL");
 #define JOYDEV_MINORS		16
 #define JOYDEV_BUFFER_SIZE	64
 
+/* Support for old ioctls using the old value of KEY_MAX. */
+#define OLD_KEY_MAX		0x1ff
+#define OLD_JSIOCSBTNMAP	_IOW('j', 0x33, __u16[OLD_KEY_MAX - BTN_MISC + 1])
+#define OLD_JSIOCGBTNMAP	_IOR('j', 0x34, __u16[OLD_KEY_MAX - BTN_MISC + 1])
+
 struct joydev {
 	int exist;
 	int open;
@@ -529,10 +534,28 @@ static int joydev_ioctl_common(struct joydev *joydev,
 
 		return 0;
 
+	case OLD_JSIOCSBTNMAP:
+		if (copy_from_user(joydev->keypam, argp,
+				   sizeof(__u16) * (OLD_KEY_MAX - BTN_MISC + 1)))
+			return -EFAULT;
+
+		for (i = 0; i < joydev->nkey && i < OLD_KEY_MAX - BTN_MISC + 1; i++) {
+			if (joydev->keypam[i] > KEY_MAX ||
+			    joydev->keypam[i] < BTN_MISC)
+				return -EINVAL;
+			joydev->keymap[joydev->keypam[i] - BTN_MISC] = i;
+		}
+
+		return 0;
+
 	case JSIOCGBTNMAP:
 		return copy_to_user(argp, joydev->keypam,
 			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
 
+	case OLD_JSIOCGBTNMAP:
+		return copy_to_user(argp, joydev->keypam,
+				    sizeof(__u16) * (OLD_KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+
 	default:
 		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
 			int len;

Re: Restoring joydev BTNMAP ioctl compatibility

[Dmitry Torokhov]

Hi Stephen,

On Mon, Aug 10, 2009 at 08:52:42AM +0200, Stephen Kitt wrote:
> Hi,
> 
> The KEY_MAX change in 2.6.28 changed the definitions of the button map
> joydev ioctls (JSIOCSBTNMAP and JSIOCGBTNMAP). Thus software built using
> pre-2.6.28 headers fails when retrieving or setting the button map on 2.6.28
> and greater kernels.
> 

Darn, this is bad...

> The attached patch reintroduced the old ioctl definitions to restore
> compatibility. It only copies as much information as was supported in
> previous versions, but since this still allows for devices with 256 buttons I
> doubt there's much chance of losing information, hence the lack of a printk()
> warning.
> 

However adding the "old" ioctls is not the right solution, we should be
just respecing the size encoded in the ioctl and limit the amount of
data sent/received if it is less than our internal buffers. Could you
please try the patch below?

Thanks!

-- 
Dmitry

Input: joydev - fix JSIOCSBTNMAP and JSIOCGBTNMAP ioctls

The KEY_MAX change in 2.6.28 changed the amount of data transmitted by
JSIOCSBTNMAP and JSIOCGBTNMAP joydev ioctls; unfortunately joydev driver
did not pay attention to the length of the request and thus caused
software that was compiled with old definitions fail. Change the
joydev driver to respect size parameter encoded in ioctl.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/joydev.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)


diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..d15aaae 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -456,6 +456,7 @@ static int joydev_ioctl_common(struct joydev *joydev,
 				unsigned int cmd, void __user *argp)
 {
 	struct input_dev *dev = joydev->handle.dev;
+	size_t len;
 	int i, j;
 
 	switch (cmd) {
@@ -516,8 +517,12 @@ static int joydev_ioctl_common(struct joydev *joydev,
 			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
 
 	case JSIOCSBTNMAP:
-		if (copy_from_user(joydev->keypam, argp,
-				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
+		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;
 
 		for (i = 0; i < joydev->nkey; i++) {
@@ -530,12 +535,11 @@ static int joydev_ioctl_common(struct joydev *joydev,
 		return 0;
 
 	case JSIOCGBTNMAP:
-		return copy_to_user(argp, joydev->keypam,
-			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
+		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : 0;
 
 	default:
 		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
-			int len;
 			const char *name = dev->name;
 
 			if (!name)

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

On Mon, 10 Aug 2009 01:14:12 -0700, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Aug 10, 2009 at 08:52:42AM +0200, Stephen Kitt wrote:
> > The attached patch reintroduced the old ioctl definitions to restore
> > compatibility. It only copies as much information as was supported in
> > previous versions, but since this still allows for devices with 256
> > buttons I doubt there's much chance of losing information, hence the lack
> > of a printk() warning.
> 
> However adding the "old" ioctls is not the right solution, we should be
> just respecing the size encoded in the ioctl and limit the amount of
> data sent/received if it is less than our internal buffers. Could you
> please try the patch below?

The more generic approach in your patch is indeed better, but it doesn't
solve the original problem. Because the ioctl command code encodes the
expected argument size, the case statements still only match the new ioctl
codes, and the old ones used before 2.6.28 aren't matched and result in
-EINVAL. For example, jstest built with the 2.6.27 headers will call
ioctl(..., 2181065268, ...) (2181065268 is the value corresponding to
JSIOCGBTNMAP before 2.6.28), but 2.6.28 kernels and later will be looking for
2214619700 which is the value corresponding to JSIOCGBTNMAP from 2.6.28
onwards.

The following merges both and handles all cases...

A cleaner approach with a single case statement would involve switching on
_IOC_NR(cmd) rather than cmd directly, and handling the length explicitly.
That would make the ioctl ABI backwards-compatible when KEY_MAX increases
without explicit handling! I can rewrite the patch in this way if you wish.

Regards,

Stephen

 Input: joydev - handle old values of JSIOCSBTNMAP and JSIOCGBTNMAP ioctls

 The KEY_MAX change in 2.6.28 changed the values of the JSIOCSBTNMAP and
 JSIOCGBTNMAP constants; software compiled with the old values no longer
 works with kernels following 2.6.28, because the ioctl switch statement no
 longer matches the values given by the software. This patch adds explicit
 handling of the old ioctl values.

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

 ---

 drivers/input/joydev.c |   21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..b26de29 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -35,6 +35,11 @@ MODULE_LICENSE("GPL");
 #define JOYDEV_MINORS		16
 #define JOYDEV_BUFFER_SIZE	64
 
+/* Support for old ioctls using the old value of KEY_MAX. */
+#define OLD_KEY_MAX		0x1ff
+#define OLD_JSIOCSBTNMAP	_IOW('j', 0x33, __u16[OLD_KEY_MAX - BTN_MISC + 1])
+#define OLD_JSIOCGBTNMAP	_IOR('j', 0x34, __u16[OLD_KEY_MAX - BTN_MISC + 1])
+
 struct joydev {
 	int exist;
 	int open;
@@ -456,6 +461,7 @@ static int joydev_ioctl_common(struct joydev *joydev,
 				unsigned int cmd, void __user *argp)
 {
 	struct input_dev *dev = joydev->handle.dev;
+	size_t len;
 	int i, j;
 
 	switch (cmd) {
@@ -516,8 +522,13 @@ static int joydev_ioctl_common(struct joydev *joydev,
 			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
 
 	case JSIOCSBTNMAP:
-		if (copy_from_user(joydev->keypam, argp,
-				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
+	case OLD_JSIOCSBTNMAP:
+		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;
 
 		for (i = 0; i < joydev->nkey; i++) {
@@ -530,12 +541,12 @@ static int joydev_ioctl_common(struct joydev *joydev,
 		return 0;
 
 	case JSIOCGBTNMAP:
-		return copy_to_user(argp, joydev->keypam,
-			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+	case OLD_JSIOCGBTNMAP:
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
+		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : 0;
 
 	default:
 		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
-			int len;
 			const char *name = dev->name;
 
 			if (!name)

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

On Mon, 10 Aug 2009 13:29:05 +0200, Stephen Kitt <steve@sk2.org> wrote:
> A cleaner approach with a single case statement would involve switching on
> _IOC_NR(cmd) rather than cmd directly, and handling the length explicitly.
> That would make the ioctl ABI backwards-compatible when KEY_MAX increases
> without explicit handling! I can rewrite the patch in this way if you wish.

Something like the following, which also makes explicit the handling of
JSIOCGNAME!

Regards,

Stephen

 Input: joydev - handle old values of JSIOCSBTNMAP and JSIOCGBTNMAP ioctls

 The KEY_MAX change in 2.6.28 changed the values of the JSIOCSBTNMAP and
 JSIOCGBTNMAP constants; software compiled with the old values no longer
 works with kernels following 2.6.28, because the ioctl switch statement no
 longer matches the values given by the software. This patch handles these
 ioctls independently of the length of data specified.

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

 ---

 drivers/input/joydev.c |   50 +++++++++++++++++++++++++++++--------------------
 1 file changed, 30 insertions(+), 20 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..8bd9569 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -456,8 +456,10 @@ static int joydev_ioctl_common(struct joydev *joydev,
 				unsigned int cmd, void __user *argp)
 {
 	struct input_dev *dev = joydev->handle.dev;
+	size_t len;
 	int i, j;
 
+	/* Process fixed-sized commands. */
 	switch (cmd) {
 
 	case JS_SET_CAL:
@@ -514,10 +516,20 @@ static int joydev_ioctl_common(struct joydev *joydev,
 	case JSIOCGAXMAP:
 		return copy_to_user(argp, joydev->abspam,
 			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
+	}
 
-	case JSIOCSBTNMAP:
-		if (copy_from_user(joydev->keypam, argp,
-				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
+	/* Process variable-sized commands (the button map commands are
+	   considered variable-sized for compatibility reasons, and to avoid
+	   problems with future changes to KEY_MAX). */
+	switch (cmd & ~IOCSIZE_MASK) {
+
+	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;
 
 		for (i = 0; i < joydev->nkey; i++) {
@@ -529,25 +541,23 @@ static int joydev_ioctl_common(struct joydev *joydev,
 
 		return 0;
 
-	case JSIOCGBTNMAP:
-		return copy_to_user(argp, joydev->keypam,
-			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+	case (JSIOCGBTNMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
+		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : 0;
 
-	default:
-		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
-			int len;
-			const char *name = dev->name;
-
-			if (!name)
-				return 0;
-			len = strlen(name) + 1;
-			if (len > _IOC_SIZE(cmd))
-				len = _IOC_SIZE(cmd);
-			if (copy_to_user(argp, name, len))
-				return -EFAULT;
-			return len;
-		}
+	case JSIOCGNAME(0):
+		const char *name = dev->name;
+
+		if (!name)
+			return 0;
+		len = strlen(name) + 1;
+		if (len > _IOC_SIZE(cmd))
+			len = _IOC_SIZE(cmd);
+		if (copy_to_user(argp, name, len))
+			return -EFAULT;
+		return len;
 	}
+
 	return -EINVAL;
 }
 

Re: Restoring joydev BTNMAP ioctl compatibility

[Dmitry Torokhov]

Hi Stephen,

On Mon, Aug 10, 2009 at 09:12:45PM +0200, Stephen Kitt wrote:
> On Mon, 10 Aug 2009 13:29:05 +0200, Stephen Kitt <steve@sk2.org> wrote:
> > A cleaner approach with a single case statement would involve switching on
> > _IOC_NR(cmd) rather than cmd directly, and handling the length explicitly.
> > That would make the ioctl ABI backwards-compatible when KEY_MAX increases
> > without explicit handling! I can rewrite the patch in this way if you wish.
> 
> Something like the following, which also makes explicit the handling of
> JSIOCGNAME!
> 

Looks much better now.

> Regards,
> 
> Stephen
> 
>  Input: joydev - handle old values of JSIOCSBTNMAP and JSIOCGBTNMAP ioctls
> 
>  The KEY_MAX change in 2.6.28 changed the values of the JSIOCSBTNMAP and
>  JSIOCGBTNMAP constants; software compiled with the old values no longer
>  works with kernels following 2.6.28, because the ioctl switch statement no
>  longer matches the values given by the software. This patch handles these
>  ioctls independently of the length of data specified.
> 
>  Signed-off-by: Stephen Kitt <steve@sk2.org>
> 
>  ---
> 
>  drivers/input/joydev.c |   50 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> index 4cfd084..8bd9569 100644
> --- a/drivers/input/joydev.c
> +++ b/drivers/input/joydev.c
> @@ -456,8 +456,10 @@ static int joydev_ioctl_common(struct joydev *joydev,
>  				unsigned int cmd, void __user *argp)
>  {
>  	struct input_dev *dev = joydev->handle.dev;
> +	size_t len;
>  	int i, j;
>  
> +	/* Process fixed-sized commands. */
>  	switch (cmd) {
>  
>  	case JS_SET_CAL:
> @@ -514,10 +516,20 @@ static int joydev_ioctl_common(struct joydev *joydev,
>  	case JSIOCGAXMAP:
>  		return copy_to_user(argp, joydev->abspam,
>  			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
> +	}
>  
> -	case JSIOCSBTNMAP:
> -		if (copy_from_user(joydev->keypam, argp,
> -				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
> +	/* Process variable-sized commands (the button map commands are
> +	   considered variable-sized for compatibility reasons, and to avoid
> +	   problems with future changes to KEY_MAX). */
> +	switch (cmd & ~IOCSIZE_MASK) {
> +
> +	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;
>  
>  		for (i = 0; i < joydev->nkey; i++) {
> @@ -529,25 +541,23 @@ static int joydev_ioctl_common(struct joydev *joydev,
>  
>  		return 0;
>  
> -	case JSIOCGBTNMAP:
> -		return copy_to_user(argp, joydev->keypam,
> -			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
> +	case (JSIOCGBTNMAP & ~IOCSIZE_MASK):
> +		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
> +		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : 0;

Actually I think we need to return "len" here to let userspace know how
much data we produced. Also, could you please give the same treatment to
JSIOCGAXMAP and JSIOCSAXMAP and I will apply it.

Thanks!

-- 
Dmitry

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

Hi Dmitry,

On Mon, 10 Aug 2009 13:27:26 -0700, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Actually I think we need to return "len" here to let userspace know how
> much data we produced. Also, could you please give the same treatment to
> JSIOCGAXMAP and JSIOCSAXMAP and I will apply it.

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

Would that be OK?

Regards,

Stephen


 Input: joydev - decouple axis and button map ioctls from input constants

 The KEY_MAX change in 2.6.28 changed the values of the JSIOCSBTNMAP and
 JSIOCGBTNMAP constants; software compiled with the old values no longer
 works with kernels following 2.6.28, because the ioctl switch statement
 no longer matches the values given by the software. This patch handles
 these ioctls independently of the length of data specified, and applies the
 same treatment to JSIOCSAXMAP and JSIOCGAXMAP which currently depend on
 ABS_MAX.

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

 ---

 drivers/input/joydev.c |   70 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 27 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..c4a5e7b 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -456,8 +456,11 @@ static int joydev_ioctl_common(struct joydev *joydev,
 				unsigned int cmd, void __user *argp)
 {
 	struct input_dev *dev = joydev->handle.dev;
+	size_t len;
 	int i, j;
+	const char *name;
 
+	/* Process fixed-sized commands. */
 	switch (cmd) {
 
 	case JS_SET_CAL:
@@ -499,9 +502,20 @@ static int joydev_ioctl_common(struct joydev *joydev,
 		return copy_to_user(argp, joydev->corr,
 			sizeof(joydev->corr[0]) * joydev->nabs) ? -EFAULT : 0;
 
-	case JSIOCSAXMAP:
-		if (copy_from_user(joydev->abspam, argp,
-				   sizeof(__u8) * (ABS_MAX + 1)))
+	}
+
+	/* Process variable-sized commands (the axis and button map commands
+	   are considered variable-sized to decouple them from the values of
+	   ABS_MAX and KEY_MAX). */
+	switch (cmd & ~IOCSIZE_MASK) {
+
+	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;
 
 		for (i = 0; i < joydev->nabs; i++) {
@@ -511,13 +525,17 @@ static int joydev_ioctl_common(struct joydev *joydev,
 		}
 		return 0;
 
-	case JSIOCGAXMAP:
-		return copy_to_user(argp, joydev->abspam,
-			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
-
-	case JSIOCSBTNMAP:
-		if (copy_from_user(joydev->keypam, argp,
-				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
+	case (JSIOCGAXMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->abspam));
+		return copy_to_user(argp, joydev->abspam, len) ? -EFAULT : len;
+
+	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;
 
 		for (i = 0; i < joydev->nkey; i++) {
@@ -529,25 +547,23 @@ static int joydev_ioctl_common(struct joydev *joydev,
 
 		return 0;
 
-	case JSIOCGBTNMAP:
-		return copy_to_user(argp, joydev->keypam,
-			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+	case (JSIOCGBTNMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
+		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : len;
 
-	default:
-		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
-			int len;
-			const char *name = dev->name;
-
-			if (!name)
-				return 0;
-			len = strlen(name) + 1;
-			if (len > _IOC_SIZE(cmd))
-				len = _IOC_SIZE(cmd);
-			if (copy_to_user(argp, name, len))
-				return -EFAULT;
-			return len;
-		}
+	case JSIOCGNAME(0):
+		name = dev->name;
+
+		if (!name)
+			return 0;
+		len = strlen(name) + 1;
+		if (len > _IOC_SIZE(cmd))
+			len = _IOC_SIZE(cmd);
+		if (copy_to_user(argp, name, len))
+			return -EFAULT;
+		return len;
 	}
+
 	return -EINVAL;
 }
 

Re: Restoring joydev BTNMAP ioctl compatibility

[Dmitry Torokhov]

On Tue, Aug 11, 2009 at 08:20:32AM +0200, Stephen Kitt wrote:
> Hi Dmitry,
> 
> On Mon, 10 Aug 2009 13:27:26 -0700, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Actually I think we need to return "len" here to let userspace know how
> > much data we produced. Also, could you please give the same treatment to
> > JSIOCGAXMAP and JSIOCSAXMAP and I will apply it.
> 
> 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.

-- 
Dmitry

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

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;
 		}
 

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

Hi Dmitry,

On Tue, 11 Aug 2009 00:26:53 -0700, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Yep, exactly, except that don't bother with vmalloc, kmalloc will do
> nicely since the amout of memory needed is relatively small.

Did you have a chance to look at the previous patch I sent? In case it's
easier to review in one go, here's a single patch grouping the changes to the
ioctls themselves as well as the map validation code!

Regards,

Stephen


 Input: handle map ioctls regardless of declared length, and validate maps

 The KEY_MAX change in 2.6.28 changed the values of the JSIOCSBTNMAP and
 JSIOCGBTNMAP constants; software compiled with the old values no longer
 works with kernels following 2.6.28, because the ioctl switch statement
 no longer matches the values given by the software. This patch handles
 these ioctls independently of the length of data specified, and applies the
 same treatment to JSIOCSAXMAP and JSIOCGAXMAP which currently depend on
 ABS_MAX.

 Furthermore the user-supplied maps are validated before being copied over
 the driver's stored values; previously an invalid map could result in the
 driver's copy being partially updated.

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

 ---

 drivers/input/joydev.c |  100 +++++++++++++++++++++++++++++++++---------------
 1 files changed, 69 insertions(+), 31 deletions(-)

diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
index 4cfd084..ceaeb7b 100644
--- a/drivers/input/joydev.c
+++ b/drivers/input/joydev.c
@@ -456,8 +456,13 @@ static int joydev_ioctl_common(struct joydev *joydev,
 				unsigned int cmd, void __user *argp)
 {
 	struct input_dev *dev = joydev->handle.dev;
+	size_t len;
 	int i, j;
+	const char *name;
+	__u8 *tmpabspam;
+	__u16 *tmpkeypam;
 
+	/* Process fixed-sized commands. */
 	switch (cmd) {
 
 	case JS_SET_CAL:
@@ -499,55 +504,88 @@ static int joydev_ioctl_common(struct joydev *joydev,
 		return copy_to_user(argp, joydev->corr,
 			sizeof(joydev->corr[0]) * joydev->nabs) ? -EFAULT : 0;
 
-	case JSIOCSAXMAP:
-		if (copy_from_user(joydev->abspam, argp,
-				   sizeof(__u8) * (ABS_MAX + 1)))
-			return -EFAULT;
+	}
+
+	/* Process variable-sized commands (the axis and button map commands
+	   are considered variable-sized to decouple them from the values of
+	   ABS_MAX and KEY_MAX). */
+	switch (cmd & ~IOCSIZE_MASK) {
 
+	case (JSIOCSAXMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->abspam));
+
+		/* 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;
 
-	case JSIOCGAXMAP:
-		return copy_to_user(argp, joydev->abspam,
-			sizeof(__u8) * (ABS_MAX + 1)) ? -EFAULT : 0;
+	case (JSIOCGAXMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->abspam));
+		return copy_to_user(argp, joydev->abspam, len) ? -EFAULT : len;
 
-	case JSIOCSBTNMAP:
-		if (copy_from_user(joydev->keypam, argp,
-				   sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)))
-			return -EFAULT;
+	case (JSIOCSBTNMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
 
+		/* 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;
 		}
 
 		return 0;
 
-	case JSIOCGBTNMAP:
-		return copy_to_user(argp, joydev->keypam,
-			sizeof(__u16) * (KEY_MAX - BTN_MISC + 1)) ? -EFAULT : 0;
+	case (JSIOCGBTNMAP & ~IOCSIZE_MASK):
+		len = min_t(size_t, _IOC_SIZE(cmd), sizeof(joydev->keypam));
+		return copy_to_user(argp, joydev->keypam, len) ? -EFAULT : len;
 
-	default:
-		if ((cmd & ~IOCSIZE_MASK) == JSIOCGNAME(0)) {
-			int len;
-			const char *name = dev->name;
-
-			if (!name)
-				return 0;
-			len = strlen(name) + 1;
-			if (len > _IOC_SIZE(cmd))
-				len = _IOC_SIZE(cmd);
-			if (copy_to_user(argp, name, len))
-				return -EFAULT;
-			return len;
-		}
+	case JSIOCGNAME(0):
+		name = dev->name;
+
+		if (!name)
+			return 0;
+		len = strlen(name) + 1;
+		if (len > _IOC_SIZE(cmd))
+			len = _IOC_SIZE(cmd);
+		if (copy_to_user(argp, name, len))
+			return -EFAULT;
+		return len;
 	}
+
 	return -EINVAL;
 }
 

Re: Restoring joydev BTNMAP ioctl compatibility

[Dmitry Torokhov]

Hi Stephen,

On Mon, Aug 17, 2009 at 09:24:16PM +0200, Stephen Kitt wrote:
> Hi Dmitry,
> 
> On Tue, 11 Aug 2009 00:26:53 -0700, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > Yep, exactly, except that don't bother with vmalloc, kmalloc will do
> > nicely since the amout of memory needed is relatively small.
> 
> Did you have a chance to look at the previous patch I sent? In case it's
> easier to review in one go, here's a single patch grouping the changes to the
> ioctls themselves as well as the map validation code!
> 

Yes, the first patch will go into .31; the second one can wait till .32.

-- 
Dmitry

Re: Restoring joydev BTNMAP ioctl compatibility

[Stephen Kitt]

On Mon, 17 Aug 2009 22:25:20 -0700, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Yes, the first patch will go into .31; the second one can wait till .32.

Great, thanks!

Stephen