NetBSD Problem Report #46128

From www@NetBSD.org  Fri Mar  2 13:22:33 2012
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [149.20.53.66])
	by www.NetBSD.org (Postfix) with ESMTP id 0A8E063E1AC
	for <gnats-bugs@gnats.NetBSD.org>; Fri,  2 Mar 2012 13:22:33 +0000 (UTC)
Message-Id: <20120302132232.650F163B952@www.NetBSD.org>
Date: Fri,  2 Mar 2012 13:22:32 +0000 (UTC)
From: ad@netbsd.org
Reply-To: ad@netbsd.org
To: gnats-bugs@NetBSD.org
Subject: Use after free type problems in fork1()
X-Send-Pr-Version: www-1.0

>Number:         46128
>Category:       kern
>Synopsis:       Use after free type problems in fork1()
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kamil
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Fri Mar 02 13:25:00 +0000 2012
>Closed-Date:    Thu Jun 13 21:26:02 +0000 2019
>Last-Modified:  Thu Jun 13 21:26:02 +0000 2019
>Originator:     Andrew Doran
>Release:        -current
>Organization:
The NetBSD Project
>Environment:
N/A
>Description:
    563 		sched_enqueue(l2, false);
    564 		lwp_unlock(l2);
    565 	}
    566 	mutex_exit(p2->p_lock);
...
    573 	while (p2->p_lflag & PL_PPWAIT)
    574 		cv_wait(&p1->p_waitcv, proc_lock);

Once we have made at least 1 trip through this loop (proc_lock
released), p2 may no longer exist or may have a different identity.

Suggest something like the following:

- Garbage collect PL_PPWAIT.
- Replace with p2->p_vforklwp.  References l1 (parent LWP).
- Add LP_PPWAIT to be set in l1->l_prflag (parent, locked by p1->p_lock).

Sequence:

Parent: set p2->p_vforklwp, SET(l1->l_prflag, LP_PPWAIT)
Parent: cv_wait() while TEST(l1->l_prflag, LP_PPWAIT)
Child: RESET(p2->p_vforklwp->l_prflag, LP_PPWAIT)
Child: clear p2->p_vforklwp, cv_broadcast()

    590 	 * Return child pid to parent process,
    591 	 * marking us as parent via retval[1].
    592 	 */
    593 	if (retval != NULL) {
    594 		retval[0] = p2->p_pid;
    595 		retval[1] = 0;
    596 	}

As above, must not touch p2 here.  Fix: move this block up
to set retval before we relinquish control of child process.
>How-To-Repeat:
Code inspection.
>Fix:
As above.

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->rmind
Responsible-Changed-By: rmind@NetBSD.org
Responsible-Changed-When: Fri, 02 Mar 2012 19:24:31 +0000
Responsible-Changed-Why:
Take.


State-Changed-From-To: open->analyzed
State-Changed-By: rmind@NetBSD.org
State-Changed-When: Sun, 22 Jul 2012 22:42:00 +0000
State-Changed-Why:
Main fix committed, but there are clean ups to do..


From: "Mindaugas Rasiukevicius" <rmind@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46128 CVS commit: src/sys
Date: Sun, 22 Jul 2012 22:40:19 +0000

 Module Name:	src
 Committed By:	rmind
 Date:		Sun Jul 22 22:40:19 UTC 2012

 Modified Files:
 	src/sys/kern: kern_exec.c kern_exit.c kern_fork.c kern_lwp.c
 	src/sys/sys: lwp.h proc.h

 Log Message:
 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.


 To generate a diff of this commit:
 cvs rdiff -u -r1.352 -r1.353 src/sys/kern/kern_exec.c
 cvs rdiff -u -r1.238 -r1.239 src/sys/kern/kern_exit.c
 cvs rdiff -u -r1.189 -r1.190 src/sys/kern/kern_fork.c
 cvs rdiff -u -r1.170 -r1.171 src/sys/kern/kern_lwp.c
 cvs rdiff -u -r1.162 -r1.163 src/sys/sys/lwp.h
 cvs rdiff -u -r1.316 -r1.317 src/sys/sys/proc.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

Responsible-Changed-From-To: rmind->kamil
Responsible-Changed-By: kamil@NetBSD.org
Responsible-Changed-When: Thu, 19 Apr 2018 17:02:12 +0200
Responsible-Changed-Why:
Take, I'm working on it right now.


From: "Kamil Rytarowski" <kamil@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/46128 CVS commit: src/sys
Date: Thu, 13 Jun 2019 20:20:18 +0000

 Module Name:	src
 Committed By:	kamil
 Date:		Thu Jun 13 20:20:18 UTC 2019

 Modified Files:
 	src/sys/kern: kern_exec.c kern_exit.c kern_fork.c
 	src/sys/sys: lwp.h

 Log Message:
 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>
 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


 To generate a diff of this commit:
 cvs rdiff -u -r1.466 -r1.467 src/sys/kern/kern_exec.c
 cvs rdiff -u -r1.275 -r1.276 src/sys/kern/kern_exit.c
 cvs rdiff -u -r1.212 -r1.213 src/sys/kern/kern_fork.c
 cvs rdiff -u -r1.183 -r1.184 src/sys/sys/lwp.h

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->closed
State-Changed-By: kamil@NetBSD.org
State-Changed-When: Thu, 13 Jun 2019 23:26:02 +0200
State-Changed-Why:
Fix landed in 8.99.45.


>Unformatted:

Home	PR Database Search