From owner-svn-src-all@FreeBSD.ORG Fri Mar 9 06:40:40 2012 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 416D41065670 for ; Fri, 9 Mar 2012 06:40:40 +0000 (UTC) (envelope-from pho@holm.cc) Received: from relay03.pair.com (relay03.pair.com [209.68.5.17]) by mx1.freebsd.org (Postfix) with SMTP id 03F6D8FC17 for ; Fri, 9 Mar 2012 06:40:39 +0000 (UTC) Received: (qmail 2802 invoked from network); 9 Mar 2012 06:40:38 -0000 Received: from 87.58.144.241 (HELO x2.osted.lan) (87.58.144.241) by relay03.pair.com with SMTP; 9 Mar 2012 06:40:38 -0000 X-pair-Authenticated: 87.58.144.241 Received: from x2.osted.lan (localhost [127.0.0.1]) by x2.osted.lan (8.14.4/8.14.4) with ESMTP id q296eb5J040101; Fri, 9 Mar 2012 07:40:37 +0100 (CET) (envelope-from pho@x2.osted.lan) Received: (from pho@localhost) by x2.osted.lan (8.14.4/8.14.4/Submit) id q296eaOY040100; Fri, 9 Mar 2012 07:40:36 +0100 (CET) (envelope-from pho) Date: Fri, 9 Mar 2012 07:40:36 +0100 From: Peter Holm To: Bruce Evans Message-ID: <20120309064036.GA40029@x2.osted.lan> References: <201203081249.q28Cn9ed045648@svn.freebsd.org> <20120309145110.D1365@besplex.bde.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20120309145110.D1365@besplex.bde.org> User-Agent: Mutt/1.4.2.3i 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 06:40:40 -0000 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