Date: Sat, 27 Jan 2018 23:12:50 +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: <20180127214500.I925@besplex.bde.org> In-Reply-To: <10edbded-b2b5-07e8-b527-b9b4c46d0bae@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> <10edbded-b2b5-07e8-b527-b9b4c46d0bae@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, 26 Jan 2018, Pedro Giffuni wrote:
> On 01/26/18 06:36, Bruce Evans wrote:
>> On Thu, 25 Jan 2018, Pedro Giffuni wrote:
>>=20
>>> On 25/01/2018 14:24, Bruce Evans wrote:
>>>> ...
>>>> This code only works because (if?) nfs is the only caller and nfs neve=
r
>>>> passes insane values.
>>>>=20
>>>=20
>>> I am starting to think that we should simply match uio_resid and set it=
to=20
>>> ssize_t.
>>> Returning the value to int is certainly not the solution.
>>=20
>> Of course using the correct type (int) is part of the solution.
>>=20
> int *was* the correct type, now it doesn't cover all the range.
int is the correct type. The range is small after range checking.
u_int is an incorrect type. It can only hold SSIZE_MAX on __LP32_ arches,
and that only after half-baked range checking for the lower limit only.
ssize_t is an incorrect type. It can hold SSIZE_MAX on all arches, but
that doesn't prevent various overflow bugs or requests to malloc()
SSIZE_MAX (plus a little more for rounding up) bytes unless there is
some range checking.
> Our problem is not really uio_*, our problem is ncookies and we only test=
=20
> that it is >0.
Our problem is lack of understanding of lectures on C types and range check=
ing.
> I think the attached patch, still keeping the unsigned ncookies, is=20
> sufficient.
X Index: sys/fs/ext2fs/ext2_lookup.c
X =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
X --- sys/fs/ext2fs/ext2_lookup.c=09(revision 328443)
X +++ sys/fs/ext2fs/ext2_lookup.c=09(working copy)
X @@ -153,7 +153,10 @@
X =09=09return (EINVAL);
X =09ip =3D VTOI(vp);
X =09if (ap->a_ncookies !=3D NULL) {
X -=09=09ncookies =3D uio->uio_resid;
X +=09=09if (uio->uio_resid < 0)
X +=09=09=09ncookies =3D 0;
X +=09=09else
X +=09=09=09ncookies =3D uio->uio_resid;
This only avoids overflow for negative values. It doesn't limit uio_resid
from above, so overflow on this assignment can occur on __LP64__ arches.
Even on __LP32__ systems, SSIZE_MAX =3D 2G is too large to work. My patch
limits uio_resid to avoid this and many other bugs.
When overflow occurs, this gives the same bugs as before, but they are
larger than I noticed before. E.g., uio->uio_resid =3D=3D 1ULL << 32, then
the overflow is to 0. There is no problem allocating a cookies array
with 0 elements, but since you didn't adjust uio_resid it is inconsistent
with ncookies. Without INVARIANTS, buffer overrun occurs when the first
dirent is read and cookies[0] is modified to point to the dirent; then
ncookies was decremented from 0 to -1, but after you broke its type it
is decremented from 0 to UINT_MAX. With INVARIANTS, the bug is detected
by a KASSERT() that ncookies > 0.
X =09=09if (uio->uio_offset >=3D ip->i_size)
X =09=09=09ncookies =3D 0;
X =09=09else if (ip->i_size - uio->uio_offset < ncookies)
When uio_resid is huge, that isn't usually a problem unless assigning it
to ncookies gives a small value. Otherwise, ncookies is at least large
here. Then it is usually larger than the residual file size, so it gets
replaced by the residual file size. This is only too large to fit in
memory if the residual file size is large.
>> The bounds checking is something like:
>> [... context lost to corruption of spaces to binary (UTF-8))
>> 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).=C2=A0 The correct ha=
ndling
>> is to return EINVAL, but EOF is good enough.
>>=20
> This also touches uio_resid which is probably not good as it is used late=
r=20
> on.
> Our job is not to "fix" the caller but only to apply a reasonable behavio=
r.
This indeed too fragile. I had throught that syscalls adjusted uio_resid
at the end (so could adjust it at the beginning too). They actually just
calculate nbytes =3D initial_uio_resid - final_uio_resid.
But if you don't adjust uio_resid, then you get buffer overruns when there
are more dirents than cookies. dirents are not malloc()ed, so there is no
limit on them except uio_resid, the size that can be malloc()ed for cookies=
,
and the residual file size.
>> 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.
>
> The minimal type conversion does not really involve corruption: ncookies =
is=20
> local
> and the caller will not perceive any change.
The value is corrupted by blind assignmemt to an unrelated type.
Bruce
From owner-svn-src-all@freebsd.org Sat Jan 27 12:28:53 2018
Return-Path: <owner-svn-src-all@freebsd.org>
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 B4A51EBD765;
Sat, 27 Jan 2018 12:28:53 +0000 (UTC)
(envelope-from oshogbo@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 5796F863F4;
Sat, 27 Jan 2018 12:28:53 +0000 (UTC)
(envelope-from oshogbo@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 4DB86251F;
Sat, 27 Jan 2018 12:28:53 +0000 (UTC)
(envelope-from oshogbo@FreeBSD.org)
Received: from repo.freebsd.org ([127.0.1.37])
by repo.freebsd.org (8.15.2/8.15.2) with ESMTP id w0RCSro3063093;
Sat, 27 Jan 2018 12:28:53 GMT (envelope-from oshogbo@FreeBSD.org)
Received: (from oshogbo@localhost)
by repo.freebsd.org (8.15.2/8.15.2/Submit) id w0RCSrTl063092;
Sat, 27 Jan 2018 12:28:53 GMT (envelope-from oshogbo@FreeBSD.org)
Message-Id: <201801271228.w0RCSrTl063092@repo.freebsd.org>
X-Authentication-Warning: repo.freebsd.org: oshogbo set sender to
oshogbo@FreeBSD.org using -f
From: Mariusz Zaborski <oshogbo@FreeBSD.org>
Date: Sat, 27 Jan 2018 12:28:53 +0000 (UTC)
To: src-committers@freebsd.org, svn-src-all@freebsd.org,
svn-src-head@freebsd.org
Subject: svn commit: r328472 - head/sys/geom/label
X-SVN-Group: head
X-SVN-Commit-Author: oshogbo
X-SVN-Commit-Paths: head/sys/geom/label
X-SVN-Commit-Revision: 328472
X-SVN-Commit-Repository: base
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
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" \)" <svn-src-all.freebsd.org>
List-Unsubscribe: <https://lists.freebsd.org/mailman/options/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/svn-src-all/>;
List-Post: <mailto:svn-src-all@freebsd.org>
List-Help: <mailto:svn-src-all-request@freebsd.org?subject=help>
List-Subscribe: <https://lists.freebsd.org/mailman/listinfo/svn-src-all>,
<mailto:svn-src-all-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 27 Jan 2018 12:28:53 -0000
Author: oshogbo
Date: Sat Jan 27 12:28:52 2018
New Revision: 328472
URL: https://svnweb.freebsd.org/changeset/base/328472
Log:
Don't truncate name of glabel.
If it's to long just report that.
Reviewed by: trasz@
Differential Revision: https://reviews.freebsd.org/D13746
Modified:
head/sys/geom/label/g_label.c
Modified: head/sys/geom/label/g_label.c
==============================================================================
--- head/sys/geom/label/g_label.c Sat Jan 27 11:54:51 2018 (r328471)
+++ head/sys/geom/label/g_label.c Sat Jan 27 12:28:52 2018 (r328472)
@@ -200,6 +200,7 @@ g_label_create(struct gctl_req *req, struct g_class *m
struct g_provider *pp2;
struct g_consumer *cp;
char name[64];
+ int n;
g_topology_assert();
@@ -213,7 +214,12 @@ g_label_create(struct gctl_req *req, struct g_class *m
}
gp = NULL;
cp = NULL;
- snprintf(name, sizeof(name), "%s/%s", dir, label);
+ n = snprintf(name, sizeof(name), "%s/%s", dir, label);
+ if (n >= sizeof(name)) {
+ if (req != NULL)
+ gctl_error(req, "Label name %s is too long.", label);
+ return (NULL);
+ }
LIST_FOREACH(gp, &mp->geom, geom) {
pp2 = LIST_FIRST(&gp->provider);
if (pp2 == NULL)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180127214500.I925>