From owner-freebsd-emulation@FreeBSD.ORG Mon Dec 28 14:20:03 2009 Return-Path: Delivered-To: freebsd-emulation@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id A4A3E106566B for ; Mon, 28 Dec 2009 14:20:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 78E368FC13 for ; Mon, 28 Dec 2009 14:20:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id nBSEK3Q0064948 for ; Mon, 28 Dec 2009 14:20:03 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id nBSEK3Tl064947; Mon, 28 Dec 2009 14:20:03 GMT (envelope-from gnats) Date: Mon, 28 Dec 2009 14:20:03 GMT Message-Id: <200912281420.nBSEK3Tl064947@freefall.freebsd.org> To: freebsd-emulation@FreeBSD.org From: Gleb Kurtsou Cc: Subject: Re: kern/142082: [patch] [panic] linuxulator: getppid: use after free X-BeenThere: freebsd-emulation@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Gleb Kurtsou List-Id: Development of Emulators of other operating systems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Dec 2009 14:20:03 -0000 The following reply was made to PR kern/142082; it has been noted by GNATS. From: Gleb Kurtsou To: Lucius Windschuh Cc: bug-followup@freebsd.org Subject: Re: kern/142082: [patch] [panic] linuxulator: getppid: use after free Date: Mon, 28 Dec 2009 16:12:51 +0200 --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=utf-8 Content-Disposition: inline On (28/12/2009 13:39), Lucius Windschuh wrote: > Hi Gleb, > I have a question on the proposed patch: Is this code path which you > extend by querying and setting p->p_emuldata protected by a lock which > I don't see and which prevents two threads from running > linux_proc_exit and linux_getppid in parallel? > > Else, I think that we have a short time window after free()ing em in > linux_proc_exit and before clearing p->p_emuldata, where it looks like > the emuldata is valid, but already freed. This would cause a panic or > undefined bahaviour, again. Although the time frame is very small. Thanks, for a point. I've updated the patch a bit to set p_emuldata=NULL with PROC_LOCK held. That should fix this particular race. But there's still a race documented in linux_proc_exec (XXX comment): p_emuldata can be dereferenced after being set to NULL. That's what p_emuldata check is for. Not sure about the rest of the code though, getppid is a bit special here. > > Regards > > Lucius --9jxsPFA5p3P2qPhR Content-Type: text/plain; charset=utf-8 Content-Disposition: attachment; filename="linuxulator-getppid-use_after_free.patch-2.txt" commit 09d5cc62f82b251eaf2c3548a026ed09c10cbbae Author: Gleb Kurtsou Date: Mon Dec 28 16:08:44 2009 +0200 linuxulator: getppid: fix use after free panic diff --git a/sys/compat/linux/linux_emul.c b/sys/compat/linux/linux_emul.c index dc81553..5086047 100644 --- a/sys/compat/linux/linux_emul.c +++ b/sys/compat/linux/linux_emul.c @@ -202,6 +202,9 @@ linux_proc_exit(void *arg __unused, struct proc *p) error = copyout(&null, child_clear_tid, sizeof(null)); if (error) { + PROC_LOCK(p); + p->p_emuldata = NULL; + PROC_UNLOCK(p); free(em, M_LINUX); return; } @@ -223,6 +226,9 @@ linux_proc_exit(void *arg __unused, struct proc *p) } /* clean the stuff up */ + PROC_LOCK(p); + p->p_emuldata = NULL; + PROC_UNLOCK(p); free(em, M_LINUX); /* this is a little weird but rewritten from exit1() */ @@ -267,17 +273,17 @@ linux_proc_exec(void *arg __unused, struct proc *p, struct image_params *imgp) * time so some other process might reference it and try to * access its p->p_emuldata and panicing on a NULL reference. */ + + PROC_LOCK(p); em = em_find(p, EMUL_DONTLOCK); + p->p_emuldata = NULL; + PROC_UNLOCK(p); KASSERT(em != NULL, ("proc_exec: emuldata not found.\n")); EMUL_SHARED_WLOCK(&emul_shared_lock); LIST_REMOVE(em, threads); - PROC_LOCK(p); - p->p_emuldata = NULL; - PROC_UNLOCK(p); - em->shared->refs--; if (em->shared->refs == 0) { EMUL_SHARED_WUNLOCK(&emul_shared_lock); diff --git a/sys/compat/linux/linux_misc.c b/sys/compat/linux/linux_misc.c index 1d5eaf8..bd8be89 100644 --- a/sys/compat/linux/linux_misc.c +++ b/sys/compat/linux/linux_misc.c @@ -1581,10 +1581,8 @@ linux_getppid(struct thread *td, struct linux_getppid_args *args) PROC_UNLOCK(p); /* if its also linux process */ - if (pp->p_sysent == &elf_linux_sysvec) { - em = em_find(pp, EMUL_DONTLOCK); - KASSERT(em != NULL, ("getppid: parent emuldata not found.\n")); - + if (pp->p_sysent == &elf_linux_sysvec && + (em = em_find(pp, EMUL_DONTLOCK)) != NULL) { td->td_retval[0] = em->shared->group_pid; } else td->td_retval[0] = pp->p_pid; --9jxsPFA5p3P2qPhR--