OSDN Git Service

[PATCH] Fix incorrect user space access locking in mincore() (CVE-2006-4814)
authorLinus Torvalds <torvalds@osdl.org>
Sun, 17 Dec 2006 14:38:15 +0000 (14:38 +0000)
committerWilly Tarreau <w@1wt.eu>
Mon, 18 Dec 2006 08:28:44 +0000 (09:28 +0100)
Fix incorrect user space access locking in mincore()

Doug Chapman noticed that mincore() will do a "copy_to_user()" of the
result while holding the mmap semaphore for reading, which is a big
no-no.  While a recursive read-lock on a semaphore in the case of a page
fault happens to work, we don't actually allow them due to deadlock
scenarios with writers due to fairness issues.

Doug and Marcel sent in a patch to fix it, but I decided to just rewrite
the mess instead - not just fixing the locking problem, but making the
code smaller and (imho) much easier to understand.

Hugh "ported" it to 2.4:
please note two slight changes to behaviour under error conditions:

(a) mincore used to report -EINVAL if input len was so big that the
given area wrapped: that shouldn't be a distinct case from crossing
a hole in the address space, 2.6.11 corrected the error to -ENOMEM,
and this patch extends that correction to 2.4.

(b) mincore used to report -ENOMEM if the given area crossed a hole
in the address space, but continued to fill in the vector for file-
backed regions above - yet didn't fill the vector with non-present
entries for the hole, just ignored it.  This patch continues to
report -ENOMEM if the given area crosses a hole in the address
space, but simply stops filling the vector at that point.  We
doubt any app could be relying on the previous weird behaviour.

Cc: Doug Chapman <dchapman@redhat.com>
Cc: Marcel Holtmann <holtmann@redhat.com>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: Andrew Morton <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
Signed-off-by: Hugh Dickins <hugh@veritas.com>
mm/filemap.c

index a31c553..deea109 100644 (file)
@@ -1,7 +1,7 @@
 /*
  *     linux/mm/filemap.c
  *
- * Copyright (C) 1994-1999  Linus Torvalds
+ * Copyright (C) 1994-2006  Linus Torvalds
  */
 
 /*
@@ -2808,46 +2808,51 @@ static unsigned char mincore_page(struct vm_area_struct * vma,
        return present;
 }
 
-static long mincore_vma(struct vm_area_struct * vma,
-       unsigned long start, unsigned long end, unsigned char * vec)
+/*
+ * Do a chunk of "sys_mincore()". We've already checked
+ * all the arguments, we hold the mmap semaphore: we should
+ * just return the amount of info we're asked for.
+ */
+static long do_mincore(unsigned long addr, unsigned char *vec, unsigned long pages)
 {
-       long error, i, remaining;
-       unsigned char * tmp;
-
-       error = -ENOMEM;
-       if (!vma->vm_file)
-               return error;
+       unsigned long i, nr, pgoff;
+       struct vm_area_struct *vma = find_vma(current->mm, addr);
 
-       start = ((start - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
-       if (end > vma->vm_end)
-               end = vma->vm_end;
-       end = ((end - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff;
-
-       error = -EAGAIN;
-       tmp = (unsigned char *) __get_free_page(GFP_KERNEL);
-       if (!tmp)
-               return error;
+       /*
+        * find_vma() didn't find anything above us, or we're
+        * in an unmapped hole in the address space: ENOMEM.
+        */
+       if (!vma || addr < vma->vm_start)
+               return -ENOMEM;
 
-       /* (end - start) is # of pages, and also # of bytes in "vec */
-       remaining = (end - start),
+       /*
+        * Ok, got it. But check whether it's a segment we support
+        * mincore() on. Right now, we don't do any anonymous mappings.
+        *
+        * FIXME: This is just stupid. And returning ENOMEM is 
+        * stupid too. We should just look at the page tables. But
+        * this is what we've traditionally done, so we'll just
+        * continue doing it.
+        */
+       if (!vma->vm_file)
+               return -ENOMEM;
 
-       error = 0;
-       for (i = 0; remaining > 0; remaining -= PAGE_SIZE, i++) {
-               int j = 0;
-               long thispiece = (remaining < PAGE_SIZE) ?
-                                               remaining : PAGE_SIZE;
+       /*
+        * Calculate how many pages there are left in the vma, and
+        * what the pgoff is for our address.
+        */
+       nr = (vma->vm_end - addr) >> PAGE_SHIFT;
+       if (nr > pages)
+               nr = pages;
 
-               while (j < thispiece)
-                       tmp[j++] = mincore_page(vma, start++);
+       pgoff = (addr - vma->vm_start) >> PAGE_SHIFT;
+       pgoff += vma->vm_pgoff;
 
-               if (copy_to_user(vec + PAGE_SIZE * i, tmp, thispiece)) {
-                       error = -EFAULT;
-                       break;
-               }
-       }
+       /* And then we just fill the sucker in.. */
+       for (i = 0 ; i < nr; i++, pgoff++)
+               vec[i] = mincore_page(vma, pgoff);
 
-       free_page((unsigned long) tmp);
-       return error;
+       return nr;
 }
 
 /*
@@ -2867,77 +2872,61 @@ static long mincore_vma(struct vm_area_struct * vma,
  * return values:
  *  zero    - success
  *  -EFAULT - vec points to an illegal address
- *  -EINVAL - addr is not a multiple of PAGE_CACHE_SIZE,
- *             or len has a nonpositive value
+ *  -EINVAL - addr is not a multiple of PAGE_CACHE_SIZE
  *  -ENOMEM - Addresses in the range [addr, addr + len] are
  *             invalid for the address space of this process, or
  *             specify one or more pages which are not currently
  *             mapped
  *  -EAGAIN - A kernel resource was temporarily unavailable.
  */
-asmlinkage long sys_mincore(unsigned long start, size_t len,
-       unsigned char * vec)
+asmlinkage long sys_mincore(unsigned long start, size_t len, unsigned char *vec)
 {
-       int index = 0;
-       unsigned long end;
-       struct vm_area_struct * vma;
-       int unmapped_error = 0;
-       long error = -EINVAL;
+       long retval;
+       unsigned long pages;
+       unsigned char *tmp;
 
-       down_read(&current->mm->mmap_sem);
+       /* Check the start address: needs to be page-aligned.. */
+       if (start & ~PAGE_CACHE_MASK)
+               return -EINVAL;
 
-       if (start & ~PAGE_CACHE_MASK)
-               goto out;
-       len = (len + ~PAGE_CACHE_MASK) & PAGE_CACHE_MASK;
-       end = start + len;
-       if (end < start)
-               goto out;
+       /* ..and we need to be passed a valid user-space range */
+       if (!access_ok(VERIFY_READ, (void *) start, len))
+               return -ENOMEM;
 
-       error = 0;
-       if (end == start)
-               goto out;
+       /* This also avoids any overflows on PAGE_CACHE_ALIGN */
+       pages = len >> PAGE_SHIFT;
+       pages += (len & ~PAGE_MASK) != 0;
 
-       /*
-        * If the interval [start,end) covers some unmapped address
-        * ranges, just ignore them, but return -ENOMEM at the end.
-        */
-       vma = find_vma(current->mm, start);
-       for (;;) {
-               /* Still start < end. */
-               error = -ENOMEM;
-               if (!vma)
-                       goto out;
+       if (!access_ok(VERIFY_WRITE, vec, pages))
+               return -EFAULT;
 
-               /* Here start < vma->vm_end. */
-               if (start < vma->vm_start) {
-                       unmapped_error = -ENOMEM;
-                       start = vma->vm_start;
-               }
+       tmp = (void *) __get_free_page(GFP_USER);
+       if (!tmp)
+               return -EAGAIN;
 
-               /* Here vma->vm_start <= start < vma->vm_end. */
-               if (end <= vma->vm_end) {
-                       if (start < end) {
-                               error = mincore_vma(vma, start, end,
-                                                       &vec[index]);
-                               if (error)
-                                       goto out;
-                       }
-                       error = unmapped_error;
-                       goto out;
-               }
+       retval = 0;
+       while (pages) {
+               /*
+                * Do at most PAGE_SIZE entries per iteration, due to
+                * the temporary buffer size.
+                */
+               down_read(&current->mm->mmap_sem);
+               retval = do_mincore(start, tmp, min(pages, PAGE_SIZE));
+               up_read(&current->mm->mmap_sem);
 
-               /* Here vma->vm_start <= start < vma->vm_end < end. */
-               error = mincore_vma(vma, start, vma->vm_end, &vec[index]);
-               if (error)
-                       goto out;
-               index += (vma->vm_end - start) >> PAGE_CACHE_SHIFT;
-               start = vma->vm_end;
-               vma = vma->vm_next;
+               if (retval <= 0)
+                       break;
+               if (copy_to_user(vec, tmp, retval)) {
+                       retval = -EFAULT;
+                       break;
+               }
+               pages -= retval;
+               vec += retval;
+               start += retval << PAGE_SHIFT;
+               retval = 0;
        }
-
-out:
-       up_read(&current->mm->mmap_sem);
-       return error;
+       free_page((unsigned long) tmp);
+       return retval;
 }
 
 static inline