Check safety of user pointer syscall arguments
authorBarret Rhoden <brho@cs.berkeley.edu>
Sat, 2 Mar 2019 00:43:19 +0000 (19:43 -0500)
committerBarret Rhoden <brho@cs.berkeley.edu>
Sat, 2 Mar 2019 00:50:19 +0000 (19:50 -0500)
Most arguments, such as a path name, are copied into the kernel.
Buffers used in read() and write() are passed deeper into the kernel
as-is.  Later on, the devices are supposed to check the pointers, often
doing a safe operation such as copy_from_user().

fs_file_write() was doing that, however the assertion at the end of the
loop was failing.  If buf + count wrapped around, we'd skip the loop
entirely and trigger a panic.

For safety's sake, we ought to just check the range early on.  The
is_user_r{,w}addr() checks can handle wraparound as well as making sure
the region is safe.

There were a few other syscalls that didn't have checks or didn't have
errstrs for the message.  This commit fixes them all.

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

index 394dbf7..2605055 100644 (file)
@@ -357,6 +357,9 @@ size_t fs_file_read(struct fs_file *f, uint8_t *buf, size_t count,
        const uint8_t *buf_end = buf + count;
        int error;
 
+       /* These errors should have been caught by higher level code */
+       if ((uintptr_t)buf + count < (uintptr_t)buf)
+               panic("Bad buf %p + count %p", buf, count);
        if (waserror()) {
                if (so_far) {
                        poperror();
@@ -403,6 +406,9 @@ size_t fs_file_write(struct fs_file *f, const uint8_t *buf, size_t count,
        const uint8_t *buf_end = buf + count;
        int error;
 
+       /* These errors should have been caught by higher level code */
+       if ((uintptr_t)buf + count < (uintptr_t)buf)
+               panic("Bad buf %p + count %p", buf, count);
        if (waserror()) {
                if (so_far) {
                        write_metadata(f, offset + so_far, false);
index b75dc66..1b9161a 100644 (file)
@@ -1575,6 +1575,14 @@ static int sys_vmm_add_gpcs(struct proc *p, unsigned int nr_more_gpcs,
        ERRSTACK(1);
        struct vmm *vmm = &p->vmm;
 
+       /* We do a copy_from_user in __vmm_add_gpcs, but it ought to be clear
+        * from the syscall.c code if we did our error checking. */
+       if (!is_user_rwaddr(gpcis, sizeof(struct vmm_gpcore_init) *
+                                  nr_more_gpcs)) {
+               set_error(EINVAL, "bad user addr %p + %p", gpcis,
+                         sizeof(struct vmm_gpcore_init) * nr_more_gpcs);
+               return -1;
+       }
        qlock(&vmm->qlock);
        if (waserror()) {
                qunlock(&vmm->qlock);
@@ -1694,12 +1702,22 @@ static unsigned long sys_populate_va(struct proc *p, uintptr_t va,
 
 static intreg_t sys_read(struct proc *p, int fd, void *buf, size_t len)
 {
+       if (!is_user_rwaddr(buf, len)) {
+               set_error(EINVAL, "bad user addr %p + %p", buf, len);
+               return -1;
+       }
        sysc_save_str("read on fd %d", fd);
        return sysread(fd, buf, len);
 }
 
 static intreg_t sys_write(struct proc *p, int fd, const void *buf, size_t len)
 {
+       /* We'll let this one include read-only areas, unlike most other
+        * syscalls that take bufs created and written by the user. */
+       if (!is_user_raddr(buf, len)) {
+               set_error(EINVAL, "bad user addr %p + %p", buf, len);
+               return -1;
+       }
        sysc_save_str("write on fd %d", fd);
        return syswrite(fd, (void*)buf, len);
 }
@@ -2234,10 +2252,11 @@ intreg_t sys_fd2path(struct proc *p, int fd, void *u_buf, size_t len)
        int ret = 0;
        struct chan *ch;
        ERRSTACK(1);
-       /* UMEM: Check the range, can PF later and kill if the page isn't present */
+
+       /* UMEM: Check the range, can PF later and kill if the page isn't
+        * present */
        if (!is_user_rwaddr(u_buf, len)) {
-               printk("[kernel] bad user addr %p (+%p) in %s (user bug)\n", u_buf,
-                      len, __FUNCTION__);
+               set_error(EINVAL, "bad user addr %p + %p", u_buf, len);
                return -1;
        }
        /* fdtochan throws */
@@ -2259,8 +2278,13 @@ intreg_t sys_wstat(struct proc *p, char *path, size_t path_l,
                    uint8_t *stat_m, size_t stat_sz, int flags)
 {
        int retval = 0;
-       char *t_path = copy_in_path(p, path, path_l);
+       char *t_path;
 
+       if (!is_user_rwaddr(stat_m, stat_sz)) {
+               set_error(EINVAL, "bad user addr %p + %p", stat_m, stat_sz);
+               return -1;
+       }
+       t_path = copy_in_path(p, path, path_l);
        if (!t_path)
                return -1;
        retval = syswstat(t_path, stat_m, stat_sz);
@@ -2271,6 +2295,10 @@ intreg_t sys_wstat(struct proc *p, char *path, size_t path_l,
 intreg_t sys_fwstat(struct proc *p, int fd, uint8_t *stat_m, size_t stat_sz,
                     int flags)
 {
+       if (!is_user_rwaddr(stat_m, stat_sz)) {
+               set_error(EINVAL, "bad user addr %p + %p", stat_m, stat_sz);
+               return -1;
+       }
        return sysfwstat(fd, stat_m, stat_sz);
 }
 
@@ -2298,7 +2326,8 @@ static intreg_t sys_dup_fds_to(struct proc *p, unsigned int pid,
        int slot;
 
        if (!is_user_rwaddr(map, sizeof(struct childfdmap) * nentries)) {
-               set_errno(EINVAL);
+               set_error(EINVAL, "bad user addr %p + %p", map,
+                         sizeof(struct childfdmap) * nentries);
                return -1;
        }
        child = get_controllable_proc(p, pid);
@@ -2340,8 +2369,10 @@ static intreg_t sys_tap_fds(struct proc *p, struct fd_tap_req *tap_reqs,
 {
        struct fd_tap_req *req_i = tap_reqs;
        int done;
+
        if (!is_user_rwaddr(tap_reqs, sizeof(struct fd_tap_req) * nr_reqs)) {
-               set_errno(EINVAL);
+               set_error(EINVAL, "bad user addr %p + %p", tap_reqs,
+                         sizeof(struct fd_tap_req) * nr_reqs);
                return 0;
        }
        for (done = 0; done < nr_reqs; done++, req_i++) {