svn commit: r292777 - in head: lib/libc/sys sys/kern

[Bruce Evans]

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