svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs
Pedro Giffuni
pfg at FreeBSD.org
Sat Jan 27 17:10:56 UTC 2018
On 01/27/18 11:14, Warner Losh wrote:
>
>
> On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <pfg at freebsd.org
> <mailto:pfg at freebsd.org>> wrote:
>
>
>
> 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.
>
> 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,
> 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.
>
> 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.
>
> 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.
>
> Hmm ...
>
> We have never checked the upper bound there, which doesn't mean it
> was right.
> I found MAXPHYS, which seems a more reasonable limit used in the
> kernel for uio_resid.
>
> I am checking the patch compiles and doesn't give surprises.
>
>
> MAXPHYS is almost the right thing to check. There's per device limits
> for normal I/O, but that doesn't seem to be a strict limit for readdir.
>
OK... new patch, this time again trying to sanitize only ncookies (which
can be int or u_int, doesn't matter to me).
Pedro.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ufs_ncookies.diff
Type: text/x-patch
Size: 1392 bytes
Desc: not available
URL: <http://lists.freebsd.org/pipermail/svn-src-head/attachments/20180127/3e17256d/attachment.bin>
More information about the svn-src-head
mailing list