Date: Fri, 9 Mar 2012 15:51:30 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Peter Holm <pho@FreeBSD.org> 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: <20120309145110.D1365@besplex.bde.org> In-Reply-To: <201203081249.q28Cn9ed045648@svn.freebsd.org> References: <201203081249.q28Cn9ed045648@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
> 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.
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120309145110.D1365>