Re: [PATCH] Fixes in 32 bit ioctl emulation code

[Andrew Morton <akpm@osdl.org>]

Raghavan <raghav@in.ibm.com> wrote:
>
> I am submitting a patch that fixes 2 race conditions in the 32 bit ioctl
>  emulation code.(fs/compat.c) Since the search is not locked; when a 
>  ioctl_trans structure is deleted, corruption can occur.
> 
>  The following scenarios discuss the race conditions:
> 
>  1) When the search is hapenning, if any ioctl_trans structure gets deleted; then
>  rather than searching the hash table, the code will start searching the free
>  list.
> 
>  while (t && t->cmd != cmd)
>          ---->Deletion of t happens here; t->next points to ioctl_free_list
>          t = (struct ioctl_trans *)t->next;
>          ---->Now t will point to the ioctl_free_list
> 
>  2) Another race is when  the delete happens while the search holds the pointer
>   to ioctl_trans. In this case the ioctl_trans structure is added to head of
>  the ioctl_free_list. In the meanwhile if another module is registering, then
>  the ioctl_trans from the head of the queue is given. This causes corruption.
> 
>  if (t) {
>          -----> t gets deleted, added to free list and gets alloted again
>          if (t->handler) {
>          ------> t is corrupted
> 
> 

OK.  There's still a race in register_ioctl32_conversion().  It is
performing a GFP_KERNEL memory allocation under lock_kernel().  That
allocation can drop the lock and schedule away, allowing another process to
come in and register the same ioctl.

Pretty unlikely, but easy to prevent: allocate the ioctl_trans before
taking the lock, throw it away again if we ended up not using it.  See the
below reworking of your patch.

>  The solution as I see it is to protect the search too. Since anyway the BKL
>  needs to be used while calling the emulation handler, it can be pushed above
>  to protect the searching. The fallout of this solution is that ioctl_free_list
>  is not required any more.

yes, I agree: we can remove the "deliberate leak" now.  In
compat_sys_ioctl() local variable `t' is dead after we call t->handler,
because the handler may drop BKL.  The function handles that correctly.

btw, please ensure that your patches are in `patch -p1' format, as below. 
Thanks.

fs/compat.c is bizarre.  What's all this unnecessary typecasting for?

	t = (struct ioctl_trans *)ioctl32_hash_table[hash];

and this gem:

-		t1 = (struct ioctl_trans *)(long)t->next;
+		t1 = t->next;

hrm.



diff -puN fs/compat.c~fixes-in-32-bit-ioctl-emulation-code fs/compat.c
--- 25/fs/compat.c~fixes-in-32-bit-ioctl-emulation-code	2004-04-30 20:18:31.920004168 -0700
+++ 25-akpm/fs/compat.c	2004-04-30 20:18:31.925003408 -0700
@@ -282,53 +282,36 @@ static int __init init_sys32_ioctl(void)
 
 __initcall(init_sys32_ioctl);
 
-static struct ioctl_trans *ioctl_free_list;
-
-/* Never free them really. This avoids SMP races. With a Read-Copy-Update
-   enabled kernel we could just use the RCU infrastructure for this. */
-static void free_ioctl(struct ioctl_trans *t) 
-{ 
-	t->cmd = 0; 
-	mb();
-	t->next = ioctl_free_list;
-	ioctl_free_list = t;
-} 
-
-int register_ioctl32_conversion(unsigned int cmd, int (*handler)(unsigned int, unsigned int, unsigned long, struct file *))
+int register_ioctl32_conversion(unsigned int cmd, int (*handler)(unsigned int,
+				unsigned int, unsigned long, struct file *))
 {
 	struct ioctl_trans *t;
+	struct ioctl_trans *new_t;
 	unsigned long hash = ioctl32_hash(cmd);
 
+	new_t = kmalloc(sizeof(*new_t), GFP_KERNEL);
+	if (!new_t)
+		return -ENOMEM;
+
 	lock_kernel(); 
-	for (t = (struct ioctl_trans *)ioctl32_hash_table[hash];
-	     t;
-	     t = t->next) { 
+	for (t = ioctl32_hash_table[hash]; t; t = t->next) {
 		if (t->cmd == cmd) {
-			printk("Trying to register duplicated ioctl32 handler %x\n", cmd);
+			printk(KERN_ERR "Trying to register duplicated ioctl32 "
+					"handler %x\n", cmd);
 			unlock_kernel();
+			kfree(new_t);
 			return -EINVAL; 
 		}
-	} 
-
-	if (ioctl_free_list) { 
-		t = ioctl_free_list; 
-		ioctl_free_list = t->next; 
-	} else { 
-		t = kmalloc(sizeof(struct ioctl_trans), GFP_KERNEL); 
-		if (!t) { 
-			unlock_kernel();
-			return -ENOMEM;
-		}
 	}
-	
-	t->next = NULL;
-	t->cmd = cmd;
-	t->handler = handler; 
-	ioctl32_insert_translation(t);
+	new_t->next = NULL;
+	new_t->cmd = cmd;
+	new_t->handler = handler;
+	ioctl32_insert_translation(new_t);
 
 	unlock_kernel();
 	return 0;
 }
+EXPORT_SYMBOL(register_ioctl32_conversion);
 
 static inline int builtin_ioctl(struct ioctl_trans *t)
 { 
@@ -347,7 +330,7 @@ int unregister_ioctl32_conversion(unsign
 
 	lock_kernel(); 
 
-	t = (struct ioctl_trans *)ioctl32_hash_table[hash];
+	t = ioctl32_hash_table[hash];
 	if (!t) { 
 		unlock_kernel();
 		return -EINVAL;
@@ -358,38 +341,39 @@ int unregister_ioctl32_conversion(unsign
 			printk("%p tried to unregister builtin ioctl %x\n",
 			       __builtin_return_address(0), cmd);
 		} else { 
-		ioctl32_hash_table[hash] = t->next;
-			free_ioctl(t); 
+			ioctl32_hash_table[hash] = t->next;
 			unlock_kernel();
-		return 0;
+			kfree(t);
+			return 0;
 		}
 	} 
 	while (t->next) {
-		t1 = (struct ioctl_trans *)(long)t->next;
+		t1 = t->next;
 		if (t1->cmd == cmd) { 
 			if (builtin_ioctl(t1)) {
-				printk("%p tried to unregister builtin ioctl %x\n",
-				       __builtin_return_address(0), cmd);
+				printk("%p tried to unregister builtin "
+					"ioctl %x\n",
+					__builtin_return_address(0), cmd);
 				goto out;
 			} else { 
-			t->next = t1->next;
-				free_ioctl(t1); 
+				t->next = t1->next;
 				unlock_kernel();
-			return 0;
+				kfree(t1);
+				return 0;
 			}
 		}
 		t = t1;
 	}
-	printk(KERN_ERR "Trying to free unknown 32bit ioctl handler %x\n", cmd);
- out:
+	printk(KERN_ERR "Trying to free unknown 32bit ioctl handler %x\n",
+				cmd);
+out:
 	unlock_kernel();
 	return -EINVAL;
 }
-
-EXPORT_SYMBOL(register_ioctl32_conversion); 
 EXPORT_SYMBOL(unregister_ioctl32_conversion); 
 
-asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
+asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
+				unsigned long arg)
 {
 	struct file * filp;
 	int error = -EBADF;
@@ -404,43 +388,53 @@ asmlinkage long compat_sys_ioctl(unsigne
 		goto out;
 	}
 
-	t = (struct ioctl_trans *)ioctl32_hash_table [ioctl32_hash (cmd)];
+	lock_kernel();
+
+	t = ioctl32_hash_table[ioctl32_hash (cmd)];
 
 	while (t && t->cmd != cmd)
-		t = (struct ioctl_trans *)t->next;
+		t = t->next;
 	if (t) {
 		if (t->handler) { 
-			lock_kernel();
 			error = t->handler(fd, cmd, arg, filp);
 			unlock_kernel();
-		} else
+		} else {
+			unlock_kernel();
 			error = sys_ioctl(fd, cmd, arg);
-	} else if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
-		error = siocdevprivate_ioctl(fd, cmd, arg);
+		}
 	} else {
-		static int count;
-		if (++count <= 50) { 
-			char buf[10];
-			char *path = (char *)__get_free_page(GFP_KERNEL), *fn = "?"; 
-
-			/* find the name of the device. */
-			if (path) {
-		       		fn = d_path(filp->f_dentry, filp->f_vfsmnt, 
-					    path, PAGE_SIZE);
+		unlock_kernel();
+		if (cmd >= SIOCDEVPRIVATE && cmd <= (SIOCDEVPRIVATE + 15)) {
+			error = siocdevprivate_ioctl(fd, cmd, arg);
+		} else {
+			static int count;
+			if (++count <= 50) {
+				char buf[10];
+				char *fn = "?";
+				char *path;
+
+				path = (char *)__get_free_page(GFP_KERNEL);
+
+				/* find the name of the device. */
+				if (path) {
+			       		fn = d_path(filp->f_dentry,
+						filp->f_vfsmnt, path,
+						PAGE_SIZE);
+				}
+
+				sprintf(buf,"'%c'", (cmd>>24) & 0x3f);
+				if (!isprint(buf[1]))
+				    sprintf(buf, "%02x", buf[1]);
+				printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
+					"cmd(%08x){%s} arg(%08x) on %s\n",
+					current->comm, current->pid,
+					(int)fd, (unsigned int)cmd, buf,
+					(unsigned int)arg, fn);
+				if (path)
+					free_page((unsigned long)path);
 			}
-
-			sprintf(buf,"'%c'", (cmd>>24) & 0x3f); 
-			if (!isprint(buf[1]))
-			    sprintf(buf, "%02x", buf[1]);
-			printk("ioctl32(%s:%d): Unknown cmd fd(%d) "
-			       "cmd(%08x){%s} arg(%08x) on %s\n",
-			       current->comm, current->pid,
-			       (int)fd, (unsigned int)cmd, buf, (unsigned int)arg,
-			       fn);
-			if (path) 
-				free_page((unsigned long)path); 
+			error = -EINVAL;
 		}
-		error = -EINVAL;
 	}
 out:
 	fput(filp);

_