Date: Sun, 28 Jan 2018 04:11:13 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "Pedro F. Giffuni" <pfg@FreeBSD.org>, 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: <20180128031737.B2618@besplex.bde.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 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180128031737.B2618>