Check event_queue pointer addresses
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 6 Mar 2019 16:47:29 +0000 (11:47 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 6 Mar 2019 16:47:29 +0000 (11:47 -0500)
We had a couple checks in send_event(), mostly warnings and prints.
This commit changes those warnings to one panic, and pushes the error
handling closer to userspace.

Note that there's not much we can do about a bad pointer in a syscall
struct, other than "silently" fail.  It's almost always a bad user bug,
hence the informational print.

Also note that the kernel can still fault on an unmapped, paged out
address in userspace.  None of that is sorted.  This is just the sanity
checks to make sure we aren't give a kernel pointer.  It also catches
any very low pointers, below the MMAP_LIM.  (So null pointers and
friends).

Reported-by: syzbot+ab67ae285c93a8d288a7@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/event.c
kern/src/syscall.c

index ed5fe39..91243aa 100644 (file)
@@ -370,22 +370,7 @@ void send_event(struct proc *p, struct event_queue *ev_q, struct event_msg *msg,
        if (proc_is_dying(p))
                return;
        printd("[kernel] sending msg to proc %p, ev_q %p\n", p, ev_q);
-       if (!ev_q) {
-               warn("[kernel] Null ev_q - kernel code should check before sending!");
-               return;
-       }
-       if (!is_user_rwaddr(ev_q, sizeof(struct event_queue))) {
-               /* Ought to kill them, just warn for now */
-               printk("[kernel] Illegal addr for ev_q\n");
-               return;
-       }
-       /* This should be caught by "future technology" that can tell when the
-        * kernel PFs on the user's behalf.  For now, we catch common userspace bugs
-        * (had this happen a few times). */
-       if (!PTE_ADDR(ev_q)) {
-               printk("[kernel] Bad addr %p for ev_q\n", ev_q);
-               return;
-       }
+       assert(is_user_rwaddr(ev_q, sizeof(struct event_queue)));
        /* ev_q is a user pointer, so we need to make sure we're in the right
         * address space */
        old_proc = switch_to(p);
index 1b9161a..a596f75 100644 (file)
@@ -1432,8 +1432,12 @@ static int sys_send_event(struct proc *p, struct event_queue *ev_q,
 {
        struct event_msg local_msg = {0};
 
-       if (memcpy_from_user(p, &local_msg, u_msg, sizeof(struct event_msg))) {
-               set_errno(EINVAL);
+       if (memcpy_from_user_errno(p, &local_msg, u_msg,
+                                  sizeof(struct event_msg))) {
+               return -1;
+       }
+       if (!is_user_rwaddr(ev_q, sizeof(struct event_queue))) {
+               set_error(EINVAL, "bad event_queue %p", ev_q);
                return -1;
        }
        send_event(p, ev_q, &local_msg, vcoreid);
@@ -2568,6 +2572,11 @@ void __signal_syscall(struct syscall *sysc, struct proc *p)
                        memset(&local_msg, 0, sizeof(struct event_msg));
                        local_msg.ev_type = EV_SYSCALL;
                        local_msg.ev_arg3 = sysc;
+                       if (!is_user_rwaddr(ev_q, sizeof(struct event_queue))) {
+                               printk("[kernel] syscall had bad ev_q %p\n",
+                                      ev_q);
+                               return;
+                       }
                        send_event(p, ev_q, &local_msg, 0);
                }
        }