On Tue, 10 Apr 2018, Ian Lepore wrote:
URL: https://svnweb.freebsd.org/changeset/base/332395
Log:
Use explicit_bzero() when cleaning values out of the kernel environment.
Sometimes the values contain geli passphrases being communicated from
loader(8) to the kernel, and some day the compiler may decide to start
eliding calls to memset() for a pointer which is not dereferenced again
before being passed to free().
Using memset() in the kernel is also a style bug. I used to police this
in files that I used, and there were still only 37 instances of it in
kern/*.c before this commit. There were 209 instances of using the BSD
API bzero().
It is interesting that using memset() also asks for security holes.
bzero() already has the correct semantics for avoiding security holes,
so explicit_bzero() instead of just bzero() in the kernel is another
style bug. There were only 6 instances of this style bug in kern/*.c
(all in kern_shutdown.c).
Most places where there is an obvious security bug just use bzero(). The
most common bug was for copying out structs. Padding in the structs must
be zeroed, and bzero() is a good way to do this. In this case, the
compiler can't see where the copy is used, so even bzero() is safe.
bzero() should not cause security bugs anywhere, since it is not a
standard C function so C compilers cannot know what it does. However,
it is a standard POSIX function so C compilers with POSIX extensions
could know what it does. POSIX has a deficient specification of it in
at least the 2001 version. It says that "The bzero() function shall
place n zero-valued bytes in the area pointed to by s" and under
APPLICATION USAGE it says "[bad advice on preferring memset() deleted.
Now I quote its bad advice on portability:] for maximum portability,
it is recommended to replace the function call to bzero [by]
#define bzero(b,len) (memset((b), '\0', (len)), (void) 0). The C
standard says much the same for memset(), but it is clearer that the
"as if" rule applies to memset(), so compilers don't have to actually
fill in the array as specified they can prove that no conforming program
can tell the difference. Before POSIX standardized bzero() in 2001,
compilers couldn't do the same opimization for bzero(), so the de-facto
standard for it was to actually fill in the array and this is what should
have been standardized. Similarly for memset() when it was standardized
in the late 1980's. Not many people would have noticed and/or cared about
the security problem then. It should have been better known in 2001.
In the kernel, the compiler cannot know what even memset() does, since the
kernel is built by freestanding compilers. However, bzero() was recently
optimized to use __builtin_memset(). This is an invalid optimization,
since it gives the security hole. bzero(9) is actually documented, but
its documentation has the same deficiencies as POSIX's and FreeBSD's
bzero(3) -- it is unobvious if the "as if" rule applies to these functions.
(The "as if" rule probably applies to all APIs, but it is too difficult
to determine and allow for operations not don;t exactly what their man
page says they do.)
Strangely, memset() in the kernel is not optimized using
__builtin_memset(), although this optimization might be valid. (No
one knows what memset() in the kernel does since it doesn't have even
a fuzzy memset(9).). memset(9) is still correctly deprecated by not
optimizing it like bzero(9). It is significantly pessimized only for
a nonzero full byte -- then it uses a simple loop, with the loop bogusly
optimized by inlining it. For a zero fill byte, it uses bzero() which
often uses __builtin_memset(). clang and even gcc-4.2.1 have a builtin
bzero, but this is not used. Its semantics are as unclear as bzero()'s.
The simple loop for memset() in the nonzero fill-byte case is an older
mistake. IIRC, it was originally only done for the inline memset() in
libkern.h. However, due to bugs in builtins and possibly with -O0,
memset() is need as an extern function too. So the loop occurs in
the inline version where it is mostly a negative optimization for space,
and in the extern version where it is good enough. Inlining the loop
only clearly optimizes for security holes -- it allows the compiler to
see what the function does, so the compiler can make it do nothing.
However, in the freestanding case, it is still an invalid optimization
to not zero things before they are freed. The compiler cannot know
what free(9) does unless free() is inlined, and free() is too large to
even be inlined. So explicit_bzero() is needed here even less than in
most places.
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
