svn commit: r339481 - head/sys/dev/random

[Bruce Evans]

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