[PATCH 2/2] fork: plug a use after free of the returned process pointer
Konstantin Belousov
kib at freebsd.org
Tue Feb 2 13:23:30 UTC 2016
On Tue, Feb 02, 2016 at 05:07:49AM +0100, Mateusz Guzik wrote:
> From: Mateusz Guzik <mjg at freebsd.org>
>
> 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.
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(-)
>
> 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)
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;
>
> sx_assert(&proctree_lock, SX_SLOCKED);
> sx_assert(&allproc_lock, SX_XLOCKED);
>
> - p2_held = 0;
> p1 = td->td_proc;
> + flags = fr->fr_flags;
Why not use fr->fr_flags directly ? It is slightly more churn, but IMO
it is worth it.
>
> 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.
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)) == (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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20160202/c0d248f7/attachment.sig>
More information about the freebsd-hackers
mailing list