Leaking kernel stack data in struct padding

[Valery Ushakov <uwe%stderr.spb.ru@localhost>]

On Wed, Jun 13, 2018 at 02:09:09 +0000, Valeriy E. Ushakov wrote:

> Module Name:	src
> Committed By:	uwe
> Date:		Wed Jun 13 02:09:09 UTC 2018
> 
> Modified Files:
> 	src/sys/dev/wscons: wsevent.c
> 
> Log Message:
> wsevent_copyout_events50 - don't leak garbage from the kernel stack.
> 
> On 64-bit machines struct timespec50 has padding between 32-bit tv_sec
> and long tv_nsec that is not affected by normal assignment.  Scrub it
> before we uiomove struct owscons_event.

I was looking at mouse events on an amd64 VM with

  # hexdump -e '/4 " %2d" /4 " %5d" /8 "  %d" /8 ".%09d" "\n"' /dev/wsmouse

note: wscons event sources give you compat event structs unless you
request the current version with an ioctl (which is kinda hard to do
in hexdump :).

I noticed that the first reported event always had bogus timestamp.
Took me a bit of time to realize what was going on.  I fixed it in
wsevent.c (indentation reduced for readability):

+#if INT32_MAX < LONG_MAX       /* scrub padding */
+   memset(&ev50.time, 0, offsetof(struct timespec50, tv_nsec));
+#endif 
    timespec_to_timespec50(&ev->time, &ev50.time);

but I wonder if this scrubbing should be moved into
timespec_to_timespec50() - after all the most likley use of the compat
struct is to write or copyout it in the compat code, so the same
problem probably happens elsewhere.

On amd64 the compiler is smart enough to convert memset() to a few
movq's.  The compiler is not smart enough to notice that tv_nsec is
written to in timespec_to_timespec50(), so

    memset(&ev50.time, 0, sizeof(ev50.time));
    timespec_to_timespec50(...);

would still emit two movq's immediately followed by another movq to
tv_nsec.  Hence this specific arguments in the call to memset().

Comments?


PS: The next logical question is if there's a tool that can help audit the
rest of the kernel for problems like that.  :)

-uwe