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