From owner-freebsd-hackers@freebsd.org Tue Feb 2 04:07:55 2016 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 91F4EA978E4 for ; Tue, 2 Feb 2016 04:07:55 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x243.google.com (mail-wm0-x243.google.com [IPv6:2a00:1450:400c:c09::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 1E2E51C4E; Tue, 2 Feb 2016 04:07:55 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x243.google.com with SMTP id 128so483448wmz.3; Mon, 01 Feb 2016 20:07:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=yLFxl4l7A+oYW5XPuv16uObuhE/RhPfCOX87b7d2MeQ=; b=waUJGWkNG2NRmCJbdRFuGSxNFvNIVVAmSOVOV4JVW+V094R4EnsELpfniwaA+K+qyW K+kjRbZWMhh4Xl+OopBn+Nsd/hg5GFyZTeQmvM/YzmkRHW1DH0DhpYhIVmS+8ZnFb8ll R7C6m1BqWsEPZ503Q8qtOAyOwI4Ycn2xhoX+5EZtOhWRglbhaXrqDr6RMli/wloreNDY V9FpUTjP+jBi4xtWCjPDJeab0cW0+0FwPN/vC/gNbhAZ9LRHG1dnvCrpm+h4K1sevQ+H bm3dXgzsZPTkaDiD4n4u7ARPk83qvPNxgw5aVC0mH8pRB21TWdRWXZJGUpiZ/Dk/oXVm K7ZQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=yLFxl4l7A+oYW5XPuv16uObuhE/RhPfCOX87b7d2MeQ=; b=YtRVjzfIip8Rn6qLIsY5DY7Bb/o3OIPTU6AHb3J1b4ItgPblgRbe4TpOTBXJK89QaP 1lcFO5+6GR+1FUGTnqec6WesJzJ00jjqZJ9Qx44fdyQ4TsDsmGtqFSaIGv4q3SSnL3yj A6bmfJEcC2m3Q/TfU5Rcs8PK71bccmEN/Bv4/EpxyyMiYBxfhhnHdTBNgmKbjDFQAfty DtBniuRcQvfjl2FSwU1LtJCx5DrPPQY4raO4MVPylpdaoAo0fjycPcCZjNiWE/G2XfA+ whEfXoZzZfiApYTFJDD4K63FiM/TgXZM3upGPaLHkiLVfe2KxWFt2Q1V4wrHk/JUxa6p 8Skg== X-Gm-Message-State: AG10YOS0rLUot1N9jOgdWFqdubZvCxuU6xPkPJwqz1aqkKtMynJrcSymfKkEpW9Xw2H/bg== X-Received: by 10.28.63.200 with SMTP id m191mr3082380wma.21.1454386073421; Mon, 01 Feb 2016 20:07:53 -0800 (PST) Received: from mguzik.localdomain (ip-62-245-66-110.net.upcbroadband.cz. [62.245.66.110]) by smtp.gmail.com with ESMTPSA id n131sm550333wmf.9.2016.02.01.20.07.52 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 01 Feb 2016 20:07:52 -0800 (PST) From: Mateusz Guzik To: kib@freebsd.org Cc: freebsd-hackers@freebsd.org, Mateusz Guzik Subject: [PATCH 2/2] fork: plug a use after free of the returned process pointer Date: Tue, 2 Feb 2016 05:07:49 +0100 Message-Id: <1454386069-29657-3-git-send-email-mjguzik@gmail.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1454386069-29657-1-git-send-email-mjguzik@gmail.com> References: <20160201103632.GL91220@kib.kiev.ua> <1454386069-29657-1-git-send-email-mjguzik@gmail.com> X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 Feb 2016 04:07:55 -0000 From: Mateusz Guzik fork1 required its callers to pass a pointer to struct proc * which would be set to the new process (if any). procdesc and racct manipulation also used said pointer. However, the process could have exited prior to do_fork return and be automatically reaped, thus making this a use-after-free. Fix the problem by letting callers indicate whether they want the pid or the struct proc, return the process in stopped state for the latter case. --- sys/compat/cloudabi/cloudabi_proc.c | 2 - sys/kern/kern_fork.c | 104 +++++++++++++++++++----------------- sys/kern/kern_racct.c | 3 +- sys/sys/proc.h | 1 + 4 files changed, 58 insertions(+), 52 deletions(-) diff --git a/sys/compat/cloudabi/cloudabi_proc.c b/sys/compat/cloudabi/cloudabi_proc.c index 010efca..2bc50ca 100644 --- a/sys/compat/cloudabi/cloudabi_proc.c +++ b/sys/compat/cloudabi/cloudabi_proc.c @@ -77,12 +77,10 @@ cloudabi_sys_proc_fork(struct thread *td, { struct fork_req fr = {}; struct filecaps fcaps = {}; - struct proc *p2; int error, fd; cap_rights_init(&fcaps.fc_rights, CAP_FSTAT, CAP_EVENT); fr.fr_flags = RFFDG | RFPROC | RFPROCDESC; - fr.fr_procp = &p2; fr.fr_pd_fd = &fd; fr.fr_pd_fcaps = &fcaps; error = fork1(td, &fr); diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index 3b51b7f..d0c3837 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -102,14 +102,13 @@ int sys_fork(struct thread *td, struct fork_args *uap) { struct fork_req fr = {}; - int error; - struct proc *p2; + int error, pid; fr.fr_flags = RFFDG | RFPROC; - fr.fr_procp = &p2; + fr.fr_pidp = &pid; error = fork1(td, &fr); if (error == 0) { - td->td_retval[0] = p2->p_pid; + td->td_retval[0] = pid; td->td_retval[1] = 0; } return (error); @@ -122,11 +121,10 @@ sys_pdfork(td, uap) struct pdfork_args *uap; { struct fork_req fr = {}; - int error, fd; - struct proc *p2; + int error, fd, pid; fr.fr_flags = RFFDG | RFPROC | RFPROCDESC; - fr.fr_procp = &p2; + fr.fr_pidp = &pid; fr.fr_pd_fd = &fd; fr.fr_pd_flags = uap->flags; /* @@ -136,7 +134,7 @@ sys_pdfork(td, uap) */ error = fork1(td, &fr); if (error == 0) { - td->td_retval[0] = p2->p_pid; + td->td_retval[0] = pid; td->td_retval[1] = 0; error = copyout(&fd, uap->fdp, sizeof(fd)); } @@ -148,14 +146,13 @@ int sys_vfork(struct thread *td, struct vfork_args *uap) { struct fork_req fr = {}; - int error; - struct proc *p2; + int error, pid; fr.fr_flags = RFFDG | RFPROC | RFPPWAIT | RFMEM; - fr.fr_procp = &p2; + fr.fr_pidp = &pid; error = fork1(td, &fr); if (error == 0) { - td->td_retval[0] = p2->p_pid; + td->td_retval[0] = pid; td->td_retval[1] = 0; } return (error); @@ -165,8 +162,7 @@ int sys_rfork(struct thread *td, struct rfork_args *uap) { struct fork_req fr = {}; - struct proc *p2; - int error; + int error, pid; /* Don't allow kernel-only flags. */ if ((uap->flags & RFKERNELONLY) != 0) @@ -174,10 +170,10 @@ sys_rfork(struct thread *td, struct rfork_args *uap) AUDIT_ARG_FFLAGS(uap->flags); fr.fr_flags = uap->flags; - fr.fr_procp = &p2; + fr.fr_pidp = &pid; error = fork1(td, &fr); if (error == 0) { - td->td_retval[0] = p2 ? p2->p_pid : 0; + td->td_retval[0] = pid; td->td_retval[1] = 0; } return (error); @@ -378,20 +374,21 @@ fail: } static void -do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, - struct vmspace *vm2, int pdflags) +do_fork(struct thread *td, struct fork_req *fr, struct proc *p2, struct thread *td2, + struct vmspace *vm2, struct file *fp_procdesc) { struct proc *p1, *pptr; - int p2_held, trypid; + int trypid; struct filedesc *fd; struct filedesc_to_leader *fdtol; struct sigacts *newsigacts; + int flags; sx_assert(&proctree_lock, SX_SLOCKED); sx_assert(&allproc_lock, SX_XLOCKED); - p2_held = 0; p1 = td->td_proc; + flags = fr->fr_flags; trypid = fork_findpid(flags); @@ -690,7 +687,7 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, * However, don't do this until after fork(2) can no longer fail. */ if (flags & RFPROCDESC) - procdesc_new(p2, pdflags); + procdesc_new(p2, fr->fr_pd_flags); /* * Both processes are set up, now check if any loadable modules want @@ -718,6 +715,11 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, if ((flags & RFMEM) == 0 && dtrace_fasttrap_fork) dtrace_fasttrap_fork(p1, p2); #endif + /* + * Hold the process so that it cannot exit after we make it runnable, + * but before we wait for the debugger. + */ + _PHOLD(p2); if ((p1->p_flag & (P_TRACED | P_FOLLOWFORK)) == (P_TRACED | P_FOLLOWFORK)) { /* @@ -730,24 +732,12 @@ 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; td->td_rfppwait_p = p2; } PROC_UNLOCK(p2); - if ((flags & RFSTOPPED) == 0) { - /* - * If RFSTOPPED not requested, make child runnable and - * add to run queue. - */ - thread_lock(td2); - TD_SET_CAN_RUN(td2); - sched_add(td2, SRQ_BORING); - thread_unlock(td2); - } /* * Now can be swapped. @@ -761,14 +751,34 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, knote_fork(&p1->p_klist, p2->p_pid); SDT_PROBE3(proc, , , create, p2, p1, flags); + if (flags & RFPROCDESC) { + procdesc_finit(p2->p_procdesc, fp_procdesc); + fdrop(fp_procdesc, td); + } + + if ((flags & RFSTOPPED) == 0) { + /* + * If RFSTOPPED not requested, make child runnable and + * add to run queue. + */ + thread_lock(td2); + TD_SET_CAN_RUN(td2); + sched_add(td2, SRQ_BORING); + thread_unlock(td2); + if (fr->fr_pidp != NULL) + *fr->fr_pidp = p2->p_pid; + } else { + *fr->fr_procp = p2; + } + + PROC_LOCK(p2); /* * Wait until debugger is attached to child. */ - PROC_LOCK(p2); while ((td2->td_dbgflags & TDB_STOPATFORK) != 0) cv_wait(&p2->p_dbgwait, &p2->p_mtx); - if (p2_held) - _PRELE(p2); + _PRELE(p2); + racct_proc_fork_done(p2); PROC_UNLOCK(p2); } @@ -788,6 +798,11 @@ fork1(struct thread *td, struct fork_req *fr) flags = fr->fr_flags; pages = fr->fr_pages; + if ((flags & RFSTOPPED) != 0) + MPASS(fr->fr_procp != NULL && fr->fr_pidp == NULL); + else + MPASS(fr->fr_procp == NULL); + /* Check for the undefined or unimplemented flags. */ if ((flags & ~(RFFLAGS | RFTSIGFLAGS(RFTSIGMASK))) != 0) return (EINVAL); @@ -821,7 +836,10 @@ fork1(struct thread *td, struct fork_req *fr) * certain parts of a process from itself. */ if ((flags & RFPROC) == 0) { - *fr->fr_procp = NULL; + if (fr->fr_procp != NULL) + *fr->fr_procp = NULL; + else if (fr->fr_pidp != NULL) + *fr->fr_pidp = 0; return (fork_norfproc(td, flags)); } @@ -949,17 +967,7 @@ fork1(struct thread *td, struct fork_req *fr) lim_cur(td, RLIMIT_NPROC)); } if (ok) { - do_fork(td, flags, newproc, td2, vm2, fr->fr_pd_flags); - - /* - * Return child proc pointer to parent. - */ - *fr->fr_procp = newproc; - if (flags & RFPROCDESC) { - procdesc_finit(newproc->p_procdesc, fp_procdesc); - fdrop(fp_procdesc, td); - } - racct_proc_fork_done(newproc); + do_fork(td, fr, newproc, td2, vm2, fp_procdesc); return (0); } diff --git a/sys/kern/kern_racct.c b/sys/kern/kern_racct.c index 0c7c0c4..ce7e2a4 100644 --- a/sys/kern/kern_racct.c +++ b/sys/kern/kern_racct.c @@ -957,16 +957,15 @@ void racct_proc_fork_done(struct proc *child) { + PROC_LOCK_ASSERT(child, MA_OWNED); #ifdef RCTL if (!racct_enable) return; - PROC_LOCK(child); mtx_lock(&racct_lock); rctl_enforce(child, RACCT_NPROC, 0); rctl_enforce(child, RACCT_NTHR, 0); mtx_unlock(&racct_lock); - PROC_UNLOCK(child); #endif } diff --git a/sys/sys/proc.h b/sys/sys/proc.h index ac96566..039fd39 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -910,6 +910,7 @@ struct proc *zpfind(pid_t); /* Find zombie process by id. */ struct fork_req { int fr_flags; int fr_pages; + int *fr_pidp; struct proc **fr_procp; int *fr_pd_fd; int fr_pd_flags; -- 2.7.0