On Sun, 27 Dec 2015, Ian Lepore wrote:
On Sun, 2015-12-27 at 15:37 +0000, Dmitry Chagin wrote:
Author: dchagin
Date: Sun Dec 27 15:37:07 2015
New Revision: 292777
URL: https://svnweb.freebsd.org/changeset/base/292777
Log:
Verify that tv_sec value specified in settimeofday() and
clock_settime()
(CLOCK_REALTIME case) system calls is non negative.
This commit hides a kernel panic in atrtc_settime() as the
clock_ts_to_ct()
does not properly convert negative tv_sec.
ps. in my opinion clock_ts_to_ct() should be rewritten to properly
handle
negative tv_sec values.
Differential Revision: https://reviews.freebsd.org/D4714
Reviewed by: kib
MFC after: 1 week
IMO, this change is completely unacceptable. If there is a bug in
atrtc code, then by all means fix it, but preventing anyone from
setting valid time values on the system because one driver's code can't
handle it is just wrong.
I agree. Even (time_t)-1 should be a valid time for input, although it
is an error indicator for output.
(This API makes correctly using functions like time(1) difficult or
impossible (impossible for time(1) specifically. The implementation
might reserve (time_t)-1 for representing an invalid time. But
nothing requires it to.
(POSIX almost requires the reverse. It requires a broken
implementation that represents times as seconds since the Epoch. I
think POSIX doesn't require times before the Epoch to work. But
FreeBSD and the ado time package tries to make them work.)
So if the representation of the current time is (time_t)-1, then
time(1) can't do anything better than return this value. But this
value is also the error value. There is no way to distinguish this
value from the error value, since time(1) is not required to set
errno.)
I think the change also doesn't actually work, especially in the Western
hemisphere, but it was written in the Eastern hemisphere. Suppose
that the time is the Epoch. This is 0, so it is pefectly valid. Then
if the clock is on local time, utc_offset() is added before calling
clock_cs_to_ct() and the result is a negative time_t in the Western
hemisphere. Similarly in the Eastern hemisphere when you test with
with Western settings.
The main bug in clock_ts_ct() is due to division being specified as
broken in C:
X void
X clock_ts_to_ct(struct timespec *ts, struct clocktime *ct)
X {
X int i, year, days;
X time_t rsec; /* remainder seconds */
X time_t secs;
X
X secs = ts->tv_sec;
X days = secs / SECDAY;
X rsec = secs % SECDAY;
Division of negative numbers used to be implementation-defined in C, but
C90 or C99 broke this by requiring the broken alternative of rounding
towards 0 like most hardware does. The remainder operation is consistently
broken. So when secs < 0, this always gives days < 0 and rsec either 0
or < 0.
If this causes a panic, then it is from a sanity check detecting the
invalid conversion later. A negative value in days breaks the loop
logic but seems to give premature exit from the loops instead of many
iterations.
Another bug here is the type of rsec. This variable is a small integer
(< SECDAY = 86400), not a time_t.
Code like this is probably common in userland. w(1) uses it, but w(1)
only deals with uptimes which should be positive.
clock_ct_to_ts() is also buggy:
X int
X clock_ct_to_ts(struct clocktime *ct, struct timespec *ts)
X {
X int i, year, days;
X ...
X /* Sanity checks. */
X if (ct->mon < 1 || ct->mon > 12 || ct->day < 1 ||
X ct->day > days_in_month(year, ct->mon) ||
X ct->hour > 23 || ct->min > 59 || ct->sec > 59 ||
X (sizeof(time_t) == 4 && year > 2037)) { /* time_t overflow */
X if (ct_debug)
X printf(" = EINVAL\n");
X return (EINVAL);
X }
The limit of 2037 is bogus with 64-bit time_t's or even with 32-bit
unsigned time_t's.
Years before 1970 are insane due to the C bug, and years before ~1906
are insanse due to representability problems, but there is no check
for the lower limit of 'year'.
There is also no check for the lower limit of ct->hour, ct->min or
ct->sec.
Bruce
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
