Date: Tue, 31 Jan 2017 23:33:37 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Bruce Evans <brde@optusnet.com.au> Cc: "Conrad E. Meyer" <cem@freebsd.org>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312702 - in head/sys: kern libkern sys Message-ID: <20170131221815.I2119@besplex.bde.org> In-Reply-To: <20170125050656.J2916@besplex.bde.org> References: <201701241805.v0OI5Tb6043549@repo.freebsd.org> <20170125050656.J2916@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 25 Jan 2017, Bruce Evans wrote:
> 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.
These bugs are still present.
Bounds checking for presposterous values in privileged syscalls is
silly, especially it is incomplete. There are thousands of sysctls
that privileged users can use to shoot themself even more easily.
Just near the settimeofday() syscall, similar foot shooting can be
accomplished directly using the adjkerntz sysctl. Changes to this
normally update the RTC immediately. This is correct if the changes
are correct. Small garbage values give garbage in the RTC and large
garbage values give overflows. Bounds checking in this commit sometimes
detects these overflows.
Foot-shooting can also be accomplished indirectly using adjkerntz.
Set it to a large (in magnitude) value that doesn't cause overflows.
This augments the overflows in the next call to settimeofday().
The fix is not to add bounds checking to all sysctls. Even restricting
according to securelevel wouldn't work right, since securelevel doesn't
work right. In sys/kern, there are only 10 sysctls (not includinging
adjkerntz) restricted by securelevel. However, settimeofday() is
restricted by securelevel. You can't set the time backwards at high
securelevels, but you can still set the RTC to anything using adjkerntz
at high securelevels.
settimeofday() gives another unsecured method with results similar to
adjkerntz for panicing and corrupting the RTC. set/gettimeofday()
still support timezones, and resettodr() adds both adjkerntz and
(tz_minuteswest * 60) to convert the kernel time to the RTC time. Both
of these additions are FreeBSD features. adjkerntz is necessary to
support normal use of the RTC on x86 (it is on local time), but
timezones in the kernel were deprecated 25 years ago, so why use them
now? On my systems, adjkerntz is always used and I never used
tz_minutewest until today to demonstrate foot shooting. The man page
says that the timezone is no longer used; actually, it is used for this
foot shooting.
There are many bugs in the implementation of the foot-shooting:
A: There are no bounds check on tz_minuteswest, so expressions like
(tz_minuteswest * 60) may overflow before the foot is the target.
settimeofday() tries to be careful about the order of operations, but
there is no order that just works for the dependent operations of
setting the time and the timezone. The new timezone should be set
before setting the time, since the timezone is needed for the RTC part
of setting the time, and settings should be reversible in case one
part fails. In practice, the time is set first, and if this succeeds
then further errors are mostly not detected, but sometimes cause panics;
then the RTC is set using a non-atomic copy of the pair (adjkerntz,
tz_minuteswest), where tz_minuteswest was usually set by the previous
settimeofday() call. Then tz_minuteswest is set for this call.
B: There is no interlocking for any of this. adjkerntz and tz_minuteswest
are plain ints, so reading them is probably atomic, but any number of
adjkerntz sysctls and settimeofday() syscalls can race each other.
The adjkerntz sysctl claims to be MPSAFE, but does nothing special.
It just overwrites the adjkerntz using a hopefully-atomic access, then
calls resettodr() like settime(). kib added some locking around the
hardware part of resettodr(), and did more careful things for the
boottime offset for timecounters. The boottime offset is more important
since it is used for every CLOCK_REALTIME timecounter call, and needs
atomic accesses more since it is larger than an int.
Some of the interlocking belongs in applications. E.g., after setting
the time, read the time and the monotonic time to check that the
(monotonic) time taken to set the time was not so long as to make the
setting too out of date, and retry until it was not too out of date.
Coherency between separate settings of the time and the RTC could be
maintained in the same way, but since the kernel sets both it do the
check. Granularity of the x86 RTC is 1 second, and accuracy is worse,
so a long lag is acceptable, but this is MD. My version doesn't bother
doing RTC updates for differences of less than 2 seconds.
>> 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.
But there is no way for lower levels to report errors, so the up-front
check is of some use. It mainly needs to be much stricter so that lower
levels can't fail.
> ...
I wrote a test program to demonstrate most of the panics reported previously,
and a couple of other problems:
X #include <sys/time.h>
X #include <limits.h>
X
X int
X main(void)
X {
X struct timeval tv;
X struct timezone tz;
X int r;
X
X r = gettimeofday(&tv, &tz);
X if (r != 0)
X err(1, "gettimeofday");
X tv.tv_usec = 0;
X #ifdef NEG
X tv.tv_sec = 0;
X /*
X * The RTC time wants to be tv_sec - adjkerntz - tz_minuteswest.
X * Make this go negative (and panic) by putting a positive value
X * in tz_minuteswest large enough to kill any non-perverse value
X * in adjkerntz. We can get the panic (and lots of races) more
X * easily using sysctl to put perverse values in adjkerntz before
X * calling settimeofday.
X */
X tz.tz_minuteswest = 24 * 60; /* 1 day before 1970 */
The limit checking added to the bcd functions detects bugs from this
setting.
If the RTC is messed up by less preposterous settings, then the error is
detected on the next boot; then machdep.adjkerntz is set to 0 instead of
the correct value, and machdep.disable_rtc_set is set to 1 to disable
updating the RTC when adjkerntz is changed. It takes settimeofday() or
the periodic update from an ntp-known-good value to update the RTC, and
these don't unset machdep.disable_rtc_set.
X #elif YETANOTHERBUG
X /*
X * With 32-bit time_t, INT_MAX is accepted, but overflows 1 second
X * later. Userland then prints the year as 1901. Userland does
X * avoid overflow when adding a positive timezone offset to INT_MAX.
X */
X tv.tv_sec = INT_MAX;
X tz.tz_minuteswest = 0;
This shows that times anywhere the limit must not be allowed. The limit
should be about 2034 for 32-bit time_t. The limit should be TIME_MAX,
but that doesn't exist.
X #elif defined(__i386__) /* XXX really 32-bit time_t */
X /*
X * 32-bit time_t is easy to overflow to negative starting at any
X * non-small nonnegative value.
X */
X tv.tv_sec = INT_MAX;
X tz.tz_minuteswest = -24 * 60; /* 1 day after end of 32-bit time_t */
X #else /* XXX really 64-bit time_t */
X /*
X * Put the bogus upper value for the limit check in tv_sec to get a
X * panic. We don't even need to go ~1 day higher using,
X * tz_minuteswest, since 366 is ~3/4 above the length of a year so
X * so the limit is already ~7499 days above the 5-digit year 10000.
X * Plus the Epoch bias of 1970 years. 1990.53 years altogether.
X * The panic for this gives the precise value 1989 above 10000.
X */
X tv.tv_sec = 9999L * 366 * 24 * 60 * 60;
X tz.tz_minuteswest = -24 * 60;
X #endif
X r = settimeofday(&tv, &tz);
X if (r != 0)
X err(1, "1st settimeofday");
X /*
X * One of many bugs in settimeofday() is that its tz_minuteswest
X * setting doesn't apply until the next call. Make this call.
X */
X r = settimeofday(&tv, &tz);
X if (r != 0)
X err(1, "2nd settimeofday");
X return (0);
X }
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170131221815.I2119>