svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs

[Pedro Giffuni]

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>