Messages in this thread | | | Date | Sun, 25 May 2008 17:32:13 +0200 | From | "Oliver Pinter" <> | Subject | Re: [PATCH] fix SMP ordering hole in fcntl_setlk() (CVE-2008-1669) |
| |
Add Adrien to CC
On 5/25/08, Miloslav Semler <majkls@prepere.com> wrote: > backport of: > http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.25.y.git;a=commitdiff;h=c493a1dd8b3a93b57208319a77a8238f76dabca2 > 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. > > > Signed-off-by: Miloslav Semler > --- > diff -uprN linux-2.6.16.60/fs/locks.c linux-2.6.16.60-new/fs/locks.c > --- linux-2.6.16.60/fs/locks.c 2008-01-27 17:58:41.000000000 +0100 > +++ linux-2.6.16.60-new/fs/locks.c 2008-05-10 17:41:19.000000000 +0200 > @@ -1615,6 +1615,7 @@ int fcntl_setlk(unsigned int fd, struct > struct file_lock *file_lock = locks_alloc_lock(); > struct flock flock; > struct inode *inode; > + struct file *f; > int error; > > if (file_lock == NULL) > @@ -1689,7 +1690,15 @@ again: > * Attempt to detect a close/fcntl race and recover by > * releasing the lock that was just acquired. > */ > - if (!error && fcheck(fd) != filp && flock.l_type != F_UNLCK) { > + /* > + * we need that spin_lock here - it prevents reordering between > + * update of inode->i_flock and check for it done in close(). > + * rcu_read_lock() wouldn't do. > + */ > + spin_lock(¤t->files->file_lock); > + f = fcheck(fd); > + spin_unlock(¤t->files->file_lock); > + if (!error && f != filp && flock.l_type != F_UNLCK) { > flock.l_type = F_UNLCK; > goto again; > } > @@ -1758,6 +1767,7 @@ int fcntl_setlk64(unsigned int fd, struc > struct file_lock *file_lock = locks_alloc_lock(); > struct flock64 flock; > struct inode *inode; > + struct file *f; > int error; > > if (file_lock == NULL) > @@ -1832,7 +1842,10 @@ again: > * Attempt to detect a close/fcntl race and recover by > * releasing the lock that was just acquired. > */ > - if (!error && fcheck(fd) != filp && flock.l_type != F_UNLCK) { > + spin_lock(¤t->files->file_lock); > + f = fcheck(fd); > + spin_unlock(¤t->files->file_lock); > + if (!error && f != filp && flock.l_type != F_UNLCK) { > flock.l_type = F_UNLCK; > goto again; > } > > -- > 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/ >
-- Thanks, Oliver
| |