Unlock the pledge(2) system call

Protect the ps_pledge/ps_execpledge fields with ps_mtx.  Shuffle the
code to call unveil_destroy() outside the critical section.  Only writes
to those fields are protected.  Since we may only remove bits from those
fields, garbage values should do no harm even when a read crosses
a write on 32 bits systems.

Input claudio@ kettenis@ deraadt@, ok deraadt@

diff --git a/sys/kern/init_sysent.c b/sys/kern/init_sysent.c
index 0b8b0fd..b4f6c81 100644 (file)
--- a/sys/kern/init_sysent.c
+++ b/sys/kern/init_sysent.c
@@ -1,10 +1,10 @@
-/*     $OpenBSD: init_sysent.c,v 1.238 2022/06/27 14:26:05 cheloha Exp $       */
+/*     $OpenBSD: init_sysent.c,v 1.239 2022/06/29 12:17:31 jca Exp $   */
 
 /*
  * System call switch table.
  *
  * DO NOT EDIT-- this file is automatically generated.
- * created from;       OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04 mvs Exp 
+ * created from;       OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05 cheloha Exp 
  */
 
 #include <sys/param.h>
@@ -248,7 +248,7 @@ const struct sysent sysent[] = {
            sys_listen },                       /* 106 = listen */
        { 4, s(struct sys_chflagsat_args), 0,
            sys_chflagsat },                    /* 107 = chflagsat */
-       { 2, s(struct sys_pledge_args), 0,
+       { 2, s(struct sys_pledge_args), SY_NOLOCK | 0,
            sys_pledge },                       /* 108 = pledge */
        { 4, s(struct sys_ppoll_args), 0,
            sys_ppoll },                        /* 109 = ppoll */

diff --git a/sys/kern/kern_pledge.c b/sys/kern/kern_pledge.c
index 7f55410..c09d534 100644 (file)
--- a/sys/kern/kern_pledge.c
+++ b/sys/kern/kern_pledge.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: kern_pledge.c,v 1.283 2022/06/29 12:01:22 jca Exp $   */
+/*     $OpenBSD: kern_pledge.c,v 1.284 2022/06/29 12:17:31 jca Exp $   */
 
 /*
  * Copyright (c) 2015 Nicholas Marriott <nicm@openbsd.org>
@@ -21,6 +21,7 @@
 
 #include <sys/mount.h>
 #include <sys/proc.h>
+#include <sys/mutex.h>
 #include <sys/fcntl.h>
 #include <sys/file.h>
 #include <sys/filedesc.h>
@@ -465,13 +466,26 @@ sys_pledge(struct proc *p, void *v, register_t *retval)
        struct process *pr = p->p_p;
        uint64_t promises, execpromises;
        int error;
+       int unveil_cleanup = 0;
 
+       /* Check for any error in user input */
        if (SCARG(uap, promises)) {
                error = parsepledges(p, "pledgereq",
                    SCARG(uap, promises), &promises);
                if (error)
                        return (error);
+       }
+       if (SCARG(uap, execpromises)) {
+               error = parsepledges(p, "pledgeexecreq",
+                   SCARG(uap, execpromises), &execpromises);
+               if (error)
+                       return (error);
+       }
+
+       mtx_enter(&pr->ps_mtx);
 
+       /* Check for any error wrt current promises */
+       if (SCARG(uap, promises)) {
                /* In "error" mode, ignore promise increase requests,
                 * but accept promise decrease requests */
                if (ISSET(pr->ps_flags, PS_PLEDGE) &&
@@ -480,37 +494,47 @@ sys_pledge(struct proc *p, void *v, register_t *retval)
 
                /* Only permit reductions */
                if (ISSET(pr->ps_flags, PS_PLEDGE) &&
-                   (((promises | pr->ps_pledge) != pr->ps_pledge)))
+                   (((promises | pr->ps_pledge) != pr->ps_pledge))) {
+                       mtx_leave(&pr->ps_mtx);
                        return (EPERM);
+               }
        }
        if (SCARG(uap, execpromises)) {
-               error = parsepledges(p, "pledgeexecreq",
-                   SCARG(uap, execpromises), &execpromises);
-               if (error)
-                       return (error);
-
                /* Only permit reductions */
                if (ISSET(pr->ps_flags, PS_EXECPLEDGE) &&
-                   (((execpromises | pr->ps_execpledge) != pr->ps_execpledge)))
+                   (((execpromises | pr->ps_execpledge) != pr->ps_execpledge))) {
+                       mtx_leave(&pr->ps_mtx);
                        return (EPERM);
+               }
        }
 
+       /* Set up promises */
        if (SCARG(uap, promises)) {
                pr->ps_pledge = promises;
                atomic_setbits_int(&pr->ps_flags, PS_PLEDGE);
-               /*
-                * Kill off unveil and drop unveil vnode refs if we no
-                * longer are holding any path-accessing pledge
-                */
+
                if ((pr->ps_pledge & (PLEDGE_RPATH | PLEDGE_WPATH |
                    PLEDGE_CPATH | PLEDGE_DPATH | PLEDGE_TMPPATH | PLEDGE_EXEC |
                    PLEDGE_UNIX | PLEDGE_UNVEIL)) == 0)
-                       unveil_destroy(pr);
+                       unveil_cleanup = 1;
        }
        if (SCARG(uap, execpromises)) {
                pr->ps_execpledge = execpromises;
                atomic_setbits_int(&pr->ps_flags, PS_EXECPLEDGE);
        }
+
+       mtx_leave(&pr->ps_mtx);
+
+       if (unveil_cleanup) {
+               /*
+                * Kill off unveil and drop unveil vnode refs if we no
+                * longer are holding any path-accessing pledge
+                */
+               KERNEL_LOCK();
+               unveil_destroy(pr);
+               KERNEL_UNLOCK();
+       }
+
        return (0);
 }
 
@@ -704,11 +728,11 @@ pledge_namei(struct proc *p, struct nameidata *ni, char *origpath)
                                 * XXX
                                 * The current hack for YP support in "getpw"
                                 * is to enable some "inet" features until
-                                * next pledge call.  Setting a bit in ps_pledge
-                                * is not safe with respect to multiple threads,
-                                * a very different approach is needed.
+                                * next pledge call.
                                 */
+                               mtx_enter(&p->p_p->ps_mtx);
                                p->p_p->ps_pledge |= PLEDGE_YPACTIVE;
+                               mtx_leave(&p->p_p->ps_mtx);
                                ni->ni_cnd.cn_flags |= BYPASSUNVEIL;
                                return (0);
                        }

diff --git a/sys/kern/syscalls.c b/sys/kern/syscalls.c
index 5f401c6..1ed2a68 100644 (file)
--- a/sys/kern/syscalls.c
+++ b/sys/kern/syscalls.c
@@ -1,10 +1,10 @@
-/*     $OpenBSD: syscalls.c,v 1.236 2022/05/16 07:38:10 mvs Exp $      */
+/*     $OpenBSD: syscalls.c,v 1.237 2022/06/29 12:17:31 jca Exp $      */
 
 /*
  * System call names.
  *
  * DO NOT EDIT-- this file is automatically generated.
- * created from;       OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04 mvs Exp 
+ * created from;       OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05 cheloha Exp 
  */
 
 const char *const syscallnames[] = {

diff --git a/sys/kern/syscalls.master b/sys/kern/syscalls.master
index b02d109..ae187f3 100644 (file)
--- a/sys/kern/syscalls.master
+++ b/sys/kern/syscalls.master
@@ -1,4 +1,4 @@
-;      $OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05 cheloha Exp $
+;      $OpenBSD: syscalls.master,v 1.226 2022/06/29 12:17:31 jca Exp $
 ;      $NetBSD: syscalls.master,v 1.32 1996/04/23 10:24:21 mycroft Exp $
 
 ;      @(#)syscalls.master     8.2 (Berkeley) 1/13/94
@@ -228,7 +228,7 @@
 106    STD             { int sys_listen(int s, int backlog); }
 107    STD             { int sys_chflagsat(int fd, const char *path, \
                            u_int flags, int atflags); }
-108    STD             { int sys_pledge(const char *promises, \
+108    STD NOLOCK      { int sys_pledge(const char *promises, \
                            const char *execpromises); }
 109    STD             { int sys_ppoll(struct pollfd *fds, \
                            u_int nfds, const struct timespec *ts, \

diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index a36fbfc..c5e9a22 100644 (file)
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: proc.h,v 1.331 2022/06/27 14:26:05 cheloha Exp $      */
+/*     $OpenBSD: proc.h,v 1.332 2022/06/29 12:17:31 jca Exp $  */
 /*     $NetBSD: proc.h,v 1.44 1996/04/22 01:23:21 christos Exp $       */
 
 /*-
@@ -231,8 +231,8 @@ struct process {
 
        u_int32_t       ps_acflag;      /* Accounting flags. */
 
-       uint64_t ps_pledge;
-       uint64_t ps_execpledge;
+       uint64_t ps_pledge;             /* [m] pledge promises */
+       uint64_t ps_execpledge;         /* [m] execpledge promises */
 
        int64_t ps_kbind_cookie;        /* [m] */
        u_long  ps_kbind_addr;          /* [m] */

diff --git a/sys/sys/syscall.h b/sys/sys/syscall.h
index e8dc9d8..2643306 100644 (file)
--- a/sys/sys/syscall.h
+++ b/sys/sys/syscall.h
@@ -1,10 +1,10 @@
-/*     $OpenBSD: syscall.h,v 1.235 2022/06/27 14:26:06 cheloha Exp $   */
+/*     $OpenBSD: syscall.h,v 1.236 2022/06/29 12:17:31 jca Exp $       */
 
 /*
  * System call numbers.
  *
  * DO NOT EDIT-- this file is automatically generated.
- * created from;       OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04 mvs Exp 
+ * created from;       OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05 cheloha Exp 
  */
 
 /* syscall: "syscall" ret: "int" args: "int" "..." */

diff --git a/sys/sys/syscallargs.h b/sys/sys/syscallargs.h
index daa303d..4ece1b1 100644 (file)
--- a/sys/sys/syscallargs.h
+++ b/sys/sys/syscallargs.h
@@ -1,10 +1,10 @@
-/*     $OpenBSD: syscallargs.h,v 1.238 2022/06/27 14:26:06 cheloha Exp $       */
+/*     $OpenBSD: syscallargs.h,v 1.239 2022/06/29 12:17:31 jca Exp $   */
 
 /*
  * System call argument lists.
  *
  * DO NOT EDIT-- this file is automatically generated.
- * created from;       OpenBSD: syscalls.master,v 1.224 2022/05/16 07:36:04 mvs Exp 
+ * created from;       OpenBSD: syscalls.master,v 1.225 2022/06/27 14:26:05 cheloha Exp 
  */
 
 #ifdef syscallarg