From owner-svn-src-head@freebsd.org Sat Jan 27 12:13:00 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 38957EB9591; Sat, 27 Jan 2018 12:13:00 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 7DEE885B4F; Sat, 27 Jan 2018 12:12:58 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id E8263103984; Sat, 27 Jan 2018 23:12:50 +1100 (AEDT) Date: Sat, 27 Jan 2018 23:12:50 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Pedro Giffuni cc: Bruce Evans , 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 In-Reply-To: <10edbded-b2b5-07e8-b527-b9b4c46d0bae@FreeBSD.org> Message-ID: <20180127214500.I925@besplex.bde.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> MIME-Version: 1.0 X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=YbvN30Zf c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=nlC_4_pT8q9DhB4Ho9EA:9 a=EXLln4MnoMlu6O24mmsA:9 a=45ClL6m2LaAA:10 Content-Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 27 Jan 2018 12:13:00 -0000 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: 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 Date: Sat, 27 Jan 2018 00:56:47 -0800 Message-ID: Subject: Re: svn commit: r328166 - in head/sys: amd64/amd64 x86/include x86/x86 To: Ed Maste , 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 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) > { > >