svn commit: r312702 - in head/sys: kern libkern sys
Bruce Evans
brde at optusnet.com.au
Tue Jan 24 20:12:28 UTC 2017
On Tue, 24 Jan 2017, Conrad E. Meyer wrote:
> Log:
> Use time_t for intermediate values to avoid overflow in clock_ts_to_ct
This is bogus. time_t is for storing times in seconds, not for times in
days, hours or minutes.
> Add additionally safety and overflow checks to clock_ts_to_ct and the
> BCD routines while we're here.
I also disagreed with previous versions of this fix.
> Perform a safety check in sys_clock_settime() first to avoid easy local
> root panic, without having to propagate an error value back through
> dozens of APIs currently lacking error returns.
I agree with not over-engineering this to check at all levels. But top-level
check needs to be more stringent and magic to work. It is easier to check
a couple of levels lower.
> PR: 211960, 214300
> Submitted by: Justin McOmie <justin.mcomie at gmail.com>, kib@
> Reported by: Tim Newsham <tim.newsham at nccgroup.trust>
> Reviewed by: kib@
> Sponsored by: Dell EMC Isilon, FreeBSD Foundation
> Differential Revision: https://reviews.freebsd.org/D9279
>
> Modified:
> head/sys/kern/kern_time.c
> head/sys/kern/subr_clock.c
> head/sys/libkern/bcd.c
> head/sys/sys/libkern.h
>
> Modified: head/sys/kern/kern_time.c
> ==============================================================================
> --- head/sys/kern/kern_time.c Tue Jan 24 17:30:13 2017 (r312701)
> +++ head/sys/kern/kern_time.c Tue Jan 24 18:05:29 2017 (r312702)
> @@ -387,6 +387,11 @@ sys_clock_settime(struct thread *td, str
> return (kern_clock_settime(td, uap->clock_id, &ats));
> }
>
> +static int allow_insane_settime = 0;
> +SYSCTL_INT(_debug, OID_AUTO, allow_insane_settime, CTLFLAG_RWTUN,
> + &allow_insane_settime, 0,
> + "do not perform possibly restrictive checks on settime(2) args");
Debugging code; shoouldn't be committed.
> +
> int
> kern_clock_settime(struct thread *td, clockid_t clock_id, struct timespec *ats)
> {
> @@ -400,6 +405,8 @@ kern_clock_settime(struct thread *td, cl
> if (ats->tv_nsec < 0 || ats->tv_nsec >= 1000000000 ||
> ats->tv_sec < 0)
> return (EINVAL);
Times before the Epoch were already disallowed here, but this doesn't prevent
negative times being passed to lower levels -- see below.
> + if (!allow_insane_settime && ats->tv_sec > 9999ULL * 366 * 24 * 60 * 60)
> + return (EINVAL);
This uses the long long abomination.
This checking belongs in lower levels.
It has a buggy limit. Since the average length of a year is below 366, this
can give a year later than 9999, so the year is not guaranteed to be
represntable which is not representable in hardware with 4 decimal digits,
so even lower levels with such hardware must do their own the check.
INT_MAX - <seconds in a year> is a more reasonable arbitrary limit. Not
LONG_MAX, since that is usually the same as TIME_T_MAX, and we want to
be able to add the timezone offset without overflow.
On arches with 32-bit time_t, the above limit exceeds TIME_T_MAX, so the
code should fail to compile due to being tautologically true. The
compiler must be smart enough to see the previous check that ats->tv_sec < 0,
so that it knows that the comparison cannot fail due to sign extension bugs
(negative times promoting to ULLONG_MAX). Using the signed abomination
9999LL would avoid the sign extension.
> /* XXX Don't convert nsec->usec and back */
> TIMESPEC_TO_TIMEVAL(&atv, ats);
> error = settime(td, &atv);
>
> Modified: head/sys/kern/subr_clock.c
> ==============================================================================
> --- head/sys/kern/subr_clock.c Tue Jan 24 17:30:13 2017 (r312701)
> +++ head/sys/kern/subr_clock.c Tue Jan 24 18:05:29 2017 (r312702)
> @@ -178,7 +178,7 @@ clock_ct_to_ts(struct clocktime *ct, str
> void
> clock_ts_to_ct(struct timespec *ts, struct clocktime *ct)
> {
> - int i, year, days;
> + time_t i, year, days;
> time_t rsec; /* remainder seconds */
> time_t secs;
This works provided the caller never passes a negative time, but
obfuscates the range checking.
rsec should also be int.
>
> @@ -214,6 +214,20 @@ clock_ts_to_ct(struct timespec *ts, stru
> print_ct(ct);
> printf("\n");
> }
> +
> + KASSERT(ct->year >= 0 && ct->year < 10000,
> + ("year %d isn't a 4 digit year", ct->year));
This is too device-dependent, and inconsistent with clock_ct_to_ts().
clock_ct_to_cs() checks that the year is < 2037. So allowing years >= 2037
here is worse than useless -- it allows writing times that will be rejected
when read back on the next boot of FreeBSD, although BIOSes and other OSes
might accept them.
The correct check here is simply an up-front test that tv->tv_sec >= 0
(don't trust callers to pass non-negative years) && tv->tv_sec < INT32_MAX.
This allows calculation of everything without overflow using int variables.
Further range checks are required:
(1) It was correct to not trust callers to pass nonnegative times, so the
check here is necessary. Negative times are passed here when the time
in clock_settime is small and utc_offset() is negative.
(2) Similarly for your sanity check. On arches with 64-bit time_t, it
accidentally preverts overlow when a positive utc_offset() is added,
but nothing prevents overflow on arches with 32-bit time_t. We could
allow this overflow since it usually gives the benign behaviour of a
large negative value which will be detected here.
(3) The year should be limited to 2037 to be consistent wth clock_ct_to_cs()
checks that the year is < 2037. Early days in year 2038 fit in a 32-bit
time_t, but later days don't, so the code uses 2037 for simplicity.
2037 should fit in even unreasonable hardware. It would be a good idea
to also limit to >= 2000 or >= 1980. < 1970 is already disallowed, except
for the negative times bugs. The code here is broken for negative times.
I think gives year 1969 for small negative offsets, but divison and modulo
round the days, minutes and seconds variables towards minus infinity,
giving negative or zero values (usually negative)).
> + KASSERT(ct->mon >= 1 && ct->mon <= 12,
> + ("month %d not in 1-12", ct->mon));
> + KASSERT(ct->day >= 1 && ct->day <= 31,
> + ("day %d not in 1-31", ct->day));
> + KASSERT(ct->hour >= 0 && ct->hour <= 23,
> + ("hour %d not in 0-23", ct->hour));
> + KASSERT(ct->min >= 0 && ct->min <= 59,
> + ("minute %d not in 0-59", ct->min));
Over-engineered, but it would detect the negative times bugs.
> + /* Not sure if this interface needs to handle leapseconds or not. */
> + KASSERT(ct->sec >= 0 && ct->sec <= 60,
> + ("seconds %d not in 0-60", ct->sec));
It really can't. POSIX time is broken by not supporting leap seconds,
so the leap seconds are not visible here, and in the opposite direction
we leap seconds in the hardware would just get in the way -- we would
have to drop then to get POSIX time.
> }
>
> int
>
> ...
> Modified: head/sys/sys/libkern.h
> ==============================================================================
> --- head/sys/sys/libkern.h Tue Jan 24 17:30:13 2017 (r312701)
> +++ head/sys/sys/libkern.h Tue Jan 24 18:05:29 2017 (r312702)
> @@ -49,9 +49,36 @@ extern u_char const bcd2bin_data[];
> extern u_char const bin2bcd_data[];
> extern char const hex2ascii_data[];
>
> -#define bcd2bin(bcd) (bcd2bin_data[bcd])
> -#define bin2bcd(bin) (bin2bcd_data[bin])
> -#define hex2ascii(hex) (hex2ascii_data[hex])
> +#define LIBKERN_LEN_BCD2BIN 154
> +#define LIBKERN_LEN_BIN2BCD 100
> +#define LIBKERN_LEN_HEX2ASCII 36
This uses Much ugliness to avoid namespace pollution.
> +
> +static inline u_char
Style bugs: 'inline' not spelled __inline.
> +bcd2bin(int bcd)
> +{
> +
> + KASSERT(bcd >= 0 && bcd < LIBKERN_LEN_BCD2BIN,
> + ("invalid bcd %d", bcd));
> + return (bcd2bin_data[bcd]);
> +}
> +
> +static inline u_char
> +bin2bcd(int bin)
> +{
> +
> + KASSERT(bin >= 0 && bin < LIBKERN_LEN_BIN2BCD,
> + ("invalid bin %d", bin));
> + return (bin2bcd_data[bin]);
> +}
> +
> +static inline char
> +hex2ascii(int hex)
> +{
> +
> + KASSERT(hex >= 0 && hex < LIBKERN_LEN_HEX2ASCII,
> + ("invalid hex %d", hex));
> + return (hex2ascii_data[hex]);
> +}
The macros were excessive optimization, but at least didn't take much
source code. The inlines are more excessive. I think converting
everything to extern only gives minor space pessimizations and time
pessimizations too small to measure.
hex2ascii() is mainly used by printf().
The others are used by a bit more than clock code (some cam).
contrib/octeon has private versions implemented using runtime divisions
and multiplications instead of table lookup. These take args of type
uint8_t, so some bounds checking is automatic, but silent truncation
can occur when the args are passed.
>
> static __inline int imax(int a, int b) { return (a > b ? a : b); }
> static __inline int imin(int a, int b) { return (a < b ? a : b); }
Bruce
More information about the svn-src-all
mailing list