svn commit: r314936 - head/sys/x86/isa

[Bruce Evans]

On Thu, 9 Mar 2017, Eric van Gyzen wrote:

> Log:
>  Validate values read from the RTC before trying BCD decoding
>
>  Submitted by:	cem
>  Reported by:	Michael Gmelin <freebsd at grem.de>
>  Tested by:	Oleksandr Tymoshenko <gonzo at bluezbox.com>
>  Sponsored by:	Dell EMC

This continues bad fixes for overflow bugs, especially in clock code.

> Modified: head/sys/x86/isa/atrtc.c
> ==============================================================================
> --- head/sys/x86/isa/atrtc.c	Thu Mar  9 01:26:10 2017	(r314935)
> +++ head/sys/x86/isa/atrtc.c	Thu Mar  9 02:19:30 2017	(r314936)
> @@ -102,7 +102,12 @@ writertc(int reg, u_char val)
> static __inline int
> readrtc(int port)
> {
> -	return(bcd2bin(rtcin(port)));

This function has a bad API.  The apparently-corresponding writertc()
writes a binary value, while for reading rtcin() reads the binary value
and this function is a wrapper that obfuscations the conversion from
bcd.  All callers of writertc() must do the bin2bcd() conversion, and
normally do it unwrapped by a local variable or interface function,
in the form writertc(bin2bcd()).  Callers of readrtc() should do the
same, or perhaps there should be wrappers for both, named to indicate
both the conversion and the rtc access.  When the conversion functions
are efficient like they used to be, they can be inlines or macros like
they are, but then it is clearer to write them explicitly like they
already are in writertc(bin2bcd()).

The names of the conversion functions use the twee abbreviation of "to"
as "2".

Expansion makes the inlining bogus.  rtcin() is very slow (thousands of
cycles on modern systems), so it is correctly not inlined.  Wrappers of
it should only be explicitly inlined if they are sure to be very small.
The conversion functions used to be simple macros, so compilers were
forced to inline them and writertc(bin2bcd()) was as clear and space/time
efficient as possible.  Now they are not so simple inline functions, so
compilers are not forced to inline them, and generate worse code for them
when the don't (e.g., with -O0).  Extern versions could afford to do
full range checking not under INVARIANTS, and the wrapped forms are
efficient for space.

> +	int readval;

rtcin() returns int instead of u_char for conveniences (ints are easier
to use) and manual optimization (to avoid extra code for promotions and
demotions).  This is now inconvenient.  Unless the compiler knows too
much about the internals of rtcin(), it cannot see that its result is
between 0 and 255 inclusive, so range checks must check that it is in
this range...

> +
> +	readval = rtcin(port);
> +	if (readval >= 0 && (readval & 0xf) < 0xa && (readval & 0xf0) < 0xa0)

... so you had added the range check for >= 0 here.  You didn't add the
range check for <= 255.  Buffer overrun still occurs for > 256, but that
is just as impossible as < 0.

The check that (readval & 0xf) < 0xa has no effect.  This is already
correctly handled in the table, by having entries for the out-of-bounds
cases 9-F mod 0x10 (except when (readval & 0xf0) == 0x90)

The check that (readval & 0xf0) < 0xa0 is an obfuscated version of the
correct check that readval <= 233.  It checks that readval <= 1, under
the valid assumption that readval <= 255.  This check is only necessary
because the table doesn't have entries for the out-of-bounds cases 160-
255 or even 154-159.  This check only rejects >= 160 mod 256, and in
the range 144-159, the (readval & 0xf) check rejects 154-159).

> +		return (bcd2bin(readval));
> +	return (0);

The correct fix is to expand the table to cover all 256 u_char values,
and maybe change the return type of rtcin() to u_char to make its range
clear.  Silently producing an in-band value in the error cases is not
so good, but the table already did it via 0 entries in the
(readval & 0xf) < 0xa cases.

The conversion functions already check that the table in is between 0 and
159 inclusive in the INVARIANTS cases.  This was the only part of previous
commits in this area which I almost agreed with, but I now see further
problems in them too.  The tables used to be declared as extern char []
so their size was opaque.  Their size is now needed to do range checks
in inline code.  This causes namespace problems.  The tables can't
be inlined, so sizeof(table) can't be used.  So hard-coded values
LIBKERN_LEN_* were added, and CTASSERT()s to check them.  This magic is
only needed because of the foot shooting in the tables.  Correctly-
sized tables must have the non-magic size 256.  The tables always had
minimal size for valid data (154 for one and 160) for the other.

Full checking (which is only worth doing when the error is actually
handled, perhaps by panicing as in the INVARIANTS case) actually needs
to check the logical bounds 154 and 160 (and 0).  This doesn't need
CTASSERT()s to check consistency, since the table sizes must be at
least this large and should be larger (256) to produce dummy values
for some unchecked cases.

Another bug in this commit is that it subverts the full checking without
providing correct error handling.

The checks in bcd2bin() aren't even full -- they prevent array overruns,
but don't detect the invalid input that is now detected by
(readval & 0xf) < 0xa.

The combined checking and handling for the bcd arg in bcd2bin() and the
readval variable here are now:
- bcd < 0: panic for invariants, else buffer overrun
- readval < 0: can't happen here; if it happened then our check and
   mishandling gives bcd = 0, so the different mishandling in bcd2bin()
   is unreachable
- bcd > 153: panic for invariants, else buffer overrun
- readval > 255: sometimes passed to bcd2bin(), giving its mishandling.
   Othertimes we subvert the sometimes-stronger chcks by passing bcd = 0
- 154 <= readval <= 255: we always subvert bcd2bin() by passing bcd = 0
- 0 <= readval <= 153 and (readval & 0xf) >= 10: bcd2bin() wouldn't detect
   this error if we passed bcd = readval since it only checks array bounds.
   We subvert a stricter bcd2bin() by passing bcd = 0.
- 0 <= readval <= 153 and (readval & 0xf) < 10: valid.

> }
>
> static void

I wrote demonstrations of holes in previous fixes in this area.  In the
INARIANTS case, privileged users can shoot their feet off in a more
controlled way and get panics in KASSERT()s for smaller errors than
before (for invalid times that would cause array indexes out of bounds
by a small amount instead of many -- previously, out of bounds by +1
only gave garbage in the RTC).  I noticed many related bugs in
securelevel.  For example, high securelevel is supposed to prevent
stepping the kernel time, but it only controls parts of settime()
and related APIs which not affected by securelevel can be used to
panic or break all times except possibly the system time.

Bruce