Date: Tue, 2 Feb 2016 15:23:22 +0200 From: Konstantin Belousov <kib@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-hackers@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <20160202132322.GU91220@kib.kiev.ua> In-Reply-To: <1454386069-29657-3-git-send-email-mjguzik@gmail.com> References: <20160201103632.GL91220@kib.kiev.ua> <1454386069-29657-1-git-send-email-mjguzik@gmail.com> <1454386069-29657-3-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <mjg@freebsd.org>
>=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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160202132322.GU91220>