The following reply was made to PR kern/151758; it has been noted by GNATS.
From: John Baldwin <j...@freebsd.org>
To: Kostik Belousov <kostik...@gmail.com>
Cc: bug-follo...@freebsd.org, and...@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
_______________________________________________
freebsd-bugs@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-bugs
To unsubscribe, send any mail to "freebsd-bugs-unsubscr...@freebsd.org"
