Skip site navigation (1)Skip section navigation (2)
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-head@freebsd.org  Sat Jan 27 08:56:51 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 1D800E7838F;
 Sat, 27 Jan 2018 08:56:51 +0000 (UTC)
 (envelope-from dexuan.bsd@gmail.com)
Received: from mail-lf0-x243.google.com (mail-lf0-x243.google.com
 [IPv6:2a00:1450:4010:c07::243])
 (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits))
 (Client CN "smtp.gmail.com",
 Issuer "Google Internet Authority G2" (verified OK))
 by mx1.freebsd.org (Postfix) with ESMTPS id 67C997D6F6;
 Sat, 27 Jan 2018 08:56:50 +0000 (UTC)
 (envelope-from dexuan.bsd@gmail.com)
Received: by mail-lf0-x243.google.com with SMTP id q194so3478789lfe.13;
 Sat, 27 Jan 2018 00:56:50 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025;
 h=mime-version:in-reply-to:references:from:date:message-id:subject:to
 :cc; bh=zuOKqYpQcJL96POoQudADaOQp/x9tezPqtDsSk/AF88=;
 b=kxNyiBAYwTySRmZIfpOTt2OPAKaWVzhICF7BxhjQa7mlWiLRe5nGf6SOSufhzwHy8Y
 r472bvlVEZJiQdJ2yI9sPerE4UwtwG3Kkf9lUcCpADZibe9BWGv04pA8aB/QiQxG+AML
 nlP5aq7nN7Vum8U+/3I4biLvVqVe/s78d8XCsDyOQ8rZCQwz6EJfsGAK2U3LuU3jxpVn
 dMSB7FxA6I/M2jWnFsXuxI9rclG6b4Fp+Fwm+gSc1JwEy1DeqXa5nx05io4B2oZkHzGz
 dxpIqHsi4twxjIUIIrhhHa7sctBnv/+RgDaqNlP87OeS9glZC08RUJfv+/9lAFBOue/R
 s0QA==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20161025;
 h=x-gm-message-state:mime-version:in-reply-to:references:from:date
 :message-id:subject:to:cc;
 bh=zuOKqYpQcJL96POoQudADaOQp/x9tezPqtDsSk/AF88=;
 b=rvu6L2ShPzCybq5KGPhrUBKOA2c+iM46WhW+3R/zO865q/Kv4EYR8Nz11ePj+Tl/SM
 XnKoT4kQ7VSHxrlXzyzIUs7N0D8AyLVxOE2z6b5JpnXrlbR+yIo2ivN0yZUAJS2cZxrl
 bEPK3kEpomK1T6PRedWWN2muqa8mbGkESMFyk9ZRL536cn4raHHX5vAtnDQoVjxH9PBl
 8Kw+Svbp7RvaObonJ3/N+D/vk2+lYF+4IsiEIjU6FaXhngOBSmg4k+hD1Xa2Ubchv/XO
 1cEjvw8M6RAXj/ujkaX/vCMCuM6mVKLtQoM8CML/+4h595vvLuWDpDg0dfEolS0UyzHY
 raKg==
X-Gm-Message-State: AKwxytfyUWeuTRLF/f5qMnn+9BVMj2RCwSB/2NmgqKClV456mDuWJnOK
 lSHvgSOeZtc4p/4grY3fWXaFckOjKWDO9bcFO6SGHmAf
X-Google-Smtp-Source: AH8x224rVqXpTQrPksGb30vokvownXnfSGCVNpSnssf6uV7Rx9DemXL4ijKTIZ5o/Slwz1nE19kbOxC46DQBdu/kwBc=
X-Received: by 10.46.65.154 with SMTP id d26mr3934341ljf.109.1517043408230;
 Sat, 27 Jan 2018 00:56:48 -0800 (PST)
MIME-Version: 1.0
Received: by 10.25.228.148 with HTTP; Sat, 27 Jan 2018 00:56:47 -0800 (PST)
In-Reply-To: <201801191542.w0JFgY1Q070919@repo.freebsd.org>
References: <201801191542.w0JFgY1Q070919@repo.freebsd.org>
From: Dexuan-BSD Cui <dexuan.bsd@gmail.com>
Date: Sat, 27 Jan 2018 00:56:47 -0800
Message-ID: <CABkgQk8eYpqGsJv-BWdsinQFW2FueHfmCuptBbBuJSv+w-24rA@mail.gmail.com>
Subject: Re: svn commit: r328166 - in head/sys: amd64/amd64 x86/include x86/x86
To: Ed Maste <emaste@freebsd.org>, markj@freebsd.org, kib@freebsd.org,
 cem@freebsd.org, 
 mhorne063@gmail.com, gordon@freebsd.org, pho@freebsd.org, jeff@freebsd.org, 
 jhb@freebsd.org, nullius@nym.zone, decui@microsoft.com, sephe@freebsd.org
Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, 
 svn-src-head@freebsd.org
X-Mailman-Approved-At: Sat, 27 Jan 2018 12:22:40 +0000
Content-Type: text/plain; charset="UTF-8"
X-Content-Filtered-By: Mailman/MimeDel 2.1.25
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: Sat, 27 Jan 2018 08:56:51 -0000

Hi,
Today I found the KPTI patch broke FreeBSD VM running on Hyper-V: the VM
can't boot due to:

vmbus0: cannot find free IDT vector

This is the related snippet:

dev/hyperv/vmbus/vmbus.c: vmbus_intr_setup() -> lapic_ipi_alloc() fails:

        /*
         * All Hyper-V ISR required resources are setup, now let's find a
         * free IDT vector for Hyper-V ISR and set it up.
         */
        sc->vmbus_idtvec = lapic_ipi_alloc(pti ? IDTVEC(vmbus_isr_pti) :
            IDTVEC(vmbus_isr));
        if (sc->vmbus_idtvec < 0) {
                device_printf(sc->vmbus_dev, "cannot find free IDT
vector\n");
                return ENXIO;
        }

Luckily for now I can work around this boot failure by adding vm.pmap.pti=0
into /boot/loader.conf.

Any suggestion?


Thanks!

-- Dexuan


On Fri, Jan 19, 2018 at 7:42 AM, Ed Maste <emaste@freebsd.org> wrote:

> Author: emaste
> Date: Fri Jan 19 15:42:34 2018
> New Revision: 328166
> URL: https://svnweb.freebsd.org/changeset/base/328166
>
> Log:
>   Enable KPTI by default on amd64 for non-AMD CPUs
>
>   Kernel Page Table Isolation (KPTI) was introduced in r328083 as a
>   mitigation for the 'Meltdown' vulnerability.  AMD CPUs are not affected,
>   per https://www.amd.com/en/corporate/speculative-execution:
>
>       We believe AMD processors are not susceptible due to our use of
>       privilege level protections within paging architecture and no
>       mitigation is required.
>
>   Thus default KPTI to off for AMD CPUs, and to on for others.  This may
>   be refined later as we obtain more specific information on the sets of
>   CPUs that are and are not affected.
>
>   Submitted by: Mitchell Horne
>   Reviewed by:  cem
>   Relnotes:     Yes
>   Security:     CVE-2017-5754
>   Sponsored by: The FreeBSD Foundation
>   Differential Revision:        https://reviews.freebsd.org/D13971
>
> Modified:
>   head/sys/amd64/amd64/machdep.c
>   head/sys/x86/include/x86_var.h
>   head/sys/x86/x86/identcpu.c
>
> Modified: head/sys/amd64/amd64/machdep.c
> ============================================================
> ==================
> --- head/sys/amd64/amd64/machdep.c      Fri Jan 19 15:32:27 2018
> (r328165)
> +++ head/sys/amd64/amd64/machdep.c      Fri Jan 19 15:42:34 2018
> (r328166)
> @@ -1621,6 +1621,7 @@ hammer_time(u_int64_t modulep, u_int64_t physfree)
>         mtx_init(&dt_lock, "descriptor tables", NULL, MTX_DEF);
>
>         /* exceptions */
> +       pti = pti_get_default();
>         TUNABLE_INT_FETCH("vm.pmap.pti", &pti);
>
>         for (x = 0; x < NIDT; x++)
>
> Modified: head/sys/x86/include/x86_var.h
> ============================================================
> ==================
> --- head/sys/x86/include/x86_var.h      Fri Jan 19 15:32:27 2018
> (r328165)
> +++ head/sys/x86/include/x86_var.h      Fri Jan 19 15:42:34 2018
> (r328166)
> @@ -136,6 +136,7 @@ void        nmi_call_kdb_smp(u_int type, struct
> trapframe *fr
>  void   nmi_handle_intr(u_int type, struct trapframe *frame);
>  void   pagecopy(void *from, void *to);
>  void   printcpuinfo(void);
> +int    pti_get_default(void);
>  int    user_dbreg_trap(void);
>  int    minidumpsys(struct dumperinfo *);
>  struct pcb *get_pcb_td(struct thread *td);
>
> Modified: head/sys/x86/x86/identcpu.c
> ============================================================
> ==================
> --- head/sys/x86/x86/identcpu.c Fri Jan 19 15:32:27 2018        (r328165)
> +++ head/sys/x86/x86/identcpu.c Fri Jan 19 15:42:34 2018        (r328166)
> @@ -1608,6 +1608,16 @@ finishidentcpu(void)
>  #endif
>  }
>
> +int
> +pti_get_default(void)
> +{
> +
> +       if (strcmp(cpu_vendor, AMD_VENDOR_ID) == 0)
> +               return (0);
> +
> +       return (1);
> +}
> +
>  static u_int
>  find_cpu_vendor_id(void)
>  {
>
>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180127214500.I925>