Date: Mon, 1 Feb 2016 06:28:09 +0100 From: Mateusz Guzik <mjguzik@gmail.com> 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> In-Reply-To: <1454303584-20941-3-git-send-email-mjguzik@gmail.com> References: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> <1454303584-20941-3-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <mjg@freebsd.org>
>
> 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20160201052809.GA7127>