kern/151758: [panic] tmux kernel panic, with out root privilegies

[John Baldwin]

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

From: John Baldwin <jhb at FreeBSD.org>
To: Kostik Belousov <kostikbel at gmail.com>
Cc: bug-followup at freebsd.org, andrey at shidakov.ru
Subject: Re: kern/151758: [panic] tmux kernel panic, with out root privilegies
Date: Thu, 08 Dec 2011 11:02:15 -0500

 On 12/8/11 10:32 AM, Kostik Belousov wrote:
 > On Thu, Dec 08, 2011 at 10:24:56AM -0500, John Baldwin wrote:
 >> The bug is that during unp_gc(), we pass NULL as the thread to closef()
 >> (to disable certain locking stuff, and because the thread performing the
 >> gc doesn't "own" orphaned file descriptors in a closed UNIX domain
 >> socket).  That resulted in the 'td' argument passed to devfs_close_f()
 >> being NULL, so td->td_fpop would fault.  The patch I have (untested) is
 >> to force devfs_close_f() to always use curthread instead of trusting the
 >> td argument it is given.
 >>
 >> Index: /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c
 >> ===================================================================
 >> --- /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c	(revision
 >> 228311)
 >> +++ /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c	(working
 >> copy)
 >> @@ -602,6 +602,11 @@
 >>   	int error;
 >>   	struct file *fpop;
 >>
 >> +	/*
 >> +	 * NB: td may be NULL if this descriptor is closed due to
 >> +	 * garbage collection from a closed UNIX domain socket.
 >> +	 */
 >> +	td = curthread;
 >>   	fpop = td->td_fpop;
 >>   	td->td_fpop = fp;
 >>   	error = vnops.fo_close(fp, td);
 >>
 > I think you need to use either curthread for td_fpop, or create another
 > local variable td1 and use it for td_fpop stuff. So that the original
 > td is passed to fo_close().
 
 Ah, doh.  I thought I had looked to see if td was used elsewhere.  I 
 will update.
 
 > I am curious whether it would cause further NULL pointer dereference
 > down the stack.
 
 Well, I checked all the other fo_close() methods.  All the other ones
 for PASSABLE fd's are safe.  For vnodes the td is passed to vn_close()
 which only uses it to pass it to VOP_CLOSE().  So, we'd have to audit 
 all the filesystems to see if that is safe.
 
 Updated patch:
 
 Index: /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c
 ===================================================================
 --- /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c	(revision 
 228311)
 +++ /home/jhb/work/freebsd/svn/head/sys/fs/devfs/devfs_vnops.c	(working 
 copy)
 @@ -602,10 +602,14 @@
   	int error;
   	struct file *fpop;
 
 -	fpop = td->td_fpop;
 -	td->td_fpop = fp;
 +	/*
 +	 * NB: td may be NULL if this descriptor is closed due to
 +	 * garbage collection from a closed UNIX domain socket.
 +	 */
 +	fpop = curthread->td_fpop;
 +	curthread->td_fpop = fp;
   	error = vnops.fo_close(fp, td);
 -	td->td_fpop = fpop;
 +	curthread->td_fpop = fpop;
 
   	/*
   	 * The f_cdevpriv cannot be assigned non-NULL value while we
 
 -- 
 John Baldwin