Fix SMP ordering hole in fcntl_setlk() (CVE-2008-1669)

[ sync up with 2.6 commit 0b2bac2f1ea0d33a3621b27ca68b9ae760fca2e9 ]

fcntl_setlk()/close() race prevention has a subtle hole - we need to
make sure that if we *do* have an fcntl/close race on SMP box, the
access to descriptor table and inode->i_flock won't get reordered.

As it is, we get STORE inode->i_flock, LOAD descriptor table entry vs.
STORE descriptor table entry, LOAD inode->i_flock with not a single
lock in common on both sides.  We do have BKL around the first STORE,
but check in locks_remove_posix() is outside of BKL and for a good
reason - we don't want BKL on common path of close(2).

Solution is to hold ->file_lock around fcheck() in there; that orders
us wrt removal from descriptor table that preceded locks_remove_posix()
on close path and we either come first (in which case eviction will be
handled by the close side) or we'll see the effect of close and do
eviction ourselves.  Note that even though it's read-only access,
we do need ->file_lock here - rcu_read_lock() won't be enough to
order the things.

[ w@1wt.eu: this patch also includes a missing fix for and older
  bug affecting the same code, which was already fixed in 2.6. As
  of now, 2.4 is in sync with 2.6 concerning this bug. ]

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 9447229..abadfdf 100644 (file)
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -283,7 +283,7 @@ static long do_fcntl(unsigned int fd, unsigned int cmd,
                        break;
                case F_SETLK:
                case F_SETLKW:
-                       err = fcntl_setlk(fd, cmd, (struct flock *) arg);
+                       err = fcntl_setlk(fd, filp, cmd, (struct flock *) arg);
                        break;
                case F_GETOWN:
                        /*
@@ -369,10 +369,12 @@ asmlinkage long sys_fcntl64(unsigned int fd, unsigned int cmd, unsigned long arg
                        err = fcntl_getlk64(fd, (struct flock64 *) arg);
                        break;
                case F_SETLK64:
-                       err = fcntl_setlk64(fd, cmd, (struct flock64 *) arg);
+                       err = fcntl_setlk64(fd, filp, cmd,
+                                       (struct flock64 *) arg);
                        break;
                case F_SETLKW64:
-                       err = fcntl_setlk64(fd, cmd, (struct flock64 *) arg);
+                       err = fcntl_setlk64(fd, filp, cmd,
+                                       (struct flock64 *) arg);
                        break;
                default:
                        err = do_fcntl(fd, cmd, arg, filp);

diff --git a/fs/locks.c b/fs/locks.c
index 2f21d25..412e91b 100644 (file)
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1477,9 +1477,10 @@ out:
 /* Apply the lock described by l to an open file descriptor.
  * This implements both the F_SETLK and F_SETLKW commands of fcntl().
  */
-int fcntl_setlk(unsigned int fd, unsigned int cmd, struct flock *l)
+int fcntl_setlk(unsigned int fd, struct file *filp, unsigned int cmd,
+               struct flock *l)
 {
-       struct file *filp;
+       struct file *f;
        struct file_lock *file_lock = locks_alloc_lock();
        struct flock flock;
        struct inode *inode;
@@ -1498,11 +1499,6 @@ int fcntl_setlk(unsigned int fd, unsigned int cmd, struct flock *l)
        /* Get arguments and validate them ...
         */
 
-       error = -EBADF;
-       filp = fget(fd);
-       if (!filp)
-               goto out;
-
        error = -EINVAL;
        inode = filp->f_dentry->d_inode;
 
@@ -1515,23 +1511,23 @@ int fcntl_setlk(unsigned int fd, unsigned int cmd, struct flock *l)
 
                if (mapping->i_mmap_shared != NULL) {
                        error = -EAGAIN;
-                       goto out_putf;
+                       goto out;
                }
        }
 
        error = flock_to_posix_lock(filp, file_lock, &flock);
        if (error)
-               goto out_putf;
+               goto out;
        
        error = -EBADF;
        switch (flock.l_type) {
        case F_RDLCK:
                if (!(filp->f_mode & FMODE_READ))
-                       goto out_putf;
+                       goto out;
                break;
        case F_WRLCK:
                if (!(filp->f_mode & FMODE_WRITE))
-                       goto out_putf;
+                       goto out;
                break;
        case F_UNLCK:
                break;
@@ -1549,23 +1545,29 @@ int fcntl_setlk(unsigned int fd, unsigned int cmd, struct flock *l)
        }
 }
                if (!(filp->f_mode & 3))
-                       goto out_putf;
+                       goto out;
                break;
 #endif
        default:
                error = -EINVAL;
-               goto out_putf;
+               goto out;
        }
 
+do_it:
        if (filp->f_op && filp->f_op->lock != NULL) {
                error = filp->f_op->lock(filp, cmd, file_lock);
                if (error < 0)
-                       goto out_putf;
+                       goto out;
        }
        error = posix_lock_file(filp, file_lock, cmd == F_SETLKW);
-
-out_putf:
-       fput(filp);
+       read_lock(&current->files->file_lock);
+       f = fcheck(fd);
+       read_unlock(&current->files->file_lock);
+       /* lost race with close, kill stuck lock if close didn't get it */
+       if (!error && flock.l_type != F_UNLCK && filp != f) {
+               file_lock->fl_type = F_UNLCK;
+               goto do_it;
+       }
 out:
        locks_free_lock(file_lock);
        return error;
@@ -1633,9 +1635,10 @@ out:
 /* Apply the lock described by l to an open file descriptor.
  * This implements both the F_SETLK and F_SETLKW commands of fcntl().
  */
-int fcntl_setlk64(unsigned int fd, unsigned int cmd, struct flock64 *l)
+int fcntl_setlk64(unsigned int fd, struct file *filp, unsigned int cmd,
+               struct flock64 *l)
 {
-       struct file *filp;
+       struct file *f;
        struct file_lock *file_lock = locks_alloc_lock();
        struct flock64 flock;
        struct inode *inode;
@@ -1654,11 +1657,6 @@ int fcntl_setlk64(unsigned int fd, unsigned int cmd, struct flock64 *l)
        /* Get arguments and validate them ...
         */
 
-       error = -EBADF;
-       filp = fget(fd);
-       if (!filp)
-               goto out;
-
        error = -EINVAL;
        inode = filp->f_dentry->d_inode;
 
@@ -1671,23 +1669,23 @@ int fcntl_setlk64(unsigned int fd, unsigned int cmd, struct flock64 *l)
 
                if (mapping->i_mmap_shared != NULL) {
                        error = -EAGAIN;
-                       goto out_putf;
+                       goto out;
                }
        }
 
        error = flock64_to_posix_lock(filp, file_lock, &flock);
        if (error)
-               goto out_putf;
+               goto out;
        
        error = -EBADF;
        switch (flock.l_type) {
        case F_RDLCK:
                if (!(filp->f_mode & FMODE_READ))
-                       goto out_putf;
+                       goto out;
                break;
        case F_WRLCK:
                if (!(filp->f_mode & FMODE_WRITE))
-                       goto out_putf;
+                       goto out;
                break;
        case F_UNLCK:
                break;
@@ -1695,18 +1693,24 @@ int fcntl_setlk64(unsigned int fd, unsigned int cmd, struct flock64 *l)
        case F_EXLCK:
        default:
                error = -EINVAL;
-               goto out_putf;
+               goto out;
        }
 
+do_it:
        if (filp->f_op && filp->f_op->lock != NULL) {
                error = filp->f_op->lock(filp, cmd, file_lock);
                if (error < 0)
-                       goto out_putf;
+                       goto out;
        }
        error = posix_lock_file(filp, file_lock, cmd == F_SETLKW64);
-
-out_putf:
-       fput(filp);
+       read_lock(&current->files->file_lock);
+       f = fcheck(fd);
+       read_unlock(&current->files->file_lock);
+       /* lost race with close, kill stuck lock if close didn't get it */
+       if (!error && flock.l_type != F_UNLCK && filp != f) {
+               file_lock->fl_type = F_UNLCK;
+               goto do_it;
+       }
 out:
        locks_free_lock(file_lock);
        return error;

diff --git a/include/linux/fs.h b/include/linux/fs.h
index c59e01d..feac534 100644 (file)
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -658,10 +658,12 @@ extern struct list_head file_lock_list;
 #include <linux/fcntl.h>
 
 extern int fcntl_getlk(unsigned int, struct flock *);
-extern int fcntl_setlk(unsigned int, unsigned int, struct flock *);
+extern int fcntl_setlk(unsigned int, struct file *, unsigned int,
+                       struct flock *);
 
 extern int fcntl_getlk64(unsigned int, struct flock64 *);
-extern int fcntl_setlk64(unsigned int, unsigned int, struct flock64 *);
+extern int fcntl_setlk64(unsigned int, struct file *, unsigned int,
+                       struct flock64 *);
 
 /* fs/locks.c */
 extern void locks_init_lock(struct file_lock *);