Date: Sat, 27 Jan 2018 11:43:13 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Konstantin Belousov <kostikbel@gmail.com> Cc: 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 Message-ID: <5335b45b-f527-c388-91fc-243d67511534@FreeBSD.org> In-Reply-To: <20180127160340.GB55707@kib.kiev.ua> References: <201801271533.w0RFXq0K057921@repo.freebsd.org> <20180127160340.GB55707@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On 01/27/18 11:03, Konstantin Belousov wrote: > On Sat, Jan 27, 2018 at 03:33:52PM +0000, Pedro F. Giffuni wrote: >> Author: pfg >> Date: Sat Jan 27 15:33:52 2018 >> New Revision: 328479 >> URL: https://svnweb.freebsd.org/changeset/base/328479 >> >> 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 >> ============================================================================== >> --- head/sys/ufs/ufs/ufs_vnops.c Sat Jan 27 13:46:55 2018 (r328478) >> +++ head/sys/ufs/ufs/ufs_vnops.c Sat Jan 27 15:33:52 2018 (r328479) >> @@ -2170,7 +2170,7 @@ ufs_readdir(ap) >> off_t offset, startoffset; >> size_t readcnt, skipcnt; >> ssize_t startresid; >> - u_int ncookies; >> + int ncookies; >> int error; >> >> if (uio->uio_offset < 0) >> @@ -2178,7 +2178,11 @@ ufs_readdir(ap) >> ip = VTOI(vp); >> if (ip->i_effnlink == 0) >> return (0); >> + if (uio->uio_resid < 0) >> + uio->uio_resid = 0; >> if (ap->a_ncookies != NULL) { >> + if (uio->uio_resid > MAXPHYS) >> + uio->uio_resid = MAXPHYS; > 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. > > I said above that only nfs server is broken, because only the server uses > cookies, otherwise your patch would break everything. Ugh, yes .. I completely missed the fact that uio is a pointer to the real thing, not a local copy. This still should never go off limits but it is certainly wrong. I reverted it sorry. Pedro.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?5335b45b-f527-c388-91fc-243d67511534>