svn commit: r353640 - head/sys/kern

[Bruce Evans]

On Tue, 22 Oct 2019, Alan Somers wrote:

> On Wed, Oct 16, 2019 at 7:21 AM Andrew Turner <andrew at freebsd.org> wrote:
>
>> Author: andrew
>> Date: Wed Oct 16 13:21:01 2019
>> New Revision: 353640
>> URL: https://svnweb.freebsd.org/changeset/base/353640
>>
>> Log:
>>   Stop leaking information from the kernel through timespec
>>
>>   The timespec struct holds a seconds value in a time_t and a nanoseconds
>>   value in a long. On most architectures these are the same size, however
>>   on 32-bit architectures other than i386 time_t is 8 bytes and long is
>>   4 bytes.
>>
>>   Most ABIs will then pad a struct holding an 8 byte and 4 byte value to
>>   16 bytes with 4 bytes of padding. When copying one of these structs the
>>   compiler is free to copy the padding if it wishes.

I doubt that most ABIs pad the struct in that case.  Pure 32-bit arches
don't have any 64-bit memory or register operations, so they use 32-bit
longs and don't pessimize structs with 64-bit types in them using padding.

If there are any such ABIs, then there are thousands if not millions more
places to fix.  Every place where a timespec is written must be checked.

E.g., gettimeofday() reduces to microtime().  The implementation of
microtime() happens to fill the timeval fields separately.  That always
leaks kernel context if struct timeval has padding and nothing else
zeros the padding.  And for gettimeofday() it is especially clear that
nothing else zeros the padding.  sys_gettimeofday() allocates a timeval
on the kernel stack; microtime() initializes its fields separately
leaving any padding untouched; copyout() then copies out kernel context.

This commit attempts to fix the different problem that if lower levels
like microtime() were pessimized to initialize the padding, then callers
would have to be careful to preserve this by not using struct
assignments for copying structs.  This is not easy to do.  bcopy() and
copyout() should preserve memory, but there are no guarantees if the
struct is accessed in other ways.  The fix in this commit can easily
not work on arches that have the problem:

 	bzero(&tv);			/* padding now all 0 */
 	tv.tv_sec = another_tv_sec;
 	/*
 	 * Padding now indeterminate (C99 6.2.6.1p6).  It is reasonable
 	 * for the implementation to use the padding on ABIs that have it,
 	 * 64-bit stores of registers with uncleared kernel context gives
 	 * gives the context leak.
 	 */
 	tv.tv_nsec = another_tv_nsec;

I don't see anything in C99 that requires padding to remain unchanged
even with no stores in the program.

This problem affects more than time_t's.  It affects all padding.

I next checked the simplest itimer call.  sys_getitimer() doesn't
handle the problem, of course: it allocates a struct itimerval
aitimerval the kernel stack; kern_getitimer() allocates a struct timeval
ctv on the kernel stack; ctv is initialized 1 field at a time in most
cases so its padding remains uninitialized; aitv is inititialized using
a struct assignment in most cases so it has the problem that this
commit attempts to fix.

This is just another bug in 64-bit time_t's, especially on 32-bit arches.
Instead of 32-bit time_t's which work until 2038 or 2106, we have subtle
security holes now.

Bruce