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
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) {
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.
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) {
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
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.
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.
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.