[PATCH] Fix NULL handler for compat_ioctl

[PATCH] Fix NULL handler for compat_ioctl

[Ben Collins]

Pretty serious ommision, but I'm guessing there's not too many users of
the {un,}register_ioctl32_conversion calls. Linux1394 just happens to
use it quite extensively, so this showed up on my radar by coincidence.

You are supposed to be able to pass a NULL handler to
register_ioctl32_conversion to signify a compatible translation, IOW,
use the 64-bit ioctl handler. Without this patch, we would instead jump
to a NULL address.

Applies to current 2.5.68-bk (see Larry, someone is using bk-cvs for
something good :).


diff -u -u -r1.8 compat.c
--- linux/fs/compat.c	30 Apr 2003 16:17:21 -0000	1.8
+++ linux/fs/compat.c	1 May 2003 01:45:46 -0000
@@ -229,7 +229,10 @@
 	
 	t->next = NULL;
 	t->cmd = cmd;
-	t->handler = handler; 
+	if (!handler)
+		t->handler = (void *)sys_ioctl;
+	else
+		t->handler = handler; 
 	ioctl32_insert_translation(t);
 
 	unlock_kernel();

Re: [PATCH] Fix NULL handler for compat_ioctl

[Andrew Morton]

Ben Collins <bcollins@debian.org> wrote:
>
> -	t->handler = handler; 
> +	if (!handler)
> +		t->handler = (void *)sys_ioctl;
> +	else
> +		t->handler = handler; 

Is that safe?

- sys_ioctl takes three args, but this vector is going to be called with
  four.  That's making assumptions about arg passing conventions which may
  not be true.

- sys_ioctl() is asmlinkage, but the caller of this vector doesn't know
  that.  Arguments may get put in the wrong place.

Is a little wrapper function needed?

Re: [PATCH] Fix NULL handler for compat_ioctl

[Ben Collins]

On Wed, Apr 30, 2003 at 07:41:24PM -0700, Andrew Morton wrote:
> Ben Collins <bcollins@debian.org> wrote:
> >
> > -	t->handler = handler; 
> > +	if (!handler)
> > +		t->handler = (void *)sys_ioctl;
> > +	else
> > +		t->handler = handler; 
> 
> Is that safe?
> 
> - sys_ioctl takes three args, but this vector is going to be called with
>   four.  That's making assumptions about arg passing conventions which may
>   not be true.
> 
> - sys_ioctl() is asmlinkage, but the caller of this vector doesn't know
>   that.  Arguments may get put in the wrong place.

Out of all the register_ioctl32_conversion functions that were
consolidated with this patch, only s390 and x86_64 did not already use
this same convention. linux/ioctl32.h already documents (and has always
been that way) this feature.

Maybe you feel better about this patch instead? Moved the logic directly
to compat_sys_ioctl. Also note that sys_ioctl is prototyped in
include/linux/ioctl32.h.


Index: fs/compat.c
===================================================================
RCS file: /home/scm/linux-2.5/fs/compat.c,v
retrieving revision 1.8
diff -u -u -r1.8 compat.c
--- linux/fs/compat.c	30 Apr 2003 16:17:21 -0000	1.8
+++ linux/fs/compat.c	1 May 2003 03:18:46 -0000
@@ -300,7 +300,6 @@
 {
 	struct file * filp;
 	int error = -EBADF;
-	int (*handler)(unsigned int, unsigned int, unsigned long, struct file * filp);
 	struct ioctl_trans *t;
 
 	filp = fget(fd);
@@ -317,8 +316,10 @@
 	while (t && t->cmd != cmd)
 		t = (struct ioctl_trans *)t->next;
 	if (t) {
-		handler = t->handler;
-		error = handler(fd, cmd, arg, filp);
+		if (t->handler)
+			error = t->handler(fd, cmd, arg, filp);
+		else
+			error = sys_ioctl(fd, cmd, arg);
 	} else if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
 		error = siocdevprivate_ioctl(fd, cmd, arg);
 	} else {

Re: [PATCH] Fix NULL handler for compat_ioctl

[Andrew Morton]

Ben Collins <bcollins@debian.org> wrote:
>
> > - sys_ioctl takes three args, but this vector is going to be called with
> >   four.  That's making assumptions about arg passing conventions which may
> >   not be true.
> > 
> > - sys_ioctl() is asmlinkage, but the caller of this vector doesn't know
> >   that.  Arguments may get put in the wrong place.
> 
> Out of all the register_ioctl32_conversion functions that were
> consolidated with this patch, only s390 and x86_64 did not already use
> this same convention. linux/ioctl32.h already documents (and has always
> been that way) this feature.

Yup. I'm not surprised that it works.  But ick.

> Maybe you feel better about this patch instead?

Infinitely ;)