svn commit: r232692 - head/sys/ufs/ffs

[Peter Holm]

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