From owner-svn-src-all@freebsd.org Sun Oct 21 11:41:50 2018 Return-Path: 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 900B3FECF3A; Sun, 21 Oct 2018 11:41:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 1124D89D0F; Sun, 21 Oct 2018 11:41:49 +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 mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 7AC881A07DF; Sun, 21 Oct 2018 22:41:46 +1100 (AEDT) Date: Sun, 21 Oct 2018 22:41:46 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Conrad Meyer cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r339481 - head/sys/dev/random In-Reply-To: <201810202015.w9KKF6Ag097561@repo.freebsd.org> Message-ID: <20181021220923.M1174@besplex.bde.org> References: <201810202015.w9KKF6Ag097561@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=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=nYu90UyBjzA0UZKbb94A:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 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: Sun, 21 Oct 2018 11:41:50 -0000 On Sat, 20 Oct 2018, Conrad Meyer wrote: > ... > Log: > Fortuna: trivial static variable cleanup > > Remove unnecessary use of function-local static variable. 32 bytes is > small enough to live on the stack. > ... > Modified: head/sys/dev/random/fortuna.c > ============================================================================== > --- head/sys/dev/random/fortuna.c Sat Oct 20 20:12:57 2018 (r339480) > +++ head/sys/dev/random/fortuna.c Sat Oct 20 20:15:06 2018 (r339481) > @@ -333,7 +333,7 @@ random_fortuna_genblocks(uint8_t *buf, u_int blockcoun > static __inline void > random_fortuna_genrandom(uint8_t *buf, u_int bytecount) > { > - static uint8_t temp[RANDOM_BLOCKSIZE*(RANDOM_KEYS_PER_BLOCK)]; > + uint8_t temp[RANDOM_BLOCKSIZE * RANDOM_KEYS_PER_BLOCK]; > u_int blockcount; > > RANDOM_RESEED_ASSERT_LOCK_OWNED(); This now uses explicit_bzero() on the stack variable (as before, just before returning from the function). This is neither necessary nor sufficient for security. Even with the static variable, it was a reasonable optimization to keep a copy of the whole variable on the stack, or better in registers which might be spilled to the stack (this is possible because the variable is small). So to be really secure, the function must clear all stack and registers used by the function. In practice, something probably clobbers the stack and registers before any untrusted thread can read them, and the explicit_bzero() is not really needed. However, this function is explicitly bogusly inlined, so the stack and registers usually don't get clobbered immediately after the function returns. The inlining is bogus since this function is very large. The function call overhead for just the explicit_bzero() in it is as large as the possible savings from inlining it. clang likes to bogusly inline almost all static functions. If this one were not too large to inline, then clang would inline it even if it is not declared __inline, so declaring it has no good effect if the compiler is clang. Auto-inlining may open security holes by extending the lifetime of stack variables. That is another reason to not do it. I use gcc -fno-inline -fno-inline-functions-called-once to kill all auto-inlining. clang inlines even functions called more than once, and doesn't support these flags. Where was the security hole with the old static variable? I think it was mostly with the long lifetime of the variable. The global memory is actually more secure against accidental leaks by copying it out than stack memory. But even local variables have long lifetimes if you can manage to suspend the system while a critical local variable is active. I think rendezvous stuff prevents this particalar variable remaining active across suspend. Bruce