Bug 443433 (CVE-2008-1669)

Summary:	CVE-2008-1669 kernel: add rcu_read_lock() to fcheck() in both dnotify, locks.c and fix fcntl store/load race in locks.c
Product:	[Other] Security Response	Reporter:	Jan Lieskovsky <jlieskov>
Component:	vulnerability	Assignee:	Red Hat Product Security <security-response-team>
Status:	CLOSED ERRATA	QA Contact:
Severity:	high	Docs Contact:
Priority:	high
Version:	unspecified	CC:	anton, aviro, dhoward, dwu, lwang, qcai, security-response-team, vgoyal
Target Milestone:	---	Keywords:	Security
Target Release:	---
Hardware:	All
OS:	Linux
Whiteboard:
Fixed In Version:		Doc Type:	Bug Fix
Doc Text:		Story Points:	---
Clone Of:		Environment:
Last Closed:	2010-12-21 17:17:24 UTC	Type:	---
Regression:	---	Mount Type:	---
Documentation:	---	CRM:
Verified Versions:		Category:	---
oVirt Team:	---	RHEL 7.3 requirements from Atomic Host:
Cloudforms Team:	---	Target Upstream Version:
Embargoed:
Bug Depends On:	443436, 443437, 443438, 443439, 443440
Bug Blocks:

Description Jan Lieskovsky 2008-04-21 15:12:56 UTC

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 15:16:17 UTC

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 06:37:52 UTC

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 21:32:00 UTC

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 22:14:49 UTC

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 22:21:24 UTC

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 17:17:24 UTC

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)