Bug 94278 - Crash with FIFOs and ktrace
Summary: Crash with FIFOs and ktrace
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 6.1-PRERELEASE
Hardware: Any Any
: Normal Affects Only Me
Assignee: Robert Watson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-03-09 16:10 UTC by Martin
Modified: 2006-06-12 12:40 UTC (History)
0 users

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin 2006-03-09 16:10:03 UTC
In the German Forum BSDForen.de (link: http://www.bsdforen.de/showthread.php?p=115505#post115505)
a poster named tib has found a situation where produces a very nice crash.

The crash is reproducible on 6.0R upto 6.1-PRERELEASE and confirmed by other posters. It involves reading from a FIFO by using ktrace and kdump. You can reproduce it with user privileges.

How-To-Repeat: mkfifo ktest
ktrace -f ktest -a date
kdump -f ktest
Comment 1 Kostik Belousov 2006-03-14 12:01:53 UTC
On Tue, Mar 14, 2006 at 04:41:32PM +0800, David Xu wrote:
> On Tuesday 14 March 2006 15:27, Martin wrote:
> > 
> > David Xu wrote:
> > 
> > > Can anyone add this to 6.1 todo list ? this definitely should be fixed 
> before
> > > 6.1R.
> > 
> > One of my friends also has found kern/94278:
> > http://www.freebsd.org/cgi/query-pr.cgi?pr=94278
> > 
> > There is no comment on it so far. This crash (without panic)
> > is not less important, in my opinion.
> > 
> > Martin
> 
> Yeah, fifo refuses to work if the caller did not allocate a FILE structure
> for it, but ktrace insists that it should work without a FILE, it believes
> a vnode is enough for everything,  I am really tired of such arch breakage.
> 
> David Xu


In fact, the problem affects most places where kernel tries writing
to the file, because usually code does not allocate file descriptor
for write, but uses direct vnode access. I found approximately a dozen
such places. BTW, the case for fhopen seems to be remotely exploitable.

Long-term fix would be to correctly integrate fifo into VFS instead
of overloading file op structure for vnodes.

For now, please, try the following patch:

Index: compat/linux/linux_misc.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/compat/linux/linux_misc.c,v
retrieving revision 1.172
diff -u -r1.172 linux_misc.c
--- compat/linux/linux_misc.c	28 Dec 2005 07:08:54 -0000	1.172
+++ compat/linux/linux_misc.c	14 Mar 2006 11:45:57 -0000
@@ -310,6 +310,21 @@
 	 * XXX: This should use vn_open() so that it is properly authorized,
 	 * and to reduce code redundancy all over the place here.
 	 */
+	if (vp->v_type == VLNK) {
+		error = EMLINK;
+		goto cleanup;
+	}
+	if (vp->v_type == VSOCK) {
+		error = EOPNOTSUPP;
+		goto cleanup;
+	}
+	if (vp->v_type == VFIFO) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto cleanup;
+	}
 #ifdef MAC
 	error = mac_check_vnode_open(td->td_ucred, vp, FREAD);
 	if (error)
Index: fs/fifofs/fifo_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
retrieving revision 1.132
diff -u -r1.132 fifo_vnops.c
--- fs/fifofs/fifo_vnops.c	1 Oct 2005 20:15:41 -0000	1.132
+++ fs/fifofs/fifo_vnops.c	14 Mar 2006 11:46:07 -0000
@@ -168,6 +168,7 @@
 		int  a_mode;
 		struct ucred *a_cred;
 		struct thread *a_td;
+		int a_fdidx;
 	} */ *ap;
 {
 	struct vnode *vp = ap->a_vp;
Index: kern/vfs_syscalls.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.411
diff -u -r1.411 vfs_syscalls.c
--- kern/vfs_syscalls.c	4 Mar 2006 00:09:09 -0000	1.411
+++ kern/vfs_syscalls.c	14 Mar 2006 11:46:10 -0000
@@ -4101,6 +4101,13 @@
 		error = EOPNOTSUPP;
 		goto bad;
 	}
+	if (vp->v_type == VFIFO) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto bad;
+	}
 	mode = 0;
 	if (fmode & (FWRITE | O_TRUNC)) {
 		if (vp->v_type == VDIR) {
Index: kern/vfs_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.238
diff -u -r1.238 vfs_vnops.c
--- kern/vfs_vnops.c	11 Mar 2006 17:14:05 -0000	1.238
+++ kern/vfs_vnops.c	14 Mar 2006 11:46:10 -0000
@@ -194,6 +194,13 @@
 		error = EOPNOTSUPP;
 		goto bad;
 	}
+	if ((vp->v_type == VFIFO) && (fdidx < 0)) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto bad;
+	}
 	mode = 0;
 	if (fmode & (FWRITE | O_TRUNC)) {
 		if (vp->v_type == VDIR) {
Comment 2 Robert Watson freebsd_committer freebsd_triage 2006-03-14 12:26:08 UTC
Responsible Changed
From-To: freebsd-bugs->rwatson


Grab ownership, since I've also been looking at this.  David, feel free to 
grab it if you'd prefer. 

My reading was that actually, the fifo code is at fault for not checking 
if the fdidx argument to vop_open() is -1, which implies there isn't a 
file descriptor in use, and that fifos there fore can't hook up to it. 
This error path may require some tweaking -- I tried having Kris add a 
patch along these lines, and he reported problems that I haven't yet 
investigated. 

Being able to feed ktrace data to a fifo is a useful thing, but will be 
difficult to re-add in the world of vnode bypass, something we might want 
to add a todo item for sometime.
Comment 3 Kostik Belousov 2006-03-14 13:02:42 UTC
Sorry for garbled patch. I do not know why mutt decided to encode
some "=" as =3D.


Index: compat/linux/linux_misc.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/compat/linux/linux_misc.c,v
retrieving revision 1.172
diff -u -r1.172 linux_misc.c
--- compat/linux/linux_misc.c	28 Dec 2005 07:08:54 -0000	1.172
+++ compat/linux/linux_misc.c	14 Mar 2006 11:45:57 -0000
@@ -310,6 +310,21 @@
 	 * XXX: This should use vn_open() so that it is properly authorized,
 	 * and to reduce code redundancy all over the place here.
 	 */
+	if (vp->v_type == VLNK) {
+		error = EMLINK;
+		goto cleanup;
+	}
+	if (vp->v_type == VSOCK) {
+		error = EOPNOTSUPP;
+		goto cleanup;
+	}
+	if (vp->v_type == VFIFO) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto cleanup;
+	}
 #ifdef MAC
 	error = mac_check_vnode_open(td->td_ucred, vp, FREAD);
 	if (error)
Index: fs/fifofs/fifo_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
retrieving revision 1.132
diff -u -r1.132 fifo_vnops.c
--- fs/fifofs/fifo_vnops.c	1 Oct 2005 20:15:41 -0000	1.132
+++ fs/fifofs/fifo_vnops.c	14 Mar 2006 11:46:07 -0000
@@ -168,6 +168,7 @@
 		int  a_mode;
 		struct ucred *a_cred;
 		struct thread *a_td;
+		int a_fdidx;
 	} */ *ap;
 {
 	struct vnode *vp = ap->a_vp;
Index: kern/vfs_syscalls.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_syscalls.c,v
retrieving revision 1.411
diff -u -r1.411 vfs_syscalls.c
--- kern/vfs_syscalls.c	4 Mar 2006 00:09:09 -0000	1.411
+++ kern/vfs_syscalls.c	14 Mar 2006 11:46:10 -0000
@@ -4101,6 +4101,13 @@
 		error = EOPNOTSUPP;
 		goto bad;
 	}
+	if (vp->v_type == VFIFO) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto bad;
+	}
 	mode = 0;
 	if (fmode & (FWRITE | O_TRUNC)) {
 		if (vp->v_type == VDIR) {
Index: kern/vfs_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_vnops.c,v
retrieving revision 1.238
diff -u -r1.238 vfs_vnops.c
--- kern/vfs_vnops.c	11 Mar 2006 17:14:05 -0000	1.238
+++ kern/vfs_vnops.c	14 Mar 2006 11:46:10 -0000
@@ -194,6 +194,13 @@
 		error = EOPNOTSUPP;
 		goto bad;
 	}
+	if ((vp->v_type == VFIFO) && (fdidx < 0)) {
+		/* Due to way fifo works (by overloading f_ops),
+		 * tricking kernel into write to the fifo leads to
+		 * panic. Make a band-aid to filter the case. */
+		error = EOPNOTSUPP;
+		goto bad;
+	}
 	mode = 0;
 	if (fmode & (FWRITE | O_TRUNC)) {
 		if (vp->v_type == VDIR) {
Comment 4 David Xu freebsd_committer freebsd_triage 2006-03-14 13:17:49 UTC
On Tuesday 14 March 2006 21:02, Kostik Belousov wrote:
> Sorry for garbled patch. I do not know why mutt decided to encode
> some "=" as =3D.
> 
> 
> Index: compat/linux/linux_misc.c
> ===================================================================
> RCS file: /usr/local/arch/ncvs/src/sys/compat/linux/linux_misc.c,v
> retrieving revision 1.172
> diff -u -r1.172 linux_misc.c
> --- compat/linux/linux_misc.c	28 Dec 2005 07:08:54 -0000	1.172
> +++ compat/linux/linux_misc.c	14 Mar 2006 11:45:57 -0000
> @@ -310,6 +310,21 @@
>  	 * XXX: This should use vn_open() so that it is properly authorized,
>  	 * and to reduce code redundancy all over the place here.
>  	 */
> +	if (vp->v_type == VLNK) {
> +		error = EMLINK;
> +		goto cleanup;
> +	}
> +	if (vp->v_type == VSOCK) {
> +		error = EOPNOTSUPP;
> +		goto cleanup;
> +	}
> +	if (vp->v_type == VFIFO) {
> +		/* Due to way fifo works (by overloading f_ops),
> +		 * tricking kernel into write to the fifo leads to
> +		 * panic. Make a band-aid to filter the case. */
> +		error = EOPNOTSUPP;
> +		goto cleanup;
> +	}
>  #ifdef MAC
>  	error = mac_check_vnode_open(td->td_ucred, vp, FREAD);
>  	if (error)
> Index: fs/fifofs/fifo_vnops.c
> ===================================================================
> RCS file: /usr/local/arch/ncvs/src/sys/fs/fifofs/fifo_vnops.c,v
> retrieving revision 1.132
> diff -u -r1.132 fifo_vnops.c
> --- fs/fifofs/fifo_vnops.c	1 Oct 2005 20:15:41 -0000	1.132
> +++ fs/fifofs/fifo_vnops.c	14 Mar 2006 11:46:07 -0000
> @@ -168,6 +168,7 @@
>  		int  a_mode;
>  		struct ucred *a_cred;
>  		struct thread *a_td;
> +		int a_fdidx;
>  	} */ *ap;
>  {
>  	struct vnode *vp = ap->a_vp;
> Index: kern/vfs_syscalls.c
> ===================================================================
> RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_syscalls.c,v
> retrieving revision 1.411
> diff -u -r1.411 vfs_syscalls.c
> --- kern/vfs_syscalls.c	4 Mar 2006 00:09:09 -0000	1.411
> +++ kern/vfs_syscalls.c	14 Mar 2006 11:46:10 -0000
> @@ -4101,6 +4101,13 @@
>  		error = EOPNOTSUPP;
>  		goto bad;
>  	}
> +	if (vp->v_type == VFIFO) {
> +		/* Due to way fifo works (by overloading f_ops),
> +		 * tricking kernel into write to the fifo leads to
> +		 * panic. Make a band-aid to filter the case. */
> +		error = EOPNOTSUPP;
> +		goto bad;
> +	}
>  	mode = 0;
>  	if (fmode & (FWRITE | O_TRUNC)) {
>  		if (vp->v_type == VDIR) {
> Index: kern/vfs_vnops.c
> ===================================================================
> RCS file: /usr/local/arch/ncvs/src/sys/kern/vfs_vnops.c,v
> retrieving revision 1.238
> diff -u -r1.238 vfs_vnops.c
> --- kern/vfs_vnops.c	11 Mar 2006 17:14:05 -0000	1.238
> +++ kern/vfs_vnops.c	14 Mar 2006 11:46:10 -0000
> @@ -194,6 +194,13 @@
>  		error = EOPNOTSUPP;
>  		goto bad;
>  	}
> +	if ((vp->v_type == VFIFO) && (fdidx < 0)) {
> +		/* Due to way fifo works (by overloading f_ops),
> +		 * tricking kernel into write to the fifo leads to
> +		 * panic. Make a band-aid to filter the case. */
> +		error = EOPNOTSUPP;
> +		goto bad;
> +	}
>  	mode = 0;
>  	if (fmode & (FWRITE | O_TRUNC)) {
>  		if (vp->v_type == VDIR) {
> 

I know, someone will work out such a messy patch, but is it reasonable ?
why does not the fifi code suddenly work with well defined vnode interface ?
why did someone want to break the well defined FILE->vnode->fs->device
layers ? sigh.

David Xu
Comment 5 Kostik Belousov 2006-03-14 13:41:42 UTC
On Tue, Mar 14, 2006 at 09:17:49PM +0800, David Xu wrote:
> I know, someone will work out such a messy patch, but is it reasonable ?
> why does not the fifi code suddenly work with well defined vnode interface ?
> why did someone want to break the well defined FILE->vnode->fs->device
> layers ? sigh.


From CVS history for sys/fs/fifofs//fifo_vnops.c:

Revision 1.105 Wed Nov 17 07:30:02 2004 UTC (15 months, 3 weeks ago) by phk

 Make vnode bypass for fifos (read, write, poll) mandatory.

Revision 1.104 Mon Nov 15 14:51:44 2004 UTC (15 months, 3 weeks ago) by phk

 Add file ops to fifofs so that we can bypass vnodes (and Giant) for the
 heavy-duty operations (read, write, poll/select, kqueue).

 Disabled for now, enable with "vfs.fifofs.fops=1" in loader.conf.
Comment 6 Robert Watson freebsd_committer freebsd_triage 2006-03-14 19:28:59 UTC
State Changed
From-To: open->patched

Change to patched as I have committed an a_fdidx check to fifo_open() 
so that EINVAL is returned if fifofs cannot set up the file descriptor 
bypass.  The revision for this change is fifo_vnops.c:1.133. 

A related change, ktrace.c:1.21, adds a similar check to user space 
so as to provide a more informative error message, and also passes 
O_NONBLOCK so that ktrace won't stall when opening a fifo.  We might 
want to map the errno from fifofs into a more user friendly message. 

This doesn't restore the ability to ktrace to a fifo; there are some 
complexities associated with this, especially in 5.x where the ktrace 
worker is shared across multiple processes and might block 
indefinitely if pointed at a fifo.  Indefinite blocking might also be 
a problem in 6.x, but should be investigated since ktrace now uses 
a different worker model for asynchronous commit. 

These changes are set for MFC in a few days with the intent of 
including them in the 5.5 and 6.1 releases.
Comment 7 Robert Watson freebsd_committer freebsd_triage 2006-06-12 12:36:26 UTC
State Changed
From-To: patched->closed

Close: changes were merged as fifo_vnops.c:1.113.2.18 on 2006/03/20 
and ktrace.c:1.20.12.1 on 2006/03/20.  Changes should be present in 
FreeBSD 6.1-RELEASE.