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

[Raghavan <raghav@in.ibm.com>]

Hi Andrew,

I went thru the modified patch and it looks fine. Please include
it in the kernel tree.

Thanks 
Raghav

 Fri, Apr 30, 2004 at 08:30:42PM -0700, Andrew Morton wrote:
> 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);
> 
> _
>