lkml.org 
[lkml]   [2008]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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(&current->files->file_lock);
> + f = fcheck(fd);
> + spin_unlock(&current->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(&current->files->file_lock);
> + f = fcheck(fd);
> + spin_unlock(&current->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


\
 
 \ /
  Last update: 2008-05-25 17:35    [W:0.036 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site