svn commit: r353640 - head/sys/kern

[Konstantin Belousov]

On Wed, Oct 23, 2019 at 11:18:06PM +1100, Bruce Evans wrote:
> 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.
32bit ARM requires 8-byte alignment for 64bit integers AFAIR.  Also I
believe that this is the only such 32bit architecture supported by FreeBSD.

> 
> 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