Date: Mon, 28 Dec 2009 14:20:03 GMT From: Gleb Kurtsou <gleb.kurtsou@gmail.com> To: freebsd-emulation@FreeBSD.org Subject: Re: kern/142082: [patch] [panic] linuxulator: getppid: use after free Message-ID: <200912281420.nBSEK3Tl064947@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/142082; it has been noted by GNATS.
From: Gleb Kurtsou <gleb.kurtsou@gmail.com>
To: Lucius Windschuh <lwindschuh@googlemail.com>
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 <gleb.kurtsou@gmail.com>
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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200912281420.nBSEK3Tl064947>