From owner-svn-src-head@freebsd.org Thu Mar 9 06:29:47 2017 Return-Path: Delivered-To: svn-src-head@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 88850D04937; Thu, 9 Mar 2017 06:29:47 +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 34E6D61A; Thu, 9 Mar 2017 06:29:46 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 12CCB1026C4; Thu, 9 Mar 2017 17:29:42 +1100 (AEDT) Date: Thu, 9 Mar 2017 17:29:42 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Eric van Gyzen cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314936 - head/sys/x86/isa In-Reply-To: <201703090219.v292JUDS085429@repo.freebsd.org> Message-ID: <20170309151739.V942@besplex.bde.org> References: <201703090219.v292JUDS085429@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.2 cv=KeqiiUQD c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=IqgaevIMAAAA:8 a=0PxMPI8DK4O2Pusi7DYA:9 a=CjuIK1q_8ugA:10 a=31ToKovYkSaqktkqGXj9:22 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Thu, 09 Mar 2017 06:29:47 -0000 On Thu, 9 Mar 2017, Eric van Gyzen wrote: > Log: > Validate values read from the RTC before trying BCD decoding > > Submitted by: cem > Reported by: Michael Gmelin > Tested by: Oleksandr Tymoshenko > Sponsored by: Dell EMC This continues bad fixes for overflow bugs, especially in clock code. > Modified: head/sys/x86/isa/atrtc.c > ============================================================================== > --- head/sys/x86/isa/atrtc.c Thu Mar 9 01:26:10 2017 (r314935) > +++ head/sys/x86/isa/atrtc.c Thu Mar 9 02:19:30 2017 (r314936) > @@ -102,7 +102,12 @@ writertc(int reg, u_char val) > static __inline int > readrtc(int port) > { > - return(bcd2bin(rtcin(port))); This function has a bad API. The apparently-corresponding writertc() writes a binary value, while for reading rtcin() reads the binary value and this function is a wrapper that obfuscations the conversion from bcd. All callers of writertc() must do the bin2bcd() conversion, and normally do it unwrapped by a local variable or interface function, in the form writertc(bin2bcd()). Callers of readrtc() should do the same, or perhaps there should be wrappers for both, named to indicate both the conversion and the rtc access. When the conversion functions are efficient like they used to be, they can be inlines or macros like they are, but then it is clearer to write them explicitly like they already are in writertc(bin2bcd()). The names of the conversion functions use the twee abbreviation of "to" as "2". Expansion makes the inlining bogus. rtcin() is very slow (thousands of cycles on modern systems), so it is correctly not inlined. Wrappers of it should only be explicitly inlined if they are sure to be very small. The conversion functions used to be simple macros, so compilers were forced to inline them and writertc(bin2bcd()) was as clear and space/time efficient as possible. Now they are not so simple inline functions, so compilers are not forced to inline them, and generate worse code for them when the don't (e.g., with -O0). Extern versions could afford to do full range checking not under INVARIANTS, and the wrapped forms are efficient for space. > + int readval; rtcin() returns int instead of u_char for conveniences (ints are easier to use) and manual optimization (to avoid extra code for promotions and demotions). This is now inconvenient. Unless the compiler knows too much about the internals of rtcin(), it cannot see that its result is between 0 and 255 inclusive, so range checks must check that it is in this range... > + > + readval = rtcin(port); > + if (readval >= 0 && (readval & 0xf) < 0xa && (readval & 0xf0) < 0xa0) ... so you had added the range check for >= 0 here. You didn't add the range check for <= 255. Buffer overrun still occurs for > 256, but that is just as impossible as < 0. The check that (readval & 0xf) < 0xa has no effect. This is already correctly handled in the table, by having entries for the out-of-bounds cases 9-F mod 0x10 (except when (readval & 0xf0) == 0x90) The check that (readval & 0xf0) < 0xa0 is an obfuscated version of the correct check that readval <= 233. It checks that readval <= 1, under the valid assumption that readval <= 255. This check is only necessary because the table doesn't have entries for the out-of-bounds cases 160- 255 or even 154-159. This check only rejects >= 160 mod 256, and in the range 144-159, the (readval & 0xf) check rejects 154-159). > + return (bcd2bin(readval)); > + return (0); The correct fix is to expand the table to cover all 256 u_char values, and maybe change the return type of rtcin() to u_char to make its range clear. Silently producing an in-band value in the error cases is not so good, but the table already did it via 0 entries in the (readval & 0xf) < 0xa cases. The conversion functions already check that the table in is between 0 and 159 inclusive in the INVARIANTS cases. This was the only part of previous commits in this area which I almost agreed with, but I now see further problems in them too. The tables used to be declared as extern char [] so their size was opaque. Their size is now needed to do range checks in inline code. This causes namespace problems. The tables can't be inlined, so sizeof(table) can't be used. So hard-coded values LIBKERN_LEN_* were added, and CTASSERT()s to check them. This magic is only needed because of the foot shooting in the tables. Correctly- sized tables must have the non-magic size 256. The tables always had minimal size for valid data (154 for one and 160) for the other. Full checking (which is only worth doing when the error is actually handled, perhaps by panicing as in the INVARIANTS case) actually needs to check the logical bounds 154 and 160 (and 0). This doesn't need CTASSERT()s to check consistency, since the table sizes must be at least this large and should be larger (256) to produce dummy values for some unchecked cases. Another bug in this commit is that it subverts the full checking without providing correct error handling. The checks in bcd2bin() aren't even full -- they prevent array overruns, but don't detect the invalid input that is now detected by (readval & 0xf) < 0xa. The combined checking and handling for the bcd arg in bcd2bin() and the readval variable here are now: - bcd < 0: panic for invariants, else buffer overrun - readval < 0: can't happen here; if it happened then our check and mishandling gives bcd = 0, so the different mishandling in bcd2bin() is unreachable - bcd > 153: panic for invariants, else buffer overrun - readval > 255: sometimes passed to bcd2bin(), giving its mishandling. Othertimes we subvert the sometimes-stronger chcks by passing bcd = 0 - 154 <= readval <= 255: we always subvert bcd2bin() by passing bcd = 0 - 0 <= readval <= 153 and (readval & 0xf) >= 10: bcd2bin() wouldn't detect this error if we passed bcd = readval since it only checks array bounds. We subvert a stricter bcd2bin() by passing bcd = 0. - 0 <= readval <= 153 and (readval & 0xf) < 10: valid. > } > > static void I wrote demonstrations of holes in previous fixes in this area. In the INARIANTS case, privileged users can shoot their feet off in a more controlled way and get panics in KASSERT()s for smaller errors than before (for invalid times that would cause array indexes out of bounds by a small amount instead of many -- previously, out of bounds by +1 only gave garbage in the RTC). I noticed many related bugs in securelevel. For example, high securelevel is supposed to prevent stepping the kernel time, but it only controls parts of settime() and related APIs which not affected by securelevel can be used to panic or break all times except possibly the system time. Bruce