From owner-svn-src-all@freebsd.org Sat Jan 27 17:21:03 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 9AF85ECC9A8 for ; Sat, 27 Jan 2018 17:21:03 +0000 (UTC) (envelope-from pfg@FreeBSD.org) Received: from sonic302-36.consmr.mail.gq1.yahoo.com (sonic302-36.consmr.mail.gq1.yahoo.com [98.137.68.162]) (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 23393711F7 for ; Sat, 27 Jan 2018 17:21:02 +0000 (UTC) (envelope-from pfg@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=yahoo.com; s=s2048; t=1517073661; bh=MMvKlmA8m5hUSiGJ7l87J7GCoxk48Om4Ow1ILIxgFnY=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From:Subject; b=ktSuWAJDrbUWmC6Vugkq8dP9c6WVJUZWmTUW6fKA3p99jAYNabr34imCxh5n0tzLUDlbxrp0br3KgK6qDneJx/J/Hnaus0eKsaJIfT6oNfMTfcZPSWLSvcUqsXa7dxKsw4SOynADulJAqeT5UlZdBMpErYzc8IwwMSOKIh7O+pyzOOY9v2duWywa4HBzURLiJ5tUe1xf3o+VvxLudYiIZdeiK5ypL4cUhNSJ4wHULOJI9JPfSUg52Tar4Nwf9CQO9UUmdyiYEH2e58VF8mYvzw3C5Bsof7S+bUfcwqG88D2F+buHfP6kq385b/ElwhDZ6FFYjTi2g+uAFTjb9YZchQ== X-YMail-OSG: 6oP_3ngVM1mRP1nXlSJn.ae1t0AZlUO7LqqBfcarN9NvBnngt1FI1p.isvasBkc uXW8Won8VdNhsZ_dvU90h7uwCTzspSgR5B6s2uxr75AQm.R9RFHeK1zSVhgERdu0FaGyz.PfolfQ Lzzh234lpIw625oFE4tqYkwKCvOskL2pf4yOke4X7X_3xyuqytdt76Tsm2LGvG_2BwqtLK7GQjyk 05K5ig1ODeaP0DNqANnS4mpLKb6zO0.gqv6UiF91oe3nP.YzRIqVZBwnrwy1RH2hdSp1nb1kts0k ezp4eb7Z1TFjj4Djea0Is8QP8VxDVJIpysQ227AJOiKbgSbTkWUvMLN5glzOuzzlhWsODdioAMk1 SPqQbONolu1tWAPwrulN3Odc77tA4x3cZb3R0q7irQ25BcwuYeZye6dmt0fTkVRyNIEDwMHvwMzO u1w3uKCg7r40o4RNSxXlI1nXO8THvFp24TDl3KAHVmpBBUhgQEDpNEZcBLs4IIa9xxKhHcSZhAx9 JH.ZzQYJiZg-- Received: from sonic.gate.mail.ne1.yahoo.com by sonic302.consmr.mail.gq1.yahoo.com with HTTP; Sat, 27 Jan 2018 17:21:01 +0000 Received: from smtpgate104.mail.gq1.yahoo.com (EHLO [192.168.0.8]) ([10.215.136.172]) by smtp404.mail.gq1.yahoo.com (JAMES SMTP Server ) with ESMTPA ID 90ce1c4058cfe7e5be6e72abf2723b02; Sat, 27 Jan 2018 17:00:45 +0000 (UTC) Subject: Re: svn commit: r328346 - in head/sys: fs/ext2fs ufs/ffs ufs/ufs To: Warner Losh Cc: Bruce Evans , src-committers , 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> <21b6bdda-65b7-89da-4dd6-bed64978eba8@FreeBSD.org> <20180126214948.C1040@besplex.bde.org> <11937120-bbb4-5da1-f48c-240a6aeafbd9@FreeBSD.org> From: Pedro Giffuni Message-ID: Date: Sat, 27 Jan 2018 12:00:44 -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: Content-Type: multipart/mixed; boundary="------------F212627EF6FB157D50797727" Content-Language: en-US X-Content-Filtered-By: Mailman/MimeDel 2.1.25 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: Sat, 27 Jan 2018 17:21:03 -0000 This is a multi-part message in MIME format. --------------F212627EF6FB157D50797727 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit On 01/27/18 11:14, Warner Losh wrote: > > > On Jan 27, 2018 8:17 AM, "Pedro Giffuni" > 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. --------------F212627EF6FB157D50797727 Content-Type: text/x-patch; name="ufs_ncookies.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="ufs_ncookies.diff" 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; --------------F212627EF6FB157D50797727--