lkml.org 
[lkml]   [2004]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Fixes in 32 bit ioctl emulation code
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);
>
> _
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:02    [W:0.074 / U:0.284 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site