From owner-freebsd-hackers@freebsd.org Tue Feb 2 13:23:30 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 95D57A97BF5 for ; Tue, 2 Feb 2016 13:23:30 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 2DDC3A17; Tue, 2 Feb 2016 13:23:30 +0000 (UTC) (envelope-from kib@freebsd.org) Received: from tom.home (kostik@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id u12DNMP2086056 (version=TLSv1 cipher=DHE-RSA-CAMELLIA256-SHA bits=256 verify=NO); Tue, 2 Feb 2016 15:23:23 +0200 (EET) (envelope-from kib@freebsd.org) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua u12DNMP2086056 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id u12DNMI8086055; Tue, 2 Feb 2016 15:23:22 +0200 (EET) (envelope-from kib@freebsd.org) X-Authentication-Warning: tom.home: kostik set sender to kib@freebsd.org using -f Date: Tue, 2 Feb 2016 15:23:22 +0200 From: Konstantin Belousov To: Mateusz Guzik Cc: freebsd-hackers@freebsd.org, Mateusz Guzik Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160202132322.GU91220@kib.kiev.ua> References: <20160201103632.GL91220@kib.kiev.ua> <1454386069-29657-1-git-send-email-mjguzik@gmail.com> <1454386069-29657-3-git-send-email-mjguzik@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="YToU2i3Vx8H2dn7O" Content-Disposition: inline In-Reply-To: <1454386069-29657-3-git-send-email-mjguzik@gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Spam-Status: No, score=-2.9 required=5.0 tests=ALL_TRUSTED,BAYES_00 autolearn=ham autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home 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 13:23:30 -0000 --YToU2i3Vx8H2dn7O Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Feb 02, 2016 at 05:07:49AM +0100, Mateusz Guzik wrote: > From: Mateusz Guzik >=20 > 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. >=20 > However, the process could have exited prior to do_fork return and be > automatically reaped, thus making this a use-after-free. >=20 > 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. Patch looks fine, some style notes and one question is below. > --- > 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(-) >=20 > diff --git a/sys/compat/cloudabi/cloudabi_proc.c b/sys/compat/cloudabi/cl= oudabi_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 =3D {}; > struct filecaps fcaps =3D {}; > - struct proc *p2; > int error, fd; > =20 > cap_rights_init(&fcaps.fc_rights, CAP_FSTAT, CAP_EVENT); > fr.fr_flags =3D RFFDG | RFPROC | RFPROCDESC; > - fr.fr_procp =3D &p2; > fr.fr_pd_fd =3D &fd; > fr.fr_pd_fcaps =3D &fcaps; > error =3D 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 =3D {}; > - int error; > - struct proc *p2; > + int error, pid; > =20 > fr.fr_flags =3D RFFDG | RFPROC; > - fr.fr_procp =3D &p2; > + fr.fr_pidp =3D &pid; > error =3D fork1(td, &fr); > if (error =3D=3D 0) { > - td->td_retval[0] =3D p2->p_pid; > + td->td_retval[0] =3D pid; > td->td_retval[1] =3D 0; > } > return (error); > @@ -122,11 +121,10 @@ sys_pdfork(td, uap) > struct pdfork_args *uap; > { > struct fork_req fr =3D {}; > - int error, fd; > - struct proc *p2; > + int error, fd, pid; > =20 > fr.fr_flags =3D RFFDG | RFPROC | RFPROCDESC; > - fr.fr_procp =3D &p2; > + fr.fr_pidp =3D &pid; > fr.fr_pd_fd =3D &fd; > fr.fr_pd_flags =3D uap->flags; > /* > @@ -136,7 +134,7 @@ sys_pdfork(td, uap) > */ > error =3D fork1(td, &fr); > if (error =3D=3D 0) { > - td->td_retval[0] =3D p2->p_pid; > + td->td_retval[0] =3D pid; > td->td_retval[1] =3D 0; > error =3D copyout(&fd, uap->fdp, sizeof(fd)); > } > @@ -148,14 +146,13 @@ int > sys_vfork(struct thread *td, struct vfork_args *uap) > { > struct fork_req fr =3D {}; > - int error; > - struct proc *p2; > + int error, pid; > =20 > fr.fr_flags =3D RFFDG | RFPROC | RFPPWAIT | RFMEM; > - fr.fr_procp =3D &p2; > + fr.fr_pidp =3D &pid; > error =3D fork1(td, &fr); > if (error =3D=3D 0) { > - td->td_retval[0] =3D p2->p_pid; > + td->td_retval[0] =3D pid; > td->td_retval[1] =3D 0; > } > return (error); > @@ -165,8 +162,7 @@ int > sys_rfork(struct thread *td, struct rfork_args *uap) > { > struct fork_req fr =3D {}; > - struct proc *p2; > - int error; > + int error, pid; > =20 > /* Don't allow kernel-only flags. */ > if ((uap->flags & RFKERNELONLY) !=3D 0) > @@ -174,10 +170,10 @@ sys_rfork(struct thread *td, struct rfork_args *uap) > =20 > AUDIT_ARG_FFLAGS(uap->flags); > fr.fr_flags =3D uap->flags; > - fr.fr_procp =3D &p2; > + fr.fr_pidp =3D &pid; > error =3D fork1(td, &fr); > if (error =3D=3D 0) { > - td->td_retval[0] =3D p2 ? p2->p_pid : 0; > + td->td_retval[0] =3D pid; > td->td_retval[1] =3D 0; > } > return (error); > @@ -378,20 +374,21 @@ fail: > } > =20 > static void > -do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td= 2, > - 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) Style, line is too long. > { > struct proc *p1, *pptr; > - int p2_held, trypid; > + int trypid; > struct filedesc *fd; > struct filedesc_to_leader *fdtol; > struct sigacts *newsigacts; > + int flags; > =20 > sx_assert(&proctree_lock, SX_SLOCKED); > sx_assert(&allproc_lock, SX_XLOCKED); > =20 > - p2_held =3D 0; > p1 =3D td->td_proc; > + flags =3D fr->fr_flags; Why not use fr->fr_flags directly ? It is slightly more churn, but IMO it is worth it. > =20 > trypid =3D fork_findpid(flags); > =20 > @@ -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); > =20 > /* > * 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 *p= 2, struct thread *td2, > if ((flags & RFMEM) =3D=3D 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. Is this possible ? The forked child must execute through fork_return(), and there we do ptracestop() before the child has a chance to ever return to usermode. Do you mean a scenario where the debugger detaches before child executes fork_return() and TDP_STOPATFORK is cleared in advance ? > + */ > + _PHOLD(p2); > if ((p1->p_flag & (P_TRACED | P_FOLLOWFORK)) =3D=3D (P_TRACED | > P_FOLLOWFORK)) { > /* > @@ -730,24 +732,12 @@ do_fork(struct thread *td, int flags, struct proc *= p2, struct thread *td2, > td->td_dbgflags |=3D TDB_FORK; > td->td_dbg_forked =3D p2->p_pid; > td2->td_dbgflags |=3D TDB_STOPATFORK; > - _PHOLD(p2); > - p2_held =3D 1; > } > if (flags & RFPPWAIT) { > td->td_pflags |=3D TDP_RFPPWAIT; > td->td_rfppwait_p =3D p2; > } > PROC_UNLOCK(p2); > - if ((flags & RFSTOPPED) =3D=3D 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); > - } > =20 > /* > * 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); > =20 > + if (flags & RFPROCDESC) { > + procdesc_finit(p2->p_procdesc, fp_procdesc); > + fdrop(fp_procdesc, td); > + } > + > + if ((flags & RFSTOPPED) =3D=3D 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 !=3D NULL) > + *fr->fr_pidp =3D p2->p_pid; > + } else { > + *fr->fr_procp =3D p2; > + } > + > + PROC_LOCK(p2); > /* > * Wait until debugger is attached to child. > */ > - PROC_LOCK(p2); > while ((td2->td_dbgflags & TDB_STOPATFORK) !=3D 0) > cv_wait(&p2->p_dbgwait, &p2->p_mtx); > - if (p2_held) > - _PRELE(p2); > + _PRELE(p2); > + racct_proc_fork_done(p2); > PROC_UNLOCK(p2); > } > =20 > @@ -788,6 +798,11 @@ fork1(struct thread *td, struct fork_req *fr) > flags =3D fr->fr_flags; > pages =3D fr->fr_pages; > =20 > + if ((flags & RFSTOPPED) !=3D 0) > + MPASS(fr->fr_procp !=3D NULL && fr->fr_pidp =3D=3D NULL); > + else > + MPASS(fr->fr_procp =3D=3D NULL); > + > /* Check for the undefined or unimplemented flags. */ > if ((flags & ~(RFFLAGS | RFTSIGFLAGS(RFTSIGMASK))) !=3D 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) =3D=3D 0) { > - *fr->fr_procp =3D NULL; > + if (fr->fr_procp !=3D NULL) > + *fr->fr_procp =3D NULL; > + else if (fr->fr_pidp !=3D NULL) > + *fr->fr_pidp =3D 0; > return (fork_norfproc(td, flags)); > } > =20 > @@ -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 =3D 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); > } > =20 > 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) > { > =20 > + PROC_LOCK_ASSERT(child, MA_OWNED); > #ifdef RCTL > if (!racct_enable) > return; > =20 > - 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 > } > =20 > 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 b= y 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; > --=20 > 2.7.0 --YToU2i3Vx8H2dn7O Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBCAAGBQJWsK3KAAoJEJDCuSvBvK1Bs2cP/2+e9oBdarmZxy4bGjzF0xbY CHWjs3Hpt5x/vrMxMXzEwB0ggEK2vA3HBbtvjLn5mDUX0UBode3OnUh2aGBqqwRG XdS9fQdbjIxl0qA5GdEp27jxvugDi2JxZ7Xom4JErS/bKJBeHKcutBo89n3KEMCA a6OEivYcdN8qJPd5TfOYDZVJDUHf98IwhZgKZ0O94toZybIWEFLnMZJBlHG0Y92V hKe469h3R6uK2SPpHKYZ8bkbrUCubqm+ROB404sRBiuahTYv6Iea726v7kEU5yjC nxPp+Jhly++lWCtVqh9ymZXGP1bIaZV+Qv/IsvtExDgFL/NSK6rvQQ52cRrxoaS4 CILlISeuV+2KjQh3Z9ceKyqn1hhO3xyb7dv56y+ASI0a0tT5hMHZYYZmPVR0uR3J QYEz3jR9VK9JyCrDRLY3Ny/JtyawJfKl92Od98OdLMRrh6UePdCNy1D2/wWUR7Gt KVWbTGhhy9UDp/yfQVMrdXc1GUrtL9CfpN/aGiJboEOMAaHEc81hLzDPMIRr5sGS vSX6T5b2LOmRUEqzTWqys3h1UIpWN2Bs9tYGZv1C1yIOXmZWlhv2pgc5xoRfC2YK 0LXK9n1onsX6EgJzyhfrmw6JBpW2OXIyXQZI8Y6a2RorKp4CemGQdWCfvoE1ITQB FOUAUKHyO0Y5hdEIBIKV =vt4+ -----END PGP SIGNATURE----- --YToU2i3Vx8H2dn7O--