kernel - Fix rare allproc scan vs p_ucred race
authorMatthew Dillon <dillon@apollo.backplane.com>
Wed, 27 Sep 2017 04:31:04 +0000 (21:31 -0700)
committerMatthew Dillon <dillon@apollo.backplane.com>
Wed, 27 Sep 2017 04:34:52 +0000 (21:34 -0700)
* This race can occur because p->p_ucred can change out from under
  an allproc scan when the allproc scan is filtering based on
  credentials.

* Access p->p_ucred via the per-process spinlock (p->p_spin).  Also
  maintain a  cache of the last ucred during the loop in order to
  avoid having to spin-lock every process.

* Add missing spinlock around p->p_ucred = NULL in exit1().  This
  is also only applicable to races against allproc scans since
  p_token is held during exit1().

Reported-by: mjg_
sys/kern/kern_exit.c
sys/kern/kern_proc.c
sys/sys/proc.h

index 0b90496..08a2589 100644 (file)
@@ -73,6 +73,7 @@
 #include <sys/refcount.h>
 #include <sys/thread2.h>
 #include <sys/sysref2.h>
+#include <sys/spinlock2.h>
 #include <sys/mplock2.h>
 
 #include <machine/vmm.h>
@@ -892,6 +893,7 @@ kern_wait(pid_t pid, int *status, int options, struct rusage *rusage, int *res)
        struct lwp *lp;
        struct proc *q = td->td_proc;
        struct proc *p, *t;
+       struct ucred *cr;
        struct pargs *pa;
        struct sigacts *ps;
        int nfound, error;
@@ -1066,10 +1068,14 @@ loop:
                        chgproccnt(p->p_ucred->cr_ruidinfo, -1, 0);
 
                        /*
-                        * Free up credentials.
+                        * Free up credentials.  p_spin is required to
+                        * avoid races against allproc scans.
                         */
-                       crfree(p->p_ucred);
+                       spin_lock(&p->p_spin);
+                       cr = p->p_ucred;
                        p->p_ucred = NULL;
+                       spin_unlock(&p->p_spin);
+                       crfree(cr);
 
                        /*
                         * Remove unused arguments
index e6f876d..89be949 100644 (file)
@@ -141,6 +141,21 @@ static uint8_t *pid_doms;
  */
 static int randompid = 0;
 
+static __inline
+struct ucred *
+pcredcache(struct ucred *cr, struct proc *p)
+{
+       if (cr != p->p_ucred) {
+               if (cr)
+                       crfree(cr);
+               spin_lock(&p->p_spin);
+               if ((cr = p->p_ucred) != NULL)
+                       crhold(cr);
+               spin_unlock(&p->p_spin);
+       }
+       return cr;
+}
+
 /*
  * No requirements.
  */
@@ -1511,6 +1526,7 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
        int n;
        int origcpu;
        struct ucred *cr1 = curproc->p_ucred;
+       struct ucred *crcache = NULL;
 
        flags = oid & KERN_PROC_FLAGMASK;
        oid &= ~KERN_PROC_FLAGMASK;
@@ -1528,7 +1544,8 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
        if (oid == KERN_PROC_PID) {
                p = pfind((pid_t)name[0]);
                if (p) {
-                       if (PRISON_CHECK(cr1, p->p_ucred))
+                       crcache = pcredcache(crcache, p);
+                       if (PRISON_CHECK(cr1, crcache))
                                error = sysctl_out_proc(p, req, flags);
                        PRELE(p);
                }
@@ -1553,10 +1570,14 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
                        /*
                         * Show a user only their processes.
                         */
-                       if ((!ps_showallprocs) &&
-                               (p->p_ucred == NULL || p_trespass(cr1, p->p_ucred))) {
-                               continue;
+                       if (ps_showallprocs == 0) {
+                               crcache = pcredcache(crcache, p);
+                               if (crcache == NULL ||
+                                   p_trespass(cr1, crcache)) {
+                                       continue;
+                               }
                        }
+
                        /*
                         * Skip embryonic processes.
                         */
@@ -1584,19 +1605,24 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
                                break;
 
                        case KERN_PROC_UID:
-                               if (p->p_ucred == NULL || 
-                                   p->p_ucred->cr_uid != (uid_t)name[0])
+                               crcache = pcredcache(crcache, p);
+                               if (crcache == NULL ||
+                                   crcache->cr_uid != (uid_t)name[0]) {
                                        continue;
+                               }
                                break;
 
                        case KERN_PROC_RUID:
-                               if (p->p_ucred == NULL || 
-                                   p->p_ucred->cr_ruid != (uid_t)name[0])
+                               crcache = pcredcache(crcache, p);
+                               if (crcache == NULL ||
+                                   crcache->cr_ruid != (uid_t)name[0]) {
                                        continue;
+                               }
                                break;
                        }
 
-                       if (!PRISON_CHECK(cr1, p->p_ucred))
+                       crcache = pcredcache(crcache, p);
+                       if (!PRISON_CHECK(cr1, crcache))
                                continue;
                        PHOLD(p);
                        error = sysctl_out_proc(p, req, flags);
@@ -1680,6 +1706,8 @@ sysctl_kern_proc(SYSCTL_HANDLER_ARGS)
        kfree(marker, M_TEMP);
 
 post_threads:
+       if (crcache)
+               crfree(crcache);
        return (error);
 }
 
index 7955dab..50ec436 100644 (file)
@@ -464,8 +464,11 @@ MALLOC_DECLARE(M_PARGS);
 /* for priv_check_cred() */
 #define        NULL_CRED_OKAY  0x2
 
-/* Handy macro to determine if p1 can mangle p2 */
-
+/*
+ * Handy macro to determine if p1 can mangle p2.
+ *
+ * Must be spin-safe.
+ */
 #define PRISON_CHECK(cr1, cr2) \
        ((!(cr1)->cr_prison) || (cr1)->cr_prison == (cr2)->cr_prison)