Date: Wed, 11 Apr 2018 23:37:46 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ian Lepore <ian@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r332395 - head/sys/kern Message-ID: <20180411222028.W952@besplex.bde.org> In-Reply-To: <201804102257.w3AMvuIF061050@repo.freebsd.org> References: <201804102257.w3AMvuIF061050@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180411222028.W952>