From owner-svn-src-all@freebsd.org Fri May 20 23:02:39 2016 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 2D0BFB4395E; Fri, 20 May 2016 23:02:39 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id 9F1C41710; Fri, 20 May 2016 23:02:38 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c122-106-149-109.carlnfd1.nsw.optusnet.com.au (c122-106-149-109.carlnfd1.nsw.optusnet.com.au [122.106.149.109]) by mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 7F041D49B0F; Sat, 21 May 2016 09:02:21 +1000 (AEST) Date: Sat, 21 May 2016 09:02:21 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r300332 - in head/sys: amd64/amd64 i386/i386 In-Reply-To: <201605201950.u4KJoWA5028092@repo.freebsd.org> Message-ID: <20160521081930.I1098@besplex.bde.org> References: <201605201950.u4KJoWA5028092@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=c+ZWOkJl c=1 sm=1 tr=0 a=R/f3m204ZbWUO/0rwPSMPw==:117 a=L9H7d07YOLsA:10 a=9cW_t1CCXrUA:10 a=s5jvgZ67dGcA:10 a=kj9zAlcOel0A:10 a=KLCK9ar9Cjh003kMN1MA:9 a=CANlSisP4UVpKUjV:21 a=3Ba7BzsO7dAWVtIk:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.22 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: Fri, 20 May 2016 23:02:39 -0000 On Fri, 20 May 2016, Konstantin Belousov wrote: > Log: > Check for overflow and return EINVAL if detected. Backport this and > r300305 to i386. > > PR: 209661 > Reported and reviewed by: cturt > Sponsored by: The FreeBSD Foundation > MFC after: 3 days > > Modified: > head/sys/amd64/amd64/sys_machdep.c > head/sys/i386/i386/sys_machdep.c > > Modified: head/sys/i386/i386/sys_machdep.c > ============================================================================== > --- head/sys/i386/i386/sys_machdep.c Fri May 20 19:46:25 2016 (r300331) > +++ head/sys/i386/i386/sys_machdep.c Fri May 20 19:50:32 2016 (r300332) > @@ -315,8 +315,9 @@ i386_set_ioperm(td, uap) > struct thread *td; > struct i386_ioperm_args *uap; > { > - int i, error; > char *iomap; > + u_int i; > + int error; > > if ((error = priv_check(td, PRIV_IO)) != 0) > return (error); > @@ -334,7 +335,8 @@ i386_set_ioperm(td, uap) > return (error); > iomap = (char *)td->td_pcb->pcb_ext->ext_iomap; > > - if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY) > + if (uap->start > uap->start + uap->length || > + uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY) > return (EINVAL); > > for (i = uap->start; i < uap->start + uap->length; i++) { I don't like using u_int for a small index. After the bounds checking fix, the range fits in a small signed integer. However, uap->start and uap->length already use bad type u_int, so it is natural to keep using that type. Except, i386_get/set_ioperm() is undocumented on amd64, so no one knows what its types are. amd64 documents sysarch(2) and that refers to i386_get_ioperm(2), i386_get_ldt(2) and i386_vm86(2), but these man pages are not installed. They are installed on i386. I think syscalls for them exist ecept for vm86. Man pages are also missing for newer sysarch() calls, and amd64 has more of these: {AMD64,I386}_{GET,SET}_XFPUSTATE, {AMD64,I386}_{GET,SET}_{FS,GS}BASE. apropos can't find anything related. It would be even more natural to do separate range checks for the start and length: start >= 0 && start <= IOPAGES * PAGE_SIZE * NBBY && length >= 0 && length <= IOPAGES * PAGE_SIZE * NBBY - start. It isn't clear whether to allow the silly null range of start = = IOPAGES * PAGE_SIZE * NBBY && length == 0. Initialization doesn't allow this. Other null ranges should be allowed. Didn't the old versions already have adequate bounds checking even without the fixes? The fixes just avoid depending on undefined^Wimplementation- defined behaviour, and return EINVAL instead of doing nothing for bad ranges: X if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY) X return (EINVAL); X X for (i = uap->start; i < uap->start + uap->length; i++) { Suppose (uap->start > uap->start + uap->length). Then if i is unsigned, the intial i is after the end so nothing is done, and if i is signed then the same is true if uap->start < INT_MAX; otherwise assignment overflows to an implementation-defined value which is usually negative and comparision turns this into a value which is much larger than the end, so again nothing is done. Changing a type from signed to unsigned is neither necessary nor sufficient for getting this right. You have to know that the entire range fits in the type. The analysis for it fitting in the range [0, INT_MAX] is not much different for the analysis for it fitting in the range [0, UINT_MAX], but is often slightly simpler because it cannot depend on special properties of unsigned types. Bruce