From owner-svn-src-head@freebsd.org Sat Jan 27 17:11:23 2018 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 66909ECC38B; Sat, 27 Jan 2018 17:11:23 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id A0FED70DD2; Sat, 27 Jan 2018 17:11:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id BFB2F430B3A; Sun, 28 Jan 2018 04:11:14 +1100 (AEDT) Date: Sun, 28 Jan 2018 04:11:13 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: "Pedro F. Giffuni" , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, svn-src-head@FreeBSD.org Subject: Re: svn commit: r328479 - in head/sys: fs/ext2fs ufs/ufs In-Reply-To: <20180127160340.GB55707@kib.kiev.ua> Message-ID: <20180128031737.B2618@besplex.bde.org> References: <201801271533.w0RFXq0K057921@repo.freebsd.org> <20180127160340.GB55707@kib.kiev.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=LKgWeNe9 c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=2CwwexBGHmdeohTiNo0A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Jan 2018 17:11:23 -0000 On Sat, 27 Jan 2018, Konstantin Belousov wrote: > On Sat, Jan 27, 2018 at 03:33:52PM +0000, Pedro F. Giffuni wrote: >> Log: >> {ext2|ufs}_readdir: Set limit on valid ncookies values. >> >> Sanitize the values that will be assigned to ncookies so that we ensure >> they are sane and we can handle them. >> >> Let ncookies signed as it was before r328346. The valid range is such >> that unsigned values are not required and we are not able to avoid at >> least one cast anyways. >> >> Hinted by: bde >> >> Modified: >> head/sys/fs/ext2fs/ext2_lookup.c >> head/sys/ufs/ufs/ufs_vnops.c >> >> Modified: head/sys/fs/ext2fs/ext2_lookup.c >> ============================================================================== >> --- head/sys/fs/ext2fs/ext2_lookup.c Sat Jan 27 13:46:55 2018 (r328478) >> +++ head/sys/fs/ext2fs/ext2_lookup.c Sat Jan 27 15:33:52 2018 (r328479) >> @@ -145,14 +145,18 @@ ext2_readdir(struct vop_readdir_args *ap) >> off_t offset, startoffset; >> size_t readcnt, skipcnt; >> ssize_t startresid; >> - u_int ncookies; >> + int ncookies; >> int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize; >> int error; >> >> if (uio->uio_offset < 0) >> return (EINVAL); >> ip = VTOI(vp); >> + if (uio->uio_resid < 0) >> + uio->uio_resid = 0; >> if (ap->a_ncookies != NULL) { >> + if (uio->uio_resid > MAXPHYS) >> + uio->uio_resid = MAXPHYS; >> ncookies = uio->uio_resid; >> if (uio->uio_offset >= ip->i_size) >> ncookies = 0; >> >> Modified: head/sys/ufs/ufs/ufs_vnops.c >> ... > You just break nfs server. > > Look at nfsrvd_readdir() call to VOP_READDIR. Almost all code which calls > VOP_READDIR() memoize the value of uio_resid before and after the call and > interpret the difference as the amount of data returned. We noticed in further discussion that there is likely to be a problem like that. Also, the instructions for testing this said to test nfs with a value of 64K and perhaps with smaller values, to see if its error handling can handle reduction of uio_resid. Any testing would have shown the problem. > I said above that only nfs server is broken, because only the server uses > cookies, otherwise your patch would break everything. My original patch have broke syscalls slightly too. In the syscall case, it only modifies uio_resid when that is negative. Spelling 0 as negative would be silly, but it has always worked, and fixing it up to 0 would break callers which memoized the negative value. I think we have use a local variable to hold a reduced resid. uio_resid can be updated just before returning. Or maybe just return EINVAL in the cookies case if uio_resid < 0 || uio_resid > MAX_SIZE_NOW_USED_BY_NFS (thiss essentially a check that nfs is the only caller and that it never asks for large sizes). I was trying to avoid the extra complexity. There are at about 13 file systems under sys/fs, and zfs under sys/cddl to fix. zfs_readdir() is quite broken (if there are any untrusted callers). It always malloc()s approx. (size_t)(int)uio_resid bytes. The casts can overflow in various ways. Some other file systems resuce this to the residual file size which tends to be small enough to fit in memory. zfs detects overrun (using KASSERT()) _after_ the overrun has occurred. The subsystems under sys/fs which mention ncookies are: - autofs: just for layering - cd9660: like zfs, except it has the wrong type (u_int) for the cookie count, and it has no KASSERT() at all, and it counts down the residual cookies quite differently - devfs: doesn't support cookies, but fakes them a little. No problem. - fdesc: doesn't support cookies. No problem. - fuse: doesn't support cookies. Doesn't even return a cookie count of 0 to advertize this like fdesc does. - msdosfs: like cd9660, except it counts down the residual cookies normally - smbfs: doesn't support cookies. Has some code to return EOPNOTSUPP if cookies are requested, but this is ifdefed out, so it is like fuse. - tmpfs: tmpfs_subr.c writes cookies and depends on the caller passing sane cookie pointer and count args. tmpfs_readdir() creates a cookie array with a size that seems to be for a whole directory (not limited by uio_resid). So nfs's saneness doesn't help here. I don't know how large tmpfs directories can be (can it be swap-backed with directories larger than memory?). - udf: like cd9660. - unionfs: does both layering and a malloc() to merge cookies. The size of this malloc() is unclear. Bruce