Sophie

Sophie

distrib > Scientific%20Linux > 5x > x86_64 > by-pkgid > 27922b4260f65d317aabda37e42bbbff > files > 823

kernel-2.6.18-238.el5.src.rpm

From: Danny Feng <dfeng@redhat.com>
Date: Wed, 6 Jan 2010 08:18:55 -0500
Subject: [fs] fasync: split 'fasync_helper()' into separate add/remove functions
Message-id: 20100106081908.20920.31735.sendpatchset@dhcp-65-180.nay.redhat.com
O-Subject: [kernel team] [PATCH RHEL5.5][EMBARGOED] CVE-2009-4141: kernel: create_elf_tables can leave urandom in a bad state
Bugzilla: 548657
RH-Acked-by: David Howells <dhowells@redhat.com>
CVE: CVE-2009-4141

RHBZ#:
https://bugzilla.redhat.com/show_bug.cgi?id=548657

Description:
Linus provided the final patch below for CVE-2009-4141, the root cause was
determined to be a use-after-free of locked async file descriptors, and it is
believed to have been introduced here

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=233e70f4228e78eb2f80dc6650f65d3ae3dbf17c

This is still embargoed, so the patch is not upstream yet.

   fasync: split 'fasync_helper()' into separate add/remove functions

   Yes, the add and remove cases do share the same basic loop and the
   locking, but the compiler can inline and then CSE some of the end result
   anyway.  And splitting it up makes the code way easier to follow,
   and makes it clearer exactly what the semantics are.

   In particular, we must make sure that the FASYNC flag in file->f_flags
   exactly matches the state of "is this file on any fasync list", since
   not only is that flag visible to user space (F_GETFL), but we also use
   that flag to check whether we need to remove any fasync entries on file
   close.

   We got that wrong for the case of a mixed use of file locking (which
   tries to remove any fasync entries for file leases) and fasync.

   Splitting the function up also makes it possible to do some future
   optimizations without making the function even messier.  In particular,
   since the FASYNC flag has to match the state of "is this on a list", we
   can do the following future optimizations:

    - on remove, we don't even need to get the locks and traverse the list
      if FASYNC isn't set, since we can know a priori that there is no
      point (this is effectively the same optimization that we already do
      in __fput() wrt removing fasync on file close)

    - on add, we can use the FASYNC flag to decide whether we are changing
      an existing entry or need to allocate a new one.

   but this is just the cleanup + fix for the FASYNC flag.

   Cc: Al Viro <viro@ZenIV.linux.org.uk>
   Cc: Tavis Ormandy <taviso@google.com>
   Cc: Jeff Dike <jdike@addtoit.com>
   Cc: Matt Mackall <mpm@selenic.com>
   Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

This is fully backport of patch provided by the reporter, the only difference
is RHEL5 doesn't have filp->f_lock, it was introduced by moving f_ep_lock out
of CONFIG_EPOLL and renamed. Since RHEL5 has CONFIG_EPOLL by default, so this
patch is using filp->f_ep_lock instead to avoid kabi issue.

Brew Build:
https://brewweb.devel.redhat.com/taskinfo?taskID=2173206

Test status:
I've applied fasync support for drivers/char/random, then applied above patch,
reproducer works fine.

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 522fc2e..139b6d1 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -537,49 +537,91 @@ static DEFINE_RWLOCK(fasync_lock);
 static kmem_cache_t *fasync_cache __read_mostly;
 
 /*
- * fasync_helper() is used by some character device drivers (mainly mice)
- * to set up the fasync queue. It returns negative on error, 0 if it did
- * no changes and positive if it added/deleted the entry.
+ * Remove a fasync entry. If successfully removed, return
+ * positive and clear the FASYNC flag. If no entry exists,
+ * do nothing and return 0.
+ *
+ * NOTE! It is very important that the FASYNC flag always
+ * match the state "is the filp on a fasync list".
+ *
+ * We always take the 'filp->f_ep_lock', in since fasync_lock
+ * needs to be irq-safe.
  */
-int fasync_helper(int fd, struct file * filp, int on, struct fasync_struct **fapp)
+static int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
 {
 	struct fasync_struct *fa, **fp;
-	struct fasync_struct *new = NULL;
 	int result = 0;
-
-	if (on) {
-		new = kmem_cache_alloc(fasync_cache, SLAB_KERNEL);
-		if (!new)
-			return -ENOMEM;
-	}
+	spin_lock(&filp->f_ep_lock);
 	write_lock_irq(&fasync_lock);
 	for (fp = fapp; (fa = *fp) != NULL; fp = &fa->fa_next) {
-		if (fa->fa_file == filp) {
-			if(on) {
-				fa->fa_fd = fd;
-				kmem_cache_free(fasync_cache, new);
-			} else {
-				*fp = fa->fa_next;
-				kmem_cache_free(fasync_cache, fa);
-				result = 1;
-			}
-			goto out;
-		}
+		if (fa->fa_file != filp)
+			continue;
+		*fp = fa->fa_next;
+		kmem_cache_free(fasync_cache, fa);
+		filp->f_flags &= ~FASYNC;
+		result = 1;
+		break;
 	}
+	write_unlock_irq(&fasync_lock);
+	spin_unlock(&filp->f_ep_lock);
+	return result;
+}
 
-	if (on) {
-		new->magic = FASYNC_MAGIC;
-		new->fa_file = filp;
-		new->fa_fd = fd;
-		new->fa_next = *fapp;
-		*fapp = new;
-		result = 1;
+/*
+ * Add a fasync entry. Return negative on error, positive if
+ * added, and zero if did nothing but change an existing one.
+ *
+ * NOTE! It is very important that the FASYNC flag always
+ * match the state "is the filp on a fasync list".
+ */
+static int fasync_add_entry(int fd, struct file *filp,
+			    struct fasync_struct **fapp)
+{
+	struct fasync_struct *new, *fa, **fp;
+	int result = 0;
+
+	new = kmem_cache_alloc(fasync_cache, GFP_KERNEL);
+	if (!new)
+		return -ENOMEM;
+
+	spin_lock(&filp->f_ep_lock);
+	write_lock_irq(&fasync_lock);
+	for (fp = fapp; (fa = *fp) != NULL; fp = &fa->fa_next) {
+		if (fa->fa_file != filp)
+			continue;
+		fa->fa_fd = fd;
+		kmem_cache_free(fasync_cache, new);
+		goto out;
 	}
+
+	new->magic = FASYNC_MAGIC;
+	new->fa_file = filp;
+	new->fa_fd = fd;
+	new->fa_next = *fapp;
+	*fapp = new;
+	result = 1;
+	filp->f_flags |= FASYNC;
+
 out:
 	write_unlock_irq(&fasync_lock);
+	spin_unlock(&filp->f_ep_lock);
 	return result;
 }
 
+/*
+ * fasync_helper() is used by almost all character device drivers
+ * to set up the fasync queue, and for regular files by the file
+ * lease code. It returns negative on error, 0 if it did no changes
+ * and positive if it added/deleted the entry.
+ */
+int fasync_helper(int fd, struct file *filp, int on,
+		  struct fasync_struct **fapp)
+{
+	if (!on)
+		return fasync_remove_entry(filp, fapp);
+	return fasync_add_entry(fd, filp, fapp);
+}
+
 EXPORT_SYMBOL(fasync_helper);
 
 void __kill_fasync(struct fasync_struct *fa, int sig, int band)