Fix NULL handler for compat_ioctl

PATCH In review Ben Collins 2 participants 4 messages May 1, 2003 VFS

Share & tools

Ben Collins

author

2003-05-01 01:43 UTC #0 lore raw

PATCH 1/1 Fix NULL handler for compat_ioctl

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 :).

fs/compat.c +4 -1

diff -u -u -r1.8 compat.c
@@ -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();

Andrew Morton

2003-05-01 02:28 UTC #1 lore raw

fs/compat.c line 235

Quoted 6 lines · Ben Collins

fs/compat.c +4 -1

-	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?

Ben Collins

author

2003-05-01 03:08 UTC #2 lore raw

Quoted 10 lines · Andrew Morton

Review
Quoted 6 lines · Ben Collins
(quoted diff) +4 -1
-	t->handler = handler; 
+	if (!handler)
+		t->handler = (void *)sys_ioctl;
+	else
+		t->handler = handler; 
Is that safe?

Quoted 6 lines

- 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

fs/compat.c +4 -3

diff -u -u -r1.8 compat.c
@@ -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 {

Andrew Morton

2003-05-01 03:27 UTC #3 lore raw

Quoted 5 lines · Ben Collins

Quoted 5 lines
- 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.

Quoted 7 lines

Quoted 2 lines
- 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.

Quoted 1 line

Maybe you feel better about this patch instead?

Infinitely ;)