Bug 443433 - (CVE-2008-1669) CVE-2008-1669 kernel: add rcu_read_lock() to fcheck() in both dnotify, locks.c and fix fcntl store/load race in locks.c
CVE-2008-1669 kernel: add rcu_read_lock() to fcheck() in both dnotify, locks....
Status: CLOSED ERRATA
Product: Security Response
Classification: Other
Component: vulnerability (Show other bugs)
unspecified
All Linux
high Severity high
: ---
: ---
Assigned To: Red Hat Product Security
reported=20080421,public=20080506,sou...
: Security
Depends On: 443436 443437 443438 443439 443440
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-21 11:12 EDT by Jan Lieskovsky
Modified: 2010-12-21 12:17 EST (History)
8 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2010-12-21 12:17:24 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description Jan Lieskovsky 2008-04-21 11:12:56 EDT
Description of problem:

Alexander Viro has reported the following problems:

a) fcheck() needs rcu_read_lock() or current->files->file_lock.
Both in dnotify.c and locks.c.

b) locks.c (fcntl_setlk()) has subtler one - 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).
Comment 2 Jan Lieskovsky 2008-04-21 11:16:17 EDT
Proposed patch:


diff -urN linux-2.6.18.x86_64/fs/dnotify.c linux-2.6.18.x86_64-fix/fs/dnotify.c
--- linux-2.6.18.x86_64/fs/dnotify.c    2006-09-19 23:42:06.000000000 -0400
+++ linux-2.6.18.x86_64-fix/fs/dnotify.c        2008-04-21 02:54:33.000000000 -0400
@@ -20,6 +20,9 @@
 #include <linux/init.h>
 #include <linux/spinlock.h>
 #include <linux/slab.h>
+#ifndef __GENKSYMS__
+#include <linux/file.h>
+#endif
 
 int dir_notify_enable __read_mostly = 1;
 
@@ -66,6 +69,7 @@
        struct dnotify_struct **prev;
        struct inode *inode;
        fl_owner_t id = current->files;
+       struct file *f;
        int error = 0;
 
        if ((arg & ~DN_MULTISHOT) == 0) {
@@ -92,6 +96,15 @@
                prev = &odn->dn_next;
        }
 
+       rcu_read_lock();
+       f = fcheck(fd);
+       rcu_read_unlock();
+       /* we'd lost the race with close(), sod off silently */
+       /* note that inode->i_lock prevents reordering problems
+        * between accesses to descriptor table and ->i_dnotify */
+       if (f != filp)
+               goto out_free;
+
        error = f_setown(filp, current->pid, 0);
        if (error)
                goto out_free;
diff -urN linux-2.6.18.x86_64/fs/locks.c linux-2.6.18.x86_64-fix/fs/locks.c
--- linux-2.6.18.x86_64/fs/locks.c      2008-04-21 03:32:42.000000000 -0400
+++ linux-2.6.18.x86_64-fix/fs/locks.c  2008-04-21 03:42:31.000000000 -0400
@@ -1683,6 +1683,7 @@
        struct file_lock *file_lock = locks_alloc_lock();
        struct flock flock;
        struct inode *inode;
+       struct file *f;
        int error;
 
        if (file_lock == NULL)
@@ -1757,7 +1758,15 @@
         * 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;
        }
@@ -1826,6 +1835,7 @@
        struct file_lock *file_lock = locks_alloc_lock();
        struct flock64 flock;
        struct inode *inode;
+       struct file *f;
        int error;
 
        if (file_lock == NULL)
@@ -1900,7 +1910,10 @@
         * 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;
        }
Comment 4 Jan Lieskovsky 2008-05-07 02:37:52 EDT
The fcntl_setlk()/close() race also already known upstream, relevant
- commit id 0b2bac2f1ea0d33a3621b27ca68b9ae760fca2e9
- link to upstream post:

http://www.kernel.org/pub/linux/kernel/v2.6/ChangeLog-2.6.25.2

-> lifting embargo on this one.
Comment 5 Fedora Update System 2008-05-14 17:32:00 EDT
kernel-2.6.24.7-92.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 6 Fedora Update System 2008-05-14 18:14:49 EDT
kernel-2.6.25.3-18.fc9 has been pushed to the Fedora 9 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 7 Fedora Update System 2008-05-17 18:21:24 EDT
kernel-2.6.23.17-88.fc7 has been pushed to the Fedora 7 stable repository.  If problems still persist, please make note of it in this bug report.
Comment 10 Vincent Danen 2010-12-21 12:17:24 EST
This was addressed via:

Red Hat Enterprise Linux version 3 (RHSA-2008:0211)
Red Hat Enterprise Linux version 5 (RHSA-2008:0233)
Red Hat Enterprise Linux version 4 (RHSA-2008:0237)

Note You need to log in before you can comment on or make changes to this bug.