kern/142082: [patch] [panic] linuxulator: getppid: use after free

[Gleb Kurtsou]

The following reply was made to PR kern/142082; it has been noted by GNATS.

From: Gleb Kurtsou <gleb.kurtsou at gmail.com>
To: Lucius Windschuh <lwindschuh at googlemail.com>
Cc: bug-followup at 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 at 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--