[src/trunk]: src/sys Correct use-after-free issue in vfork(2)

[kamil <kamil%NetBSD.org@localhost>]

details:   https://anonhg.NetBSD.org/src/rev/807ab0f2e3da
branches:  trunk
changeset: 451985:807ab0f2e3da
user:      kamil <kamil%NetBSD.org@localhost>
date:      Thu Jun 13 20:20:18 2019 +0000

description:
Correct use-after-free issue in vfork(2)

In the previous behavior vforking parent was keeping pointer to a child
and checking whether it clears a PL_PPWAIT in its bitfield p_lflag. However
a child can go invalid between exec/exit event from child and waking up
vforked parent and this can cause invalid pointer read and in the worst
scenario kernel crash.

In the new behavior vforked child keeps a reference to vforked parent LWP
and sets a value l_vforkwaiting to false. This means that vforked child
can finish its work, exec/exit and be terminated and once parent will be
woken up it will read its own field whether its child is still blocking.

Add new field in struct lwp: l_vforkwaiting protected by proc_lock.
In future it should be refactored and all PL_PPWAIT users transformed to
l_vforkwaiting and next l_vforkwaiting probably transformed into a bit
field.

This is another attempt of fixing this bug after <rmind> from 2012 in
commit:

Author: rmind <rmind%NetBSD.org@localhost>
Date:   Sun Jul 22 22:40:18 2012 +0000

    fork1: fix use-after-free problems.  Addresses PR/46128 from Andrew Doran.
    Note: PL_PPWAIT should be fully replaced and modificaiton of l_pflag by
    other LWP is undesirable, but this is enough for netbsd-6.

The new version no longer performs unsafe access in l_lflag changing the
LP_VFORKWAIT bit.

Verified with ATF t_vfork and t_ptrace* tests and they are no longer
causing any issues in my local setup.

Fixes PR/46128 by Andrew Doran

diffstat:

 sys/kern/kern_exec.c |  13 ++++++++++---
 sys/kern/kern_exit.c |  12 +++++++++---
 sys/kern/kern_fork.c |  13 +++++++------
 sys/sys/lwp.h        |   4 ++--
 4 files changed, 28 insertions(+), 14 deletions(-)

diffs (143 lines):

diff -r 4dfabf645dca -r 807ab0f2e3da sys/kern/kern_exec.c
--- a/sys/kern/kern_exec.c      Thu Jun 13 19:37:36 2019 +0000
+++ b/sys/kern/kern_exec.c      Thu Jun 13 20:20:18 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_exec.c,v 1.466 2019/06/11 23:18:55 kamil Exp $    */
+/*     $NetBSD: kern_exec.c,v 1.467 2019/06/13 20:20:18 kamil Exp $    */
 
 /*-
  * Copyright (c) 2008 The NetBSD Foundation, Inc.
@@ -59,7 +59,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.466 2019/06/11 23:18:55 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_exec.c,v 1.467 2019/06/13 20:20:18 kamil Exp $");
 
 #include "opt_exec.h"
 #include "opt_execfmt.h"
@@ -1207,10 +1207,17 @@
         * exited and exec()/exit() are the only places it will be cleared.
         */
        if ((p->p_lflag & PL_PPWAIT) != 0) {
+               lwp_t *lp;
+
                mutex_enter(proc_lock);
+               lp = p->p_vforklwp;
+               p->p_vforklwp = NULL;
+
                l->l_lwpctl = NULL; /* was on loan from blocked parent */
                p->p_lflag &= ~PL_PPWAIT;
-               cv_broadcast(&p->p_pptr->p_waitcv);
+               lp->l_vforkwaiting = false;
+
+               cv_broadcast(&lp->l_waitcv);
                mutex_exit(proc_lock);
        }
 
diff -r 4dfabf645dca -r 807ab0f2e3da sys/kern/kern_exit.c
--- a/sys/kern/kern_exit.c      Thu Jun 13 19:37:36 2019 +0000
+++ b/sys/kern/kern_exit.c      Thu Jun 13 20:20:18 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_exit.c,v 1.275 2019/05/17 03:34:26 ozaki-r Exp $  */
+/*     $NetBSD: kern_exit.c,v 1.276 2019/06/13 20:20:18 kamil Exp $    */
 
 /*-
  * Copyright (c) 1998, 1999, 2006, 2007, 2008 The NetBSD Foundation, Inc.
@@ -67,7 +67,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_exit.c,v 1.275 2019/05/17 03:34:26 ozaki-r Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_exit.c,v 1.276 2019/06/13 20:20:18 kamil Exp $");
 
 #include "opt_ktrace.h"
 #include "opt_dtrace.h"
@@ -342,9 +342,15 @@
         */
        mutex_enter(proc_lock);
        if (p->p_lflag & PL_PPWAIT) {
+               lwp_t *lp;
+
                l->l_lwpctl = NULL; /* was on loan from blocked parent */
                p->p_lflag &= ~PL_PPWAIT;
-               cv_broadcast(&p->p_pptr->p_waitcv);
+
+               lp = p->p_vforklwp;
+               p->p_vforklwp = NULL;
+               lp->l_vforkwaiting = false;
+               cv_broadcast(&lp->l_waitcv);
        }
 
        if (SESS_LEADER(p)) {
diff -r 4dfabf645dca -r 807ab0f2e3da sys/kern/kern_fork.c
--- a/sys/kern/kern_fork.c      Thu Jun 13 19:37:36 2019 +0000
+++ b/sys/kern/kern_fork.c      Thu Jun 13 20:20:18 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: kern_fork.c,v 1.212 2019/05/03 22:34:21 kamil Exp $    */
+/*     $NetBSD: kern_fork.c,v 1.213 2019/06/13 20:20:18 kamil Exp $    */
 
 /*-
  * Copyright (c) 1999, 2001, 2004, 2006, 2007, 2008 The NetBSD Foundation, Inc.
@@ -67,7 +67,7 @@
  */
 
 #include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_fork.c,v 1.212 2019/05/03 22:34:21 kamil Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_fork.c,v 1.213 2019/06/13 20:20:18 kamil Exp $");
 
 #include "opt_ktrace.h"
 #include "opt_dtrace.h"
@@ -413,11 +413,12 @@
 
        if (flags & FORK_PPWAIT) {
                /* Mark ourselves as waiting for a child. */
-               l1->l_pflag |= LP_VFORKWAIT;
                p2->p_lflag = PL_PPWAIT;
+               l1->l_vforkwaiting = true;
                p2->p_vforklwp = l1;
        } else {
                p2->p_lflag = 0;
+               l1->l_vforkwaiting = false;
        }
        p2->p_sflag = 0;
        p2->p_slflag = 0;
@@ -610,10 +611,10 @@
 
        /*
         * Preserve synchronization semantics of vfork.  If waiting for
-        * child to exec or exit, sleep until it clears LP_VFORKWAIT.
+        * child to exec or exit, sleep until it clears p_vforkwaiting.
         */
-       while (p2->p_lflag & PL_PPWAIT) // XXX: p2 can go invalid
-               cv_wait(&p1->p_waitcv, proc_lock);
+       while (l1->l_vforkwaiting)
+               cv_wait(&l1->l_waitcv, proc_lock);
 
        /*
         * Let the parent know that we are tracing its child.
diff -r 4dfabf645dca -r 807ab0f2e3da sys/sys/lwp.h
--- a/sys/sys/lwp.h     Thu Jun 13 19:37:36 2019 +0000
+++ b/sys/sys/lwp.h     Thu Jun 13 20:20:18 2019 +0000
@@ -1,4 +1,4 @@
-/*     $NetBSD: lwp.h,v 1.183 2019/05/17 03:34:26 ozaki-r Exp $        */
+/*     $NetBSD: lwp.h,v 1.184 2019/06/13 20:20:18 kamil Exp $  */
 
 /*
  * Copyright (c) 2001, 2006, 2007, 2008, 2009, 2010
@@ -132,6 +132,7 @@
        callout_t       l_timeout_ch;   /* !: callout for tsleep */
        u_int           l_emap_gen;     /* !: emap generation number */
        kcondvar_t      l_waitcv;       /* a: vfork() wait */
+       bool            l_vforkwaiting; /* a: vfork() waiting */
 
 #if PCU_UNIT_COUNT > 0
        struct cpu_info * volatile l_pcu_cpu[PCU_UNIT_COUNT];
@@ -256,7 +257,6 @@
 #define        LP_INTR         0x00000040 /* Soft interrupt handler */
 #define        LP_SYSCTLWRITE  0x00000080 /* sysctl write lock held */
 #define        LP_MUSTJOIN     0x00000100 /* Must join kthread on exit */
-#define        LP_VFORKWAIT    0x00000200 /* Waiting at vfork() for a child */
 #define        LP_SINGLESTEP   0x00000400 /* Single step thread in ptrace(2) */
 #define        LP_TIMEINTR     0x00010000 /* Time this soft interrupt */
 #define        LP_PREEMPTING   0x00020000 /* mi_switch called involuntarily */