Sanitize vcoreid from untrusted sources
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 11 Apr 2019 00:57:15 +0000 (20:57 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 11 Apr 2019 00:57:15 +0000 (20:57 -0400)
Reported-by: syzbot+a21358b54760bd84b850@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/include/process.h
kern/src/event.c
kern/src/monitor.c
kern/src/process.c
kern/src/syscall.c

index 69ece85..4a38214 100644 (file)
@@ -118,6 +118,14 @@ void proc_free_set(struct process_set *pset);
 
 /* Vcoremap info: */
 uint32_t proc_get_vcoreid(struct proc *p);
+
+/* In some places, vcoreid may be an int.  In other/older places, it's unsigned.
+ * Casting to unsigned will also catch < 0 for sane systems. */
+static inline bool proc_vcoreid_is_safe(struct proc *p, uint32_t vcoreid)
+{
+       return vcoreid < p->procinfo->max_vcores;
+}
+
 /* TODO: make all of these inline once we gut the Env crap */
 bool vcore_is_mapped(struct proc *p, uint32_t vcoreid);
 uint32_t vcore2vcoreid(struct proc *p, struct vcore *vc);
index b578d00..62a51e8 100644 (file)
 #include <pmap.h>
 #include <schedule.h>
 
-/* Userspace could give us a vcoreid that causes us to compute a vcpd that is
- * outside procdata.  If we hit UWLIM, then we've gone farther than we should.
- * We check the vcoreid, instead of the resulting address, to avoid issues like
- * address wrap-around. */
-static bool vcoreid_is_safe(uint32_t vcoreid)
-{
-       /* MAX_NUM_VCORES == MAX_NUM_CORES (check procinfo/procdata) */
-       return vcoreid < MAX_NUM_CORES;
-}
-
 /* Note these three helpers return the user address of the mbox, not the KVA.
  * Load current to access this, and it will work for any process. */
 static struct event_mbox *get_vcpd_mbox_priv(uint32_t vcoreid)
@@ -403,7 +393,7 @@ void send_event(struct proc *p, struct event_queue *ev_q, struct event_msg *msg,
                vcoreid = (ev_q->ev_vcore + 1) % p->procinfo->num_vcores;
                ev_q->ev_vcore = vcoreid;
        }
-       if (!vcoreid_is_safe(vcoreid)) {
+       if (!proc_vcoreid_is_safe(p, vcoreid)) {
                /* Ought to kill them, just warn for now */
                printk("[kernel] Vcoreid %d unsafe! (too big?)\n", vcoreid);
                goto out;
@@ -487,6 +477,7 @@ void post_vcore_event(struct proc *p, struct event_msg *msg, uint32_t vcoreid,
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
        uintptr_t old_proc = switch_to(p);
 
+       assert(proc_vcoreid_is_safe(p, vcoreid));
        /* *ev_mbox is the user address of the vcpd mbox */
        post_vc_msg(p, vcoreid, get_vcpd_mbox(vcoreid, ev_flags), msg, ev_flags);
        switch_back(p, old_proc);
index 79cd19d..26072e7 100644 (file)
@@ -506,6 +506,11 @@ int mon_notify(int argc, char **argv, struct hw_trapframe *hw_tf)
        msg.ev_type = strtol(argv[2], 0, 0);
        if (argc == 4) {
                vcoreid = strtol(argv[3], 0, 0);
+               if (!proc_vcoreid_is_safe(p, vcoreid)) {
+                       printk("Bad vcoreid %d\n", vcoreid);
+                       proc_decref(p);
+                       return -1;
+               }
                /* This will go to the private mbox */
                post_vcore_event(p, &msg, vcoreid, EVENT_VCORE_PRIVATE);
                proc_notify(p, vcoreid);
index e47968b..3e4398d 100644 (file)
@@ -1352,6 +1352,7 @@ void proc_notify(struct proc *p, uint32_t vcoreid)
 {
        struct preempt_data *vcpd = &p->procdata->vcore_preempt_data[vcoreid];
 
+       assert(proc_vcoreid_is_safe(p, vcoreid));
        /* If you're thinking about checking notif_pending and then returning if
         * it is already set, note that some callers (e.g. the event system) set
         * notif_pending when they deliver a message, regardless of whether
@@ -2100,8 +2101,7 @@ int proc_change_to_vcore(struct proc *p, uint32_t new_vcoreid,
 
        /* Need to not reach outside the vcoremap, which might be smaller in the
         * future, but should always be as big as max_vcores */
-       if (new_vcoreid >= p->procinfo->max_vcores)
-               return -EINVAL;
+       assert(proc_vcoreid_is_safe(p, new_vcoreid));
        /* Need to lock to prevent concurrent vcore changes, like in yield. */
        spin_lock(&p->proc_lock);
        /* new_vcoreid is already runing, abort */
index 9099d2c..a7aba62 100644 (file)
@@ -865,8 +865,13 @@ static int sys_proc_yield(struct proc *p, bool being_nice)
 }
 
 static int sys_change_vcore(struct proc *p, uint32_t vcoreid,
-                             bool enable_my_notif)
+                            bool enable_my_notif)
 {
+       if (!proc_vcoreid_is_safe(p, vcoreid)) {
+               set_error(EINVAL, "vcoreid %d out of range %d", vcoreid,
+                         p->procinfo->max_vcores);
+               return -1;
+       }
        /* Note retvals can be negative, but we don't mess with errno in case
         * callers use this in low-level code and want to extract the 'errno'.
         */
@@ -1456,6 +1461,11 @@ static int sys_self_notify(struct proc *p, uint32_t vcoreid,
                       ev_type, u_msg, u_msg ? u_msg->ev_type : 0);
                return -1;
        }
+       if (!proc_vcoreid_is_safe(p, vcoreid)) {
+               set_error(EINVAL, "vcoreid %d out of range %d", vcoreid,
+                         p->procinfo->max_vcores);
+               return -1;
+       }
        /* this will post a message and IPI, regardless of
         * wants/needs/debutantes.*/
        post_vcore_event(p, &local_msg, vcoreid,
@@ -1477,6 +1487,11 @@ static int sys_send_event(struct proc *p, struct event_queue *ev_q,
                set_error(EINVAL, "bad event_queue %p", ev_q);
                return -1;
        }
+       if (!proc_vcoreid_is_safe(p, vcoreid)) {
+               set_error(EINVAL, "vcoreid %d out of range %d", vcoreid,
+                         p->procinfo->max_vcores);
+               return -1;
+       }
        send_event(p, ev_q, &local_msg, vcoreid);
        return 0;
 }