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:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.