Date: Mon, 1 Feb 2016 06:13:04 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: freebsd-hackers@freebsd.org Cc: kib@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: [PATCH 2/2] fork: plug a use after free of the returned process pointer Message-ID: <1454303584-20941-3-git-send-email-mjguzik@gmail.com> In-Reply-To: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> References: <1454303584-20941-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 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1454303584-20941-3-git-send-email-mjguzik>