From owner-svn-src-all@freebsd.org Thu Jan 25 18:13:43 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id F06CAEC42A9 for ; Thu, 25 Jan 2018 18:13:42 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic309-35.consmr.mail.gq1.yahoo.com (sonic309-35.consmr.mail.gq1.yahoo.com [98.137.65.161]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 7AA736A67F for ; Thu, 25 Jan 2018 18:13:42 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1516904015; bh=99rfCN4pOC+Bue0RbBql6hzb7SvNxoRIHwRKjoo3DU8=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=ScU+4LBFv6G3m+Pa210vUpZs1pxLP5cCD7GQqqVQ0Qczhptm1FKra5Gf04yilTaUl5ammcRb+u4Md3BujhXWEfPMjfSmSG+2wB1PYqtt6KIaS3vggHsjH5QW0XoURrr/O6F+0ZoO7BXwq5fZhIAFWwD7b6fj5nWKQijMNuRuKgyoYvKaLovZXIs5AF7REHiquv4WJs3LgHlCyI9mlGH9ksgGlsOudl7mLqDH7cKDAh8RXKUR72pr0H2uQANEq9aFRXxgxDh9EjvWujT0h6jIOv3gdj/fPyBHj5Iu/jjMFPQLl4tcjLT/pcH1FLIz/V6m9H6uSe5s3VpmTaIcbi2wyw== X-YMail-OSG: Au5G1kMVM1lMrbf_2t246a09X7qbz7S9waAOiIpcwFZXzo80s4FmT.TrH.uBJEl TwZYjiitzrMJHBlAETVlH6ZMioTO9_46GkPWM6mxgwX_ZrUFGlJXYgUI1x6iFocfJDjpqj3Dio8F hiYPvdvS0_QUdlNOGyJ6EG76eXVkWXnjM9jrBmZ_coAUmHlXk53RUQMqD288TPULOQ7P0Ve8VrJH Tfekg0RTfZK7p67dP8jocl1_Ke54dXzLf6Az5TXn7LyW7TMbaHCqaT05iHv.qcuMJHcFg6qAfZU9 UB0uJu._5Wy_knFbuK7q3y48JADzTOwILMfo2_E16HjqU3DyT5pWXLF1CbIhprCuAJVwacnjQoy8 ELLBC33gLN7J4ekmeoscwqAWF.56CEmoYjN8RJTcNbhl6DjW0nnJ7K.elocq9yTuR7jBizNMtI0s T5sRObcGSOOSLmODCbs.VV_hElVP0.nZny2hIqPHFaQyQ1yECjSSOX.WxMVwz9xWgHnpdKk5QKTY Y.ku_TMxT7A-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic309.consmr.mail.gq1.yahoo.com with HTTP; Thu, 25 Jan 2018 18:13:35 +0000 Received: from smtp104.rhel.mail.gq1.yahoo.com (EHLO [192.168.0.8]) ([216.39.57.214]) by smtp410.mail.gq1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID c694356ccce12c40c41cacc78fbcff2d; Thu, 25 Jan 2018 18:13:32 +0000 (UTC) Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs To: Bruce Evans Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> From: Pedro Giffuni Message-ID: <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> Date: Thu, 25 Jan 2018 13:13:32 -0500 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180126020540.B2181@besplex.bde.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 18:13:43 -0000 On 01/25/18 11:28, Bruce Evans wrote: > On Wed, 24 Jan 2018, Pedro F. Giffuni wrote: > >> Log: >>  ext2fs|ufs:Unsign some values related to allocation. >> >>  When allocating memory through malloc(9), we always expect the >> amount of >>  memory requested to be unsigned as a negative value would either >> stand for >>  an error or an overflow. >>  Unsign some values, found when considering the use of >> mallocarray(9), to >>  avoid unnecessary casting. Also consider that indexes should be of >>  at least the same size/type as the upper limit they pretend to index. > > This might not break much, but it adds many more type errors and bogus > (implicit) casts than it fixes.  It actually changes the brokenness of > the > first variable touched: > >> Modified: head/sys/fs/ext2fs/ext2_lookup.c >> ============================================================================== >> >> --- head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:52:06 2018    >> (r328345) >> +++ head/sys/fs/ext2fs/ext2_lookup.c    Wed Jan 24 17:58:48 2018    >> (r328346) >> @@ -145,9 +145,9 @@ ext2_readdir(struct vop_readdir_args *ap) >>     off_t offset, startoffset; >>     size_t readcnt, skipcnt; >>     ssize_t startresid; >> -    int ncookies; >>     int DIRBLKSIZ = VTOI(ap->a_vp)->i_e2fs->e2fs_bsize; >>     int error; >> +    u_int ncookies; > > The first bug is only a style bug (unsorting the u_int after the ints > even > when its variable was accidentally already before the ints. > Yup, my error. >> >>     if (uio->uio_offset < 0) >>         return (EINVAL); > > That is the only change in this file.  No comment on other changes. > > ncookies is mostly used in contexts other than for multiplying by it > before > calling malloc(), and its type is now inconsistent with most other > places. > > The other places are: > > X     ncookies = uio->uio_resid; > > This has a more serious type error and consequent overflow bugs. > uio_resid > used to have type int, and cookies had to have type int to match that, > else > there overflow occurs before the bounds checks.  Now uio_resid has type > ssize_t, which is excessively large on 64-bit arches (64 bits then), > so the > assignment overflowed when ncookies had type int and uio_resid > INT_MAX. > Now it overflows differently when uio_resid > UINT_MAX, and unsign > extension > causes overflow when uio_resid < 0.  There might be a sanity check on > uio_resid at higher levels, but I can only find a check related to EOF in > vfs_read_dirent(). > I will argue that none of the code in this function is prepared for the eventually of uio->uio_resid < 0 In that case we would have a rather spectacular failure in malloc. Unsigning ncookies is a theoretical, although likely impractical, improvement here. It is not clear to me that using int or u_int makes a difference given it is a local variable and in this scope the signedness of the variable is basically irrelevant. From a logical point of view .. we can't really have a negative number of cookies. > Next, we do some bounds checking which seems to be correct modulo > previous > overflows, and show the care needed for unsigned variables (i_size is > unsigned > and must be compared with uio_offset which is signed). > > Next, we assign ncookies, to ap_ncookies which still has the correct type > (plain signed int).  If ncookies were actually large enough to need a > u_int, > or worse a 64-bit ssize_t, then this would overflow. > > Later, we KASSERT() that ncookies > 0.  This might cause a compiler > warning > "unsigned comparison with 0" now that ncookies is unsigned.  We count > down > ncookies, but the loop termination condition is complicated and we don't > get any benefits from the possible micro-optimization of using > ncookies as > a loop counter that counts down to 0. > > To fix the problem that mallocarray() wanted a size_t arg, ncookies could > be cast to size_t, but that would be silly.  The prototype does the same > cast automatically even for cases with sign mismatches.  Now malloc() > wants a type of size_t (after further breakage to change malloc()s arg > type).  Many conversions are still involved, and casting would at most > limit > compiler warnings:  the code is now: > > X     malloc(ncookies * sizeof(*cookies), ...) > > First, ncookies and sizeof(...) are promoted to a common type. When > ncookies > was int, usually it was promoted to size_t and sizeof(...) was not > promoted. > But on exotic arches with size_t smaller than int, sizeof(...) is > promoted to > int and ncookies is not promoted. > > Next, the type of the result of the multiplication is the common type. > > Finally, the prototype used to convert to u_long, but now converts to > size_t. > When the common type is size_t, then the conversion is now null. When the > common type was int, the conversion was promotion to u_long, but it is > now > demotion to size_t. > > All this only obviously works in practice because all the variables and > the product are small, so they are smaller than all of INT_MAX, SIZE_MAX > and ULONG_MAX on all supported and unsupported arches.  However, it is > hard to write bounds checks that obviously handle all cases, except > by using simple small bounds on the variables. > > It is now obvious that the bounds checking is broken for general use. > There is no obvious limit on the malloc() except the file size. > However, I think cookies are only used by nfs, so this is not a > user-serving bug.  nfs just has to limit its cookie size.  It seems to > use a limit of something like NFS_SRVMAXIO (128K).  This also makes > the overflow from the ssize_t type error unreachable.  Note that it > > So all of this actually works very unobviously in practice.  The sizes > are only small because nfs is the only (?) caller and it never asks for > large sizes. > > ffs has almost the same code, so has the same fragility. > > This fragility goes back to at least FreeBSD-3, but for ffs it was > only in the big-endian case then (this is the case which always had > to copy and convert the on-disk layout).  This changed relatively > recently, because the ffs on-disk layout only matches struct dirent > for 32-bit ino_t. > > Bruce > Pedro.