From owner-svn-src-head@freebsd.org Thu Jan 25 20:01:25 2018 Return-Path: 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 E121DEC9E62 for ; Thu, 25 Jan 2018 20:01:24 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic317-41.consmr.mail.bf2.yahoo.com (sonic317-41.consmr.mail.bf2.yahoo.com [74.6.129.96]) (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 89E6F6E9FB for ; Thu, 25 Jan 2018 20:01:24 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1516910478; bh=0DOsI0ZeuDKlfnmWnd5p4S0CsOI4me/K08lwWACkK4c=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=pD4q6g7MuZOvy9RIF6vwRTmHDPxUFdlct6W6pud0LYmfghXMSP+/tT92gZPaSXa7+ys4cLfIQNQm1gvSOhM5VCVKV/IRceB8mwPPF/fb9Z85C5bZU0UQSipq5k2PDWxlD1FNIof6Kzc53bOP1sEJjHZ8McNEj5ADo78K0/YbErsxtdLZGKrwP1miXI2zkn+K1aLcRHa7p047gzC1o/EuDOmyXiZjQEAsZHZ8O5m0t72NZKNWsM1vrlhnHqgl76q+SQwAw3uHTkfjRIrLhGggBwb8+S1g4ppD+Njdks+ePYguNhFQc02OyXxk2fz1BxZudp8nI1nCGVXfNg4w6ArXCA== X-YMail-OSG: TtNLmjsVM1mN.3.p0MVqhkMOx6IYjJT4p1.wEZAJmq9WlqJsWX1773jig.8eLym QN6qNB_fmhINz8Kw2QkcCwuv88BSZhBdIcG9C1q9OQo.kcqV3QwYCDybqeCOxh2yOBb8Gf0jhHNM TBg6kyODChNfTzJ7R8e5aj6lu47dmJVhzukTaLp3QMnEG4.3KFcSxw4XOWeLi99G0WSZYqEHN_vY LRJ8edmW.4UvgBn6q0i1f6_NpF_syBe4sNe8.oZ3WPRCrx2a0Zzwri8SIw3Oy2eOiEKBIQ5LNu49 TcYQeXm.9DJHzHG2dEVDuXdf6BzEfe14Ell2iDoiqzv7C_54b6Lx5wEXIfTQOH3locdeyFa2kYJf Zbe_ZChRbhYKCGJcaZxHSyJue4N7EllwTjllojFlXJrWZOuahcrcmovit0fkyrmNaM4R3wsMR7U1 RVwmSz8iL_a0S5sFA07pDMZU73iWD3Z.lBVVhMZ1XNs1CwZ06z1Vot.jjvnoLtV5WYqJQiyNQkFr n4IgOV1zZcw-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic317.consmr.mail.bf2.yahoo.com with HTTP; Thu, 25 Jan 2018 20:01:18 +0000 Received: from smtp105.rhel.mail.bf1.yahoo.com (EHLO [192.168.0.8]) ([98.139.231.38]) by smtp412.mail.bf1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID a53f401ffab560e1068aa3e11952352a; Thu, 25 Jan 2018 20:01:14 +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> <8d5ddd06-14b2-e7e1-14dd-5e9d42f9b33c@FreeBSD.org> <20180126053133.R3207@besplex.bde.org> From: Pedro Giffuni Organization: FreeBSD Project Message-ID: <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> Date: Thu, 25 Jan 2018 15:01:15 -0500 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180126053133.R3207@besplex.bde.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Jan 2018 20:01:25 -0000 On 25/01/2018 14:24, Bruce Evans wrote: > On Thu, 25 Jan 2018, Pedro Giffuni wrote: > > This is almost unreadable due to hard-coded UTF-8 (mostly for tabs > corrupted > to spaces) even in previously-literally quoted C code. > Mailer agents ... they all suck :( >> On 01/25/18 11:28, Bruce Evans wrote: >>> On Wed, 24 Jan 2018, Pedro F. Giffuni wrote: > > [... Most unreadable lines deleted] > >>> 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 > > 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 >= 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, >> improvement here. > > No, it increases the bug slightly.  E.g., if uio_resid is -1, ncookies > was -1 / (offsetof(...) + 4) + 1 = 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 = 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 = 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. > 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. >> 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. > > It is clear to me that overflow bugs occur with both if untrusted > callers are > allowed. > >> From a logical point of view .. we can't really have a negative >> number of 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