linux-kernel - Re: [PATCH] fix SMP ordering hole in fcntl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sun, 25 May 2008 17:32:13 +0200
From:	"Oliver Pinter" <oliver.pntr@...il.com>
To:	"Miloslav Semler" <majkls@...pere.com>
Cc:	linux-kernel@...r.kernel.org, "Adrian Bunk" <bunk@...nel.org>,
	"Adrian Bunk" <bunk@...sta.de>
Subject: Re: [PATCH] fix SMP ordering hole in fcntl_setlk() (CVE-2008-1669)

Add Adrien to CC

On 5/25/08, Miloslav Semler <majkls@...pere.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@...r.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/