Date: Fri, 9 Mar 2012 07:40:36 +0100 From: Peter Holm <peter@holm.cc> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r232692 - head/sys/ufs/ffs Message-ID: <20120309064036.GA40029@x2.osted.lan> In-Reply-To: <20120309145110.D1365@besplex.bde.org> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Mar 09, 2012 at 03:51:30PM +1100, Bruce Evans wrote:
> On Thu, 8 Mar 2012, Peter Holm wrote:
>
> >Log:
> > syscall() fuzzing can trigger this panic. Return EINVAL instead.
> >
> > MFC after: 1 week
>
> If so, then, this is not the place to hide the bug.
>
OK
> >Modified: head/sys/ufs/ffs/ffs_vnops.c
> >==============================================================================
> >--- head/sys/ufs/ffs/ffs_vnops.c Thu Mar 8 11:05:53 2012 (r232691)
> >+++ head/sys/ufs/ffs/ffs_vnops.c Thu Mar 8 12:49:08 2012 (r232692)
> >@@ -464,11 +464,11 @@ ffs_read(ap)
> > } else if (vp->v_type != VREG && vp->v_type != VDIR)
> > panic("ffs_read: type %d", vp->v_type);
> >#endif
> >+ if (uio->uio_resid < 0 || uio->uio_offset < 0)
> >+ return (EINVAL);
> > orig_resid = uio->uio_resid;
> >- KASSERT(orig_resid >= 0, ("ffs_read: uio->uio_resid < 0"));
> > if (orig_resid == 0)
> > return (0);
> >- KASSERT(uio->uio_offset >= 0, ("ffs_read: uio->uio_offset < 0"));
> > fs = ip->i_fs;
> > if (uio->uio_offset < ip->i_size &&
> > uio->uio_offset >= fs->fs_maxfilesize)
>
> All file systems are supposed to copy ffs here. The ones that actually
> do are still correct here.
>
> The code that enforces a non-negative uio_resid for read(2) is:
>
> % #ifndef _SYS_SYSPROTO_H_
> % struct read_args {
> % int fd;
> % void *buf;
> % size_t nbyte;
> % };
> % #endif
> % int
> % sys_read(td, uap)
> % struct thread *td;
> % struct read_args *uap;
> % {
> % struct uio auio;
> % struct iovec aiov;
> % int error;
> %
> % if (uap->nbyte > IOSIZE_MAX)
> % return (EINVAL);
>
> uap->nbyte is unsigned, so it is never negative. IOSIZE_MAX is either
> INT_MAX or SSIZE_MAX.
>
> % aiov.iov_base = uap->buf;
> % aiov.iov_len = uap->nbyte;
>
> iov_len has type size_t, so it can represent any value of nbytes, since
> that has type size_t too.
>
> % auio.uio_iov = &aiov;
> % auio.uio_iovcnt = 1;
> % auio.uio_resid = uap->nbyte;
>
> uio_resid has type ssize_t, so it can represent any value of nbyte less
> than IOSIZE_MAX. Thus no overflow occurs on this assignment, else there
> would be undefined behaviour and perhaps a negative value here.
>
> % auio.uio_segflg = UIO_USERSPACE;
> % error = kern_readv(td, uap->fd, &auio);
> % return(error);
> % }
>
> kib is supposed to have been careful converting the old int types and
> INT_MAX limit to ssize_t types and SSIZE_MAX limit, so that negative
> values remain impossible. They should be so impossible that asserting
> that they don't happen in VOPs is as useful as asserting that parity
> errors in the CPU don't happen, but if one happens then the fix is not
> to remove the assertion.
>
> Negative offsets seem to be disallowed correctly in sys_lseek() and
> sys_preadv().
>
> syscall() can be used to pass weirder args than usual, but I don't
> know any way to reach ffs_read() without going through the checking
> in sys_lseek() or sys_preadv(). A stack trace of the panic would be
> useful for showing how it got through.
>
This specific problem here was discover by:
for (i = 0; i < 2000; i++) {
if ((dirp = opendir(path)) == NULL)
break;
bcopy(dirp, &fuzz, sizeof(fuzz));
fuzz.dd_len = arc4random();
readdir(&fuzz);
closedir(dirp);
}
http://people.freebsd.org/~pho/stress/log/kostik478.txt
> There seems to be no checking of the non-negativeness of the arg for
> mmap(). That is almost correct, since the offset for mmap() is for
> memory, so off_t and its signedness are bogus anyway.
>
> sys_sendfile() checks.
>
> Bruce
--
Peter
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120309064036.GA40029>