Date: Sat, 27 Jan 2018 12:00:44 -0500 From: Pedro Giffuni <pfg@FreeBSD.org> To: Warner Losh <imp@bsdimp.com> Cc: Bruce Evans <brde@optusnet.com.au>, src-committers <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: <f159b531-8df1-c24e-0255-a9c2338b2e03@FreeBSD.org> In-Reply-To: <CANCZdfqq7eS5qMWVrG=U0aJJuNc%2BGmqvxa9=_0BbNr0tAqpx=Q@mail.gmail.com> 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> <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org> <CANCZdfqq7eS5qMWVrG=U0aJJuNc%2BGmqvxa9=_0BbNr0tAqpx=Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
On 01/27/18 11:14, Warner Losh wrote:
>
>
> On Jan 27, 2018 8:17 AM, "Pedro Giffuni" <pfg@freebsd.org
> <mailto:pfg@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.
[-- Attachment #2 --]
Index: sys/fs/ext2fs/ext2_lookup.c
===================================================================
--- sys/fs/ext2fs/ext2_lookup.c (revision 328478)
+++ sys/fs/ext2fs/ext2_lookup.c (working copy)
@@ -145,7 +145,7 @@
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;
@@ -152,7 +152,11 @@
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;
Index: sys/ufs/ufs/ufs_vnops.c
===================================================================
--- sys/ufs/ufs/ufs_vnops.c (revision 328478)
+++ sys/ufs/ufs/ufs_vnops.c (working copy)
@@ -2170,7 +2170,7 @@
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 @@
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;
ncookies = uio->uio_resid;
if (uio->uio_offset >= ip->i_size)
ncookies = 0;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?f159b531-8df1-c24e-0255-a9c2338b2e03>