From owner-freebsd-hackers@freebsd.org Mon Feb 1 05:28:13 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 9201BA8E7BA for ; Mon, 1 Feb 2016 05:28:13 +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 1C8C11CD3; Mon, 1 Feb 2016 05:28:13 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x243.google.com with SMTP id p63so7427798wmp.1; Sun, 31 Jan 2016 21:28:13 -0800 (PST) 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:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=7C0JqcVNqhWEi07eZMmhs1RNTndQgEyXwcJrW6CU3Fk=; b=Frg+kIXIYUv8cRwyNFBo7jeOq6q1Gb+mYwJXuDJG86rrxefczNmyh+tbGHeGAd7HAs 907kMCHo55j0wC4Sx5UxEkE/+tWmYtf3hGZIarOGdXK7Ycq6I/yVd6IYuy66coHBO/mp q/B23N+P/JcPMEvNwZ0p3nr8TKUG7i5wnmCJRr/ALOk5khBPiqc4Xnub1AtKK39IoHW/ Aknkc3bNK6sAqkwAcPhC90mPWlGWBV5a0anAIHhvbn+ISpcagYF752ibaIpXkdAgPYNG EdK/hogsAR1KR83dfrmrcv0FvRVlpu8nvKShFRip6oAZtF1hZSd1mTAl1CUYx3zE+T2i /ujA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id :mail-followup-to:references:mime-version:content-type :content-disposition:in-reply-to:user-agent; bh=7C0JqcVNqhWEi07eZMmhs1RNTndQgEyXwcJrW6CU3Fk=; b=i50sWYxgn6++5z0Hm3jTbzSFh0ImQH/H9XlPgrpybZcreti48ixsCEcMcwC3d8TeTx KYIxTwIcdhTRHkjkrW28L+vfQwHfrj+JwAW3ex83qq58Q3PO66zVsrPrp2IaXR8B9Zly NsjxJHUQ36qve1PGpjUTHRqUzuZsM/QtLg3GELYmprE4kYY4uBbvnWwzNISCmTkUTPTh JEAN99YOooZspnTaXpwoDj0E6+Qdfo66RRPv3VVlXyXBcwyJpxooatEDjLFDiuZIs+iS KZKteXJ6vi8ts1Duuvm8r7fzdw7YnFL7DmPBCXEI+hB0jTaI28Z3xCSwABSWaFIogJXp 4iQg== X-Gm-Message-State: AG10YOQ//xsk9mRSxNP+fE79fdnpWhR8fHgdBzsqGi0Tlld3d+ceYZ4pV6KCgB0LEgl8qQ== X-Received: by 10.28.132.146 with SMTP id g140mr9851274wmd.49.1454304491684; Sun, 31 Jan 2016 21:28:11 -0800 (PST) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by smtp.gmail.com with ESMTPSA id m206sm9569272wmf.16.2016.01.31.21.28.10 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Sun, 31 Jan 2016 21:28:11 -0800 (PST) Date: Mon, 1 Feb 2016 06:28:09 +0100 From: Mateusz Guzik To: freebsd-hackers@freebsd.org Cc: kib@freebsd.org Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160201052809.GA7127@dft-labs.eu> Mail-Followup-To: Mateusz Guzik , freebsd-hackers@freebsd.org, kib@freebsd.org References: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> <1454303584-20941-3-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1454303584-20941-3-git-send-email-mjguzik@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) 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: Mon, 01 Feb 2016 05:28:13 -0000 On Mon, Feb 01, 2016 at 06:13:04AM +0100, Mateusz Guzik wrote: Ops, the diff excluded racct change (see the end). > From: Mateusz Guzik > > fork1 required its callers to pass a point 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. > > Fix the problem by changing the API to let callers indicate whether they > want the pid or the struct proc, return the process in stopped state for > the latter case. > > The process is held after its thread is marked as runnable to prevent it > from exiting untill all work is done. > --- > sys/compat/cloudabi/cloudabi_proc.c | 3 +- > sys/compat/linux/linux_fork.c | 7 ++- > sys/kern/init_main.c | 2 +- > sys/kern/kern_fork.c | 108 ++++++++++++++++++++---------------- > sys/kern/kern_kthread.c | 2 +- > sys/sys/proc.h | 2 +- > 6 files changed, 67 insertions(+), 57 deletions(-) > > diff --git a/sys/compat/cloudabi/cloudabi_proc.c b/sys/compat/cloudabi/cloudabi_proc.c > index e98471b..d8c3bef 100644 > --- a/sys/compat/cloudabi/cloudabi_proc.c > +++ b/sys/compat/cloudabi/cloudabi_proc.c > @@ -78,13 +78,12 @@ cloudabi_sys_proc_fork(struct thread *td, > { > struct procdesc_req pdr; > struct filecaps fcaps = {}; > - struct proc *p2; > int error; > > pdr.pdr_flags = 0; > pdr.pdr_fcaps = &fcaps; > cap_rights_init(&fcaps.fc_rights, CAP_FSTAT, CAP_EVENT); > - error = fork1(td, RFFDG | RFPROC | RFPROCDESC, 0, &p2, &pdr); > + error = fork1(td, RFFDG | RFPROC | RFPROCDESC, 0, NULL, NULL, &pdr); > if (error != 0) > return (error); > /* Return the file descriptor to the parent process. */ > diff --git a/sys/compat/linux/linux_fork.c b/sys/compat/linux/linux_fork.c > index 7cbe216..0c0f800 100644 > --- a/sys/compat/linux/linux_fork.c > +++ b/sys/compat/linux/linux_fork.c > @@ -73,7 +73,8 @@ linux_fork(struct thread *td, struct linux_fork_args *args) > printf(ARGS(fork, "")); > #endif > > - if ((error = fork1(td, RFFDG | RFPROC | RFSTOPPED, 0, &p2, NULL)) != 0) > + if ((error = fork1(td, RFFDG | RFPROC | RFSTOPPED, 0, &p2, NULL, > + NULL)) != 0) > return (error); > > td2 = FIRST_THREAD_IN_PROC(p2); > @@ -106,7 +107,7 @@ linux_vfork(struct thread *td, struct linux_vfork_args *args) > #endif > > if ((error = fork1(td, RFFDG | RFPROC | RFMEM | RFPPWAIT | RFSTOPPED, > - 0, &p2, NULL)) != 0) > + 0, &p2, NULL, NULL)) != 0) > return (error); > > td2 = FIRST_THREAD_IN_PROC(p2); > @@ -169,7 +170,7 @@ linux_clone_proc(struct thread *td, struct linux_clone_args *args) > if (args->flags & LINUX_CLONE_VFORK) > ff |= RFPPWAIT; > > - error = fork1(td, ff, 0, &p2, NULL); > + error = fork1(td, ff, 0, &p2, NULL, NULL); > if (error) > return (error); > > diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c > index 7d0443a..d4fdcc0 100644 > --- a/sys/kern/init_main.c > +++ b/sys/kern/init_main.c > @@ -833,7 +833,7 @@ create_init(const void *udata __unused) > int error; > > error = fork1(&thread0, RFFDG | RFPROC | RFSTOPPED, 0, &initproc, > - NULL); > + NULL, NULL); > if (error) > panic("cannot fork init: %d\n", error); > KASSERT(initproc->p_pid == 1, ("create_init: initproc->p_pid != 1")); > diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c > index 8cc56b7..d15f517 100644 > --- a/sys/kern/kern_fork.c > +++ b/sys/kern/kern_fork.c > @@ -101,12 +101,11 @@ struct fork_args { > int > sys_fork(struct thread *td, struct fork_args *uap) > { > - int error; > - struct proc *p2; > + int error, pid; > > - error = fork1(td, RFFDG | RFPROC, 0, &p2, NULL); > + error = fork1(td, RFFDG | RFPROC, 0, NULL, &pid, NULL); > if (error == 0) { > - td->td_retval[0] = p2->p_pid; > + td->td_retval[0] = pid; > td->td_retval[1] = 0; > } > return (error); > @@ -119,8 +118,7 @@ sys_pdfork(td, uap) > struct pdfork_args *uap; > { > struct procdesc_req pdr; > - struct proc *p2; > - int error; > + int error, pid; > > /* > * It is necessary to return fd by reference because 0 is a valid file > @@ -129,9 +127,9 @@ sys_pdfork(td, uap) > */ > pdr.pdr_flags = uap->flags; > pdr.pdr_fcaps = NULL; > - error = fork1(td, RFFDG | RFPROC | RFPROCDESC, 0, &p2, &pdr); > + error = fork1(td, RFFDG | RFPROC | RFPROCDESC, 0, NULL, &pid, &pdr); > if (error == 0) { > - td->td_retval[0] = p2->p_pid; > + td->td_retval[0] = pid; > td->td_retval[1] = 0; > error = copyout(&pdr.pdr_fd, uap->fdp, sizeof(pdr.pdr_fd)); > } > @@ -142,13 +140,12 @@ sys_pdfork(td, uap) > int > sys_vfork(struct thread *td, struct vfork_args *uap) > { > - int error, flags; > - struct proc *p2; > + int error, pid; > > - flags = RFFDG | RFPROC | RFPPWAIT | RFMEM; > - error = fork1(td, flags, 0, &p2, NULL); > + error = fork1(td, RFFDG | RFPROC | RFPPWAIT | RFMEM, 0, NULL, &pid, > + NULL); > if (error == 0) { > - td->td_retval[0] = p2->p_pid; > + td->td_retval[0] = pid; > td->td_retval[1] = 0; > } > return (error); > @@ -157,17 +154,16 @@ sys_vfork(struct thread *td, struct vfork_args *uap) > int > sys_rfork(struct thread *td, struct rfork_args *uap) > { > - struct proc *p2; > - int error; > + int error, pid; > > /* Don't allow kernel-only flags. */ > if ((uap->flags & RFKERNELONLY) != 0) > return (EINVAL); > > AUDIT_ARG_FFLAGS(uap->flags); > - error = fork1(td, uap->flags, 0, &p2, NULL); > + error = fork1(td, uap->flags, 0, NULL, &pid, NULL); > if (error == 0) { > - td->td_retval[0] = p2 ? p2->p_pid : 0; > + td->td_retval[0] = pid; > td->td_retval[1] = 0; > } > return (error); > @@ -369,10 +365,11 @@ fail: > > static void > do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, > - struct vmspace *vm2, int pdflags) > + struct vmspace *vm2, struct proc **procp, int *procpid, int pdflags, > + 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; > @@ -380,7 +377,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; > > trypid = fork_findpid(flags); > @@ -708,6 +704,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)) { > /* > @@ -720,25 +721,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. > */ > @@ -751,20 +739,43 @@ 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 (procpid != NULL) > + *procpid = p2->p_pid; > + 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); > + } else { > + /* > + * Return child proc pointer to parent. > + */ > + *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); > } > > int > fork1(struct thread *td, int flags, int pages, struct proc **procp, > - struct procdesc_req *pdr) > + int *procpid, struct procdesc_req *pdr) > { > struct proc *p1, *newproc; > struct thread *td2; > @@ -775,6 +786,11 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp, > static int curfail; > static struct timeval lastfail; > > + if ((flags & RFSTOPPED) != 0) > + MPASS(procp != NULL && procpid == NULL); > + else > + MPASS(procp == NULL); > + > /* Check for the undefined or unimplemented flags. */ > if ((flags & ~(RFFLAGS | RFTSIGFLAGS(RFTSIGMASK))) != 0) > return (EINVAL); > @@ -810,7 +826,10 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp, > * certain parts of a process from itself. > */ > if ((flags & RFPROC) == 0) { > - *procp = NULL; > + if (procp != NULL) > + *procp = NULL; > + if (procpid != NULL) > + *procpid = 0; > return (fork_norfproc(td, flags)); > } > > @@ -938,17 +957,8 @@ fork1(struct thread *td, int flags, int pages, struct proc **procp, > lim_cur(td, RLIMIT_NPROC)); > } > if (ok) { > - do_fork(td, flags, newproc, td2, vm2, pdflags); > - > - /* > - * Return child proc pointer to parent. > - */ > - *procp = newproc; > - if (flags & RFPROCDESC) { > - procdesc_finit(newproc->p_procdesc, fp_procdesc); > - fdrop(fp_procdesc, td); > - } > - racct_proc_fork_done(newproc); > + do_fork(td, flags, newproc, td2, vm2, procp, procpid, pdflags, > + fp_procdesc); > return (0); > } > > diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c > index 0673f68..67f6cc2 100644 > --- a/sys/kern/kern_kthread.c > +++ b/sys/kern/kern_kthread.c > @@ -89,7 +89,7 @@ kproc_create(void (*func)(void *), void *arg, > panic("kproc_create called too soon"); > > error = fork1(&thread0, RFMEM | RFFDG | RFPROC | RFSTOPPED | flags, > - pages, &p2, NULL); > + pages, &p2, NULL, NULL); > if (error) > return error; > > diff --git a/sys/sys/proc.h b/sys/sys/proc.h > index 60efdcd..b072e10 100644 > --- a/sys/sys/proc.h > +++ b/sys/sys/proc.h > @@ -931,7 +931,7 @@ int enterpgrp(struct proc *p, pid_t pgid, struct pgrp *pgrp, > int enterthispgrp(struct proc *p, struct pgrp *pgrp); > void faultin(struct proc *p); > void fixjobc(struct proc *p, struct pgrp *pgrp, int entering); > -int fork1(struct thread *, int, int, struct proc **, > +int fork1(struct thread *, int, int, struct proc **, int *, > struct procdesc_req *); > void fork_exit(void (*)(void *, struct trapframe *), void *, > struct trapframe *); > -- > 2.7.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 } -- Mateusz Guzik