Date: Fri, 26 Jan 2018 06:24:42 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Pedro Giffuni <pfg@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, 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: <20180126053133.R3207@besplex.bde.org> In-Reply-To: <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> References: <201801241758.w0OHwm26063524@repo.freebsd.org> <20180126020540.B2181@besplex.bde.org> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 25 Jan 2018, Pedro Giffuni wrote: This is almost unreadable due to hard-coded UTF-8 (mostly for tabs corrupte= d to spaces) even in previously-literally quoted C code. > On 01/25/18 11:28, Bruce Evans wrote: >> On Wed, 24 Jan 2018, Pedro F. Giffuni wrote: [... Most unreadable lines deleted] >> X=C2=A0=C2=A0=C2=A0=C2=A0 ncookies =3D uio->uio_resid; >>=20 >> This has a more serious type error and consequent overflow bugs. uio_res= id >> used to have type int, and cookies had to have type int to match that, e= lse >> there overflow occurs before the bounds checks.=C2=A0 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= =2E >> Now it overflows differently when uio_resid > UINT_MAX, and unsign=20 >> extension >> causes overflow when uio_resid < 0.=C2=A0 There might be a sanity check = on >> uio_resid at higher levels, but I can only find a check related to EOF i= n >> vfs_read_dirent(). >>=20 > I will argue that none of the code in this function is prepared for the= =20 > eventually of > uio->uio_resid < 0 All of it except the cookies code has to be prepared for that, and seems to handle it OK, since this userland can set uio_resid. The other code is not broken by either the ssize_t expansion or the unsigned bugs, since it mostly doesn't truncate uio_resid by assigning it to a variable of the wrong type (it uses uio->uio_resid in-place, except for copying its initial value to startresid, and startresid is not missing the ssize_t expansion). It mostly does comparisons of the form (uio->uio_resid > 0), where it is 0 in uio_resid means EOF, negative is treated as EOF, and strictly positive means more to do. There is a clear up-front check that uio_offset >=3D 0 (return EINVAL if uio_offset < 0). This is not needed for the trusted nfs caller, but is needed for syscalls and is done for both. > In that case we would have a rather spectacular failure in malloc. > Unsigning ncookies is a theoretical, although likely impractical, improve= ment=20 > here. No, it increases the bug slightly. E.g., if uio_resid is -1, ncookies was -1 / (offsetof(...) + 4) + 1 =3D 0 + 1 after rounding. This might even work (1 cookie at a time, just like if the caller asked for that). Now ncookies is -1U / (offsetof(...) + 4) + 1 =3D a large value. However, if uio_resid was slightly more negative than -2 * (offsetof(...) + 4), then ncookies was -1 and in the multiplication this overflows to -1U =3D a large value and the result is much the same as for earlier overflow on assignment to u_int ncookies. This code only works because (if?) nfs is the only caller and nfs never passes insane values. > It is not clear to me that using int or u_int makes a difference given it= is=20 > a local variable > and in this scope the signedness of the variable is basically irrelevant. It is clear to me that overflow bugs occur with both if untrusted callers a= re allowed. > From a logical point of view .. we can't really have a negative number of= =20 > cookies. Malicious and buggy callers do illogical things to get through holes in your logic. It is also illogical to have a zero number of cookies, but ncookies can be 0 in various ways. First, ncookies is 0 in the EOF case (and cookies are requested). We depend on 0 not being an invalid size for malloc() so that we malloc() nothing and later do more nothings in the main loop. This is a standard use for 0. If you don't like negative numbers, then you also shouldn't like 0. Both exist to make some calculations easier. Later, ncookies is abused as a residual count, so it becomes 0 at the end. This is another standard use for 0. Bruce From owner-svn-src-head@freebsd.org Thu Jan 25 19:57:22 2018 Return-Path: <owner-svn-src-head@freebsd.org> Delivered-To: svn-src-head@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 47C4DEC98D3; Thu, 25 Jan 2018 19:57:22 +0000 (UTC) (envelope-from emaste@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "mxrelay.nyi.freebsd.org", Issuer "Let's Encrypt Authority X3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id F00CC6E3E5; Thu, 25 Jan 2018 19:57:21 +0000 (UTC) (envelope-from emaste@FreeBSD.org) Received: from repo.freebsd.org (repo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:0]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id EB134117A2; Thu, 25 Jan 2018 19:57:21 +0000 (UTC) (envelope-from emaste@FreeBSD.org) Received: from repo.freebsd.org ([127.0.1.37]) by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w0PJvLhg047878; Thu, 25 Jan 2018 19:57:21 GMT (envelope-from emaste@FreeBSD.org) Received: (from emaste@localhost) by repo.freebsd.org (8.15.2/8.15.2/Submit) id w0PJvLNf047877; Thu, 25 Jan 2018 19:57:21 GMT (envelope-from emaste@FreeBSD.org) Message-Id: <201801251957.w0PJvLNf047877@repo.freebsd.org> X-Authentication-Warning: repo.freebsd.org: emaste set sender to emaste@FreeBSD.org using -f From: Ed Maste <emaste@FreeBSD.org> Date: Thu, 25 Jan 2018 19:57:21 +0000 (UTC) To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r328410 - head/usr.sbin/bsdinstall/partedit X-SVN-Group: head X-SVN-Commit-Author: emaste X-SVN-Commit-Paths: head/usr.sbin/bsdinstall/partedit X-SVN-Commit-Revision: 328410 X-SVN-Commit-Repository: base MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for the src tree for head/-current <svn-src-head.freebsd.org> List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/svn-src-head/>; List-Post: <mailto:svn-src-head@freebsd.org> List-Help: <mailto:svn-src-head-request@freebsd.org?subject=help> List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-head>, <mailto:svn-src-head-request@freebsd.org?subject=subscribe> X-List-Received-Date: Thu, 25 Jan 2018 19:57:22 -0000 Author: emaste Date: Thu Jan 25 19:57:21 2018 New Revision: 328410 URL: https://svnweb.freebsd.org/changeset/base/328410 Log: bsdinstall: enable SUJ by default (revert r327890) fsck should be fixed as of r328092. PR: 225110 Tested by: dumbbell, Arshan Khanifar <arshankhanifar gmail.com> Modified: head/usr.sbin/bsdinstall/partedit/gpart_ops.c Modified: head/usr.sbin/bsdinstall/partedit/gpart_ops.c ============================================================================== --- head/usr.sbin/bsdinstall/partedit/gpart_ops.c Thu Jan 25 18:10:33 2018 (r328409) +++ head/usr.sbin/bsdinstall/partedit/gpart_ops.c Thu Jan 25 19:57:21 2018 (r328410) @@ -91,7 +91,8 @@ newfs_command(const char *fstype, char *command, int u {"SU", "Softupdates", "Enable softupdates (default)", 1 }, {"SUJ", "Softupdates journaling", - "Enable file system journaling", 0 }, + "Enable file system journaling (default - " + "turn off for SSDs)", 1 }, {"TRIM", "Enable SSD TRIM support", "Enable TRIM support, useful on solid-state drives", 0 },
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180126053133.R3207>