From owner-freebsd-hackers@freebsd.org Mon Feb 1 05:13:10 2016 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id DBCC9A90207 for ; Mon, 1 Feb 2016 05:13:09 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 6A7B7172A; Mon, 1 Feb 2016 05:13:09 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wm0-x241.google.com with SMTP id r129so7399585wmr.0; Sun, 31 Jan 2016 21:13:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=ymSBW8BFAjMPDajg056Yhg3alZEtpSgsEBbc8Etwm8c=; b=F7bUbp65horYjIDmTGPIl7k3++0AFhgAdYo+UseFdK8959buGgWG0p/UASwEry8hf+ Ecl9nH7M5ObrqT1YqjDATsO7CWw6PMh/iAGWAe+2laIzPleTAk9C7p6oz5r7JPVrdN5Y SW0RkoZEiZsURdi6R7n4pZJ1XTuyLThJkpThsl7iDjhyUG8bJK7ble5l6TKxF9LtGnWx PuRf6YV+KY0oAGoK72q456XVJ2seBZt5nCoFXgQipN3H01QTU/mCuyloesl5eJksSPDj jFutMFVEgq6dcS8HHcj7B/y36vTgjSBa1or+t6CZ5I2kv7lZeBzr+CIXCvA3PIABBNvy G7qg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=ymSBW8BFAjMPDajg056Yhg3alZEtpSgsEBbc8Etwm8c=; b=fj9AqswVBGLEZHm4QVPKst7mbjMCZQLNx2XeRmyBLoNIBar18zLIRW4+BOE58M6442 yPf+iXHEKkf4HoR0+PaulVFSdBOO07aNRlqT+y+0X22LOzvVGDJdyCBMGyRbtaWueViF RlObaGXgM4FXPWIzskqankkuh33mFjMKUxwC5ECpRLx22zFgpdGKG2+yCI6UcN18MPO9 5cAzE0/CMAShx3yGK2UWrWXZBSP9++qu8k9Km9BzKkYpwtyIid3+eBIwTw8bycdtbxBG 7xm+C9Au3Oeqc4DufhHX7i72AlykwNuBkhkxgSHrn26JdyNrUBpRoilZWfexCzVg1VX8 txhg== X-Gm-Message-State: AG10YOTPspq6Up+IAn4S5eQUCR4/d+G/pmQI29wIwTdt4QHaKxp+bccG4JSX5Tn9Yl0A8A== X-Received: by 10.28.64.131 with SMTP id n125mr8799306wma.65.1454303587891; Sun, 31 Jan 2016 21:13:07 -0800 (PST) Received: from mguzik.localdomain (ip-62-245-66-110.net.upcbroadband.cz. [62.245.66.110]) by smtp.gmail.com with ESMTPSA id x6sm27239437wje.38.2016.01.31.21.13.07 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 31 Jan 2016 21:13:07 -0800 (PST) From: Mateusz Guzik To: freebsd-hackers@freebsd.org Cc: kib@freebsd.org, Mateusz Guzik Subject: [PATCH 2/2] fork: plug a use after free of the returned process pointer Date: Mon, 1 Feb 2016 06:13:04 +0100 Message-Id: <1454303584-20941-3-git-send-email-mjguzik@gmail.com> X-Mailer: git-send-email 2.4.3 In-Reply-To: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> References: <1454303584-20941-1-git-send-email-mjguzik@gmail.com> X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 01 Feb 2016 05:13:10 -0000 From: Mateusz Guzik 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