Date: Fri, 26 Jan 2018 13:43:16 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs Message-ID: <10edbded-b2b5-07e8-b527-b9b4c46d0bae@FreeBSD.org> In-Reply-To: <20180126214948.C1040@besplex.bde.org> References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> <20180126053133.R3207@besplex.bde.org> <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> <20180126214948.C1040@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format.
--------------CD1214954B5C6366568C4C97
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
On 01/26/18 06:36, Bruce Evans wrote:
> On Thu, 25 Jan 2018, Pedro Giffuni wrote:
>
>> On 25/01/2018 14:24, Bruce Evans wrote:
>>> ...
>>> This code only works because (if?) nfs is the only caller and nfs never
>>> passes insane values.
>>>
>>
>> I am starting to think that we should simply match uio_resid and set
>> it to ssize_t.
>> Returning the value to int is certainly not the solution.
>
> Of course using the correct type (int) is part of the solution.
>
int *was* the correct type, now it doesn't cover all the range.
> uio_must be checked before it is used for cookies, and after checking
> it, it
> is small so it fits easily in an int. It must also checked to be
> nonnegative,
Our problem is not really uio_*, our problem is ncookies and we only
test that it is >0.
I think the attached patch, still keeping the unsigned ncookies, is
sufficient.
>
> so that it doesn't suffer unsigned poisoning when it is promoted, so
> it would
> also fit in a u_int, but using u_int to store it is silly as using 1U
> instead
> of 1 for a count of 1.
>
> The bounds checking is something like:
>
> if (ap->uio_resid < 0)
> ap->uio_resid = 0;
> if (ap->a_ncookies != NULL) {
> if (ap->uio_resid >= 64 * 1024)
> ap->uio_resid = 64 * 1024;
> ncookies = ap->uio_resid;
> }
>
> This checks for negative values for all cases and converts to 0 (EOF) to
> preserve historical behaviour for the syscall case and to avoid overflow
> for the cookies case (in case the caller is buggy). The correct handling
> is to return EINVAL, but EOF is good enough.
>
This also touches uio_resid which is probably not good as it is used
later on.
Our job is not to "fix" the caller but only to apply a reasonable behavior.
I also don't like the magic numbers here.
> In the syscall case, uio_resid can be up to SSIZE_MAX, so don't check it
> or corrupt it by assigning it to an int or u_int.
>
The minimal type conversion does not really involve corruption: ncookies
is local
and the caller will not perceive any change.
Pedro.
> Limit uio_resid from above only in the cookies case. The final limit
> should
> be about 128K (whatever nfs uses) or maybe 1M. Don't return EINVAL above
> the limit, since nfs probably wouldn't know how to handle that (by
> retrying
> with a smaller size). Test its handling of short counts instead. It is
> expected than nfs asks for 128K and we supply at most 64K. The supply is
> always reduced at EOF. Hopefully nfs doesn't treat the short count as
> EOF.
> It should retry until we supply 0.
>
> After limiting uio_resid, assign it to the int ncookies.
>
> This doesn't fix the abuse of the ncookies counter to hold the size of
> the
> cookies array in bytes for this and the next couple of statements.
>
> Normally the bounds checking should be at the top level, with at most
> KASSERT()s at lower levels, but here the levels are mixed, and it isn't
> clear if kernel callers have already checked, and it doesn't cost much
> to do much the same checking for the kernel callers as for the syscall
> callers.
>
> Perhaps the 128K limit is good for all cases (this depends on callers not
> having buggy short count handling). Directories of this size are very
> rare (don't forget to create very large ones when you test this). Doing
> anything with directories of this size tends to be slow anyway, and the
> slowness has nothing to do with reading only 128K instead of SSIZE_MAX
> bytes at a time.
>
> readdir() in FreeBSD seems to use a read size of only PAGE_SIZE, except
> in the unionfs case it seems to try to read the whole direction. It
> malloc()s the buffer in both cases. Blindy malloc()ing or mmap()ing
> a buffer large enough for a whole file or directory is no good, since
> in theory even directory sizes can be much larger than memory.
>
> Bruce
--------------CD1214954B5C6366568C4C97
Content-Type: text/x-patch;
name="ext2_ncookies.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="ext2_ncookies.diff"
Index: sys/fs/ext2fs/ext2_lookup.c
===================================================================
--- sys/fs/ext2fs/ext2_lookup.c (revision 328443)
+++ sys/fs/ext2fs/ext2_lookup.c (working copy)
@@ -153,7 +153,10 @@
return (EINVAL);
ip = VTOI(vp);
if (ap->a_ncookies != NULL) {
- ncookies = uio->uio_resid;
+ if (uio->uio_resid < 0)
+ ncookies = 0;
+ else
+ ncookies = uio->uio_resid;
if (uio->uio_offset >= ip->i_size)
ncookies = 0;
else if (ip->i_size - uio->uio_offset < ncookies)
--------------CD1214954B5C6366568C4C97--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?10edbded-b2b5-07e8-b527-b9b4c46d0bae>