Fix data leak in fs_file_write()
authorBarret Rhoden <brho@cs.berkeley.edu>
Sat, 2 Mar 2019 01:13:31 +0000 (20:13 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Sat, 2 Mar 2019 01:16:11 +0000 (20:16 -0500)
The user could give write() the valid address of a non-resident page,
particularly one in a file-backed region.  The kernel would fail to read
from it, and thus not write any data into the page cache.  The page
cache page is uninitialized, and thus contains arbitrary kernel data.

This is a stopgap, until I sort out handling page faults in the kernel -
particularly on file-backed virtual addresses.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/include/umem.h
kern/src/ns/fs_file.c
kern/src/umem.c

index 9659b8d..3ff2118 100644 (file)
@@ -55,8 +55,8 @@ int memcpy_to_user(struct proc *p, void *dest, const void *src, size_t len);
 /* Same as above, but sets errno */
 int memcpy_from_user_errno(struct proc *p, void *dst, const void *src, int len);
 int memcpy_to_user_errno(struct proc *p, void *dst, const void *src, int len);
-void memcpy_to_safe(void *dst, const void *src, size_t amt);
-void memcpy_from_safe(void *dst, const void *src, size_t amt);
+int memcpy_to_safe(void *dst, const void *src, size_t amt);
+int memcpy_from_safe(void *dst, const void *src, size_t amt);
 
 /* Creates a buffer (kmalloc) and safely copies into it from va.  Can return an
  * error code.  Check its response with IS_ERR().  Must be paired with
index 09aa073..d1a4086 100644 (file)
@@ -432,7 +432,20 @@ size_t fs_file_write(struct fs_file *f, const uint8_t *buf, size_t count,
                if (error)
                        error(-error, "write pm_load_page failed");
                copy_amt = MIN(PGSIZE - pg_off, buf_end - buf);
-               memcpy_from_safe(page2kva(page) + pg_off, buf, copy_amt);
+               /* TODO: If you ask the kernel to write from a user address that
+                * will page fault, the memcpy will fail and we'll move on to
+                * the next region.  To avoid leaving a chunk of uninitialized
+                * memory, we'll zero it out in the page cache.  Otherwise the
+                * user could come back and read old kernel data.
+                *
+                * The real fix will be to have the kernel throw an error if it
+                * was a segfault or block if it wasn't.  Note that when the
+                * kernel attempts to access the user's page, it does so with a
+                * handle_page_fault_nofile, which won't attempt to handle
+                * file-backed VMRs *even if* the file is in the page cache.
+                * Yikes! */
+               if (memcpy_from_safe(page2kva(page) + pg_off, buf, copy_amt))
+                       memset(page2kva(page) + pg_off, 0, copy_amt);
                buf += copy_amt;
                so_far += copy_amt;
                atomic_or(&page->pg_flags, PG_DIRTY);
index 0baea20..fea17ad 100644 (file)
@@ -119,20 +119,26 @@ int memcpy_to_user_errno(struct proc *p, void *dst, const void *src, int len)
  * TODO: (KFOP) Probably shouldn't do this.  Either memcpy directly, or split
  * out the is_user_r(w)addr from copy_{to,from}_user().  Or throw from the fault
  * handler.  Right now, we ignore the ret/errors completely. */
-void memcpy_to_safe(void *dst, const void *src, size_t amt)
+int memcpy_to_safe(void *dst, const void *src, size_t amt)
 {
+       int error = 0;
+
        if (!is_ktask(per_cpu_info[core_id()].cur_kthread))
-               memcpy_to_user(current, dst, src, amt);
+               error = memcpy_to_user(current, dst, src, amt);
        else
                memcpy(dst, src, amt);
+       return error;
 }
 
-void memcpy_from_safe(void *dst, const void *src, size_t amt)
+int memcpy_from_safe(void *dst, const void *src, size_t amt)
 {
+       int error = 0;
+
        if (!is_ktask(per_cpu_info[core_id()].cur_kthread))
-               memcpy_from_user(current, dst, src, amt);
+               error = memcpy_from_user(current, dst, src, amt);
        else
                memcpy(dst, src, amt);
+       return error;
 }
 
 /* Creates a buffer (kmalloc) and safely copies into it from va.  Can return an