Date: Wed, 23 Oct 2019 23:18:06 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Alan Somers <asomers@freebsd.org> Cc: Andrew Turner <andrew@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r353640 - head/sys/kern Message-ID: <20191023210546.L892@besplex.bde.org> In-Reply-To: <CAOtMX2hfGrUtskf36H6r3kFu1JpjTs2yAU7rK5dRtAMp%2BXm=XQ@mail.gmail.com> References: <201910161321.x9GDL2ee021543@repo.freebsd.org> <CAOtMX2hfGrUtskf36H6r3kFu1JpjTs2yAU7rK5dRtAMp%2BXm=XQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 22 Oct 2019, Alan Somers wrote: > On Wed, Oct 16, 2019 at 7:21 AM Andrew Turner <andrew@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191023210546.L892>