Fix data leak in fs_file_write()
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>
diff --git a/kern/include/umem.h b/kern/include/umem.h
index 9659b8d..3ff2118 100644
--- a/kern/include/umem.h
+++ b/kern/include/umem.h
@@ -55,8 +55,8 @@
/* 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
diff --git a/kern/src/ns/fs_file.c b/kern/src/ns/fs_file.c
index 09aa073..d1a4086 100644
--- a/kern/src/ns/fs_file.c
+++ b/kern/src/ns/fs_file.c
@@ -432,7 +432,20 @@
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);
diff --git a/kern/src/umem.c b/kern/src/umem.c
index 0baea20..fea17ad 100644
--- a/kern/src/umem.c
+++ b/kern/src/umem.c
@@ -119,20 +119,26 @@
* 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
