From owner-svn-src-all@FreeBSD.ORG Fri Mar 9 04:51:43 2012 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 60BC4106566B; Fri, 9 Mar 2012 04:51:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail28.syd.optusnet.com.au (mail28.syd.optusnet.com.au [211.29.133.169]) by mx1.freebsd.org (Postfix) with ESMTP id F188E8FC12; Fri, 9 Mar 2012 04:51:42 +0000 (UTC) Received: from c211-30-171-136.carlnfd1.nsw.optusnet.com.au (c211-30-171-136.carlnfd1.nsw.optusnet.com.au [211.30.171.136]) by mail28.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id q294pYWE013258 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 9 Mar 2012 15:51:35 +1100 Date: Fri, 9 Mar 2012 15:51:30 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Peter Holm In-Reply-To: <201203081249.q28Cn9ed045648@svn.freebsd.org> Message-ID: <20120309145110.D1365@besplex.bde.org> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org Subject: Re: svn commit: r232692 - head/sys/ufs/ffs X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 09 Mar 2012 04:51:43 -0000 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