From owner-freebsd-hackers@FreeBSD.ORG Sun Oct 5 10:29:18 2014 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 34445E5; Sun, 5 Oct 2014 10:29:18 +0000 (UTC) Received: from mail-wg0-x230.google.com (mail-wg0-x230.google.com [IPv6:2a00:1450:400c:c00::230]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 9DA58305; Sun, 5 Oct 2014 10:29:17 +0000 (UTC) Received: by mail-wg0-f48.google.com with SMTP id k14so3211209wgh.7 for ; Sun, 05 Oct 2014 03:29:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:mail-followup-to:mime-version :content-type:content-disposition:user-agent; bh=lazS5fpj/Sgv5tEq7vZ4HkSvpw9wqQfXOUmKmCo1DhI=; b=aqF+nzscbe0nleFwrxBcAG09eWeMbjPeKhrSbtTglPednGzrjcH+6aW1rK2vMbWgWg o+JWUwZIgky1sQCBy1VoqWSTRTQqxViF5oowkYVldfxq6C5IWTGKdpU/euCN0qN/g6a/ JF4mkKLjF+WulI0XWKv9Oz5ryU+6KgKc66Ayblth4vAcECbum55U+0f70nS2Lf70gPGb 9v39gIsva357tNJpG5WLZrTZh8vwkJb8e0zvOUCcEtd8EHk2RIDtKvlYpubD6KyYCFtW yABaM/Ful4Yw/ee51rjYmmlHkb7pX7sOMnKTRihzy4wcQugyEjZ+WfKqp15oUOCnTEZg Fyjw== X-Received: by 10.180.39.44 with SMTP id m12mr11676084wik.25.1412504955927; Sun, 05 Oct 2014 03:29:15 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id n5sm7581591wix.0.2014.10.05.03.29.14 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sun, 05 Oct 2014 03:29:15 -0700 (PDT) Date: Sun, 5 Oct 2014 12:29:12 +0200 From: Mateusz Guzik To: freebsd-hackers@freebsd.org Subject: fork: hold newly created processes Message-ID: <20141005102912.GB9262@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , freebsd-hackers@freebsd.org, kib@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Cc: kib@freebsd.org X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 05 Oct 2014 10:29:18 -0000 fork: hold newly created processes Consumers of fork1 -> do_fork receive new proc pointer, but nothing guarnatees its stability at that time. New process could already exit and be waited for, in which case we get a use after free. This is a temporary fix. diff --git a/sys/compat/linux/linux_fork.c b/sys/compat/linux/linux_fork.c index 316cf2a..8ad33f8 100644 --- a/sys/compat/linux/linux_fork.c +++ b/sys/compat/linux/linux_fork.c @@ -95,6 +95,8 @@ linux_fork(struct thread *td, struct linux_fork_args *args) sched_add(td2, SRQ_BORING); thread_unlock(td2); + PRELE(p2); + return (0); } @@ -139,6 +141,7 @@ linux_vfork(struct thread *td, struct linux_vfork_args *args) PROC_LOCK(p2); while (p2->p_flag & P_PPWAIT) cv_wait(&p2->p_pwait, &p2->p_mtx); + _PRELE(p2); PROC_UNLOCK(p2); return (0); @@ -287,13 +290,14 @@ linux_clone(struct thread *td, struct linux_clone_args *args) td->td_retval[0] = p2->p_pid; td->td_retval[1] = 0; + PROC_LOCK(p2); if (args->flags & LINUX_CLONE_VFORK) { /* wait for the children to exit, ie. emulate vfork */ - PROC_LOCK(p2); while (p2->p_flag & P_PPWAIT) cv_wait(&p2->p_pwait, &p2->p_mtx); - PROC_UNLOCK(p2); } + _PRELE(p2); + PROC_UNLOCK(p2); return (0); } diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c index 141d438..af905a5 100644 --- a/sys/kern/init_main.c +++ b/sys/kern/init_main.c @@ -832,6 +832,7 @@ create_init(const void *udata __unused) audit_cred_proc1(newcred); #endif initproc->p_ucred = newcred; + _PRELE(initproc); PROC_UNLOCK(initproc); crfree(oldcred); cred_update_thread(FIRST_THREAD_IN_PROC(initproc)); diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index 8135afb..596a28c 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -107,6 +107,7 @@ sys_fork(struct thread *td, struct fork_args *uap) if (error == 0) { td->td_retval[0] = p2->p_pid; td->td_retval[1] = 0; + PRELE(p2); } return (error); } @@ -130,6 +131,7 @@ sys_pdfork(td, uap) if (error == 0) { td->td_retval[0] = p2->p_pid; td->td_retval[1] = 0; + PRELE(p2); error = copyout(&fd, uap->fdp, sizeof(fd)); } return (error); @@ -147,6 +149,7 @@ sys_vfork(struct thread *td, struct vfork_args *uap) if (error == 0) { td->td_retval[0] = p2->p_pid; td->td_retval[1] = 0; + PRELE(p2); } return (error); } @@ -166,6 +169,8 @@ sys_rfork(struct thread *td, struct rfork_args *uap) if (error == 0) { td->td_retval[0] = p2 ? p2->p_pid : 0; td->td_retval[1] = 0; + if (p2 != NULL) + PRELE(p2); } return (error); } @@ -359,7 +364,7 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, struct vmspace *vm2, int pdflags) { struct proc *p1, *pptr; - int p2_held, trypid; + int trypid; struct filedesc *fd; struct filedesc_to_leader *fdtol; struct sigacts *newsigacts; @@ -367,7 +372,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, sx_assert(&proctree_lock, SX_SLOCKED); sx_assert(&allproc_lock, SX_XLOCKED); - p2_held = 0; p1 = td->td_proc; /* @@ -674,6 +678,12 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, p2->p_state = PRS_NORMAL; PROC_SUNLOCK(p2); + /* + * We return p2 pointer to the caller. Hold it so that it is + * guaranteed to remain valid. + */ + _PHOLD(p2); + #ifdef KDTRACE_HOOKS /* * Tell the DTrace fasttrap provider about the new process so that any @@ -696,8 +706,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, td->td_dbgflags |= TDB_FORK; td->td_dbg_forked = p2->p_pid; td2->td_dbgflags |= TDB_STOPATFORK; - _PHOLD(p2); - p2_held = 1; } if (flags & RFPPWAIT) { td->td_pflags |= TDP_RFPPWAIT; @@ -733,8 +741,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, PROC_LOCK(p2); while ((td2->td_dbgflags & TDB_STOPATFORK) != 0) cv_wait(&p2->p_dbgwait, &p2->p_mtx); - if (p2_held) - _PRELE(p2); PROC_UNLOCK(p2); } diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c index 969c513..bb0e232 100644 --- a/sys/kern/kern_kthread.c +++ b/sys/kern/kern_kthread.c @@ -134,6 +134,8 @@ kproc_create(void (*func)(void *), void *arg, sched_add(td, SRQ_BORING); thread_unlock(td); + PRELE(p2); + return 0; }