svn commit: r347063 - head/sys/kern

[John Baldwin]

On 5/6/19 1:57 PM, Mark Johnston wrote:
> On Mon, May 06, 2019 at 01:40:19PM -0700, John Baldwin wrote:
>> On 5/6/19 11:45 AM, Mark Johnston wrote:
>>> On Mon, May 06, 2019 at 11:07:18AM -0700, John Baldwin wrote:
>>>> On 5/3/19 2:26 PM, Mark Johnston wrote:
>>>>> Author: markj
>>>>> Date: Fri May  3 21:26:44 2019
>>>>> New Revision: 347063
>>>>> URL: https://svnweb.freebsd.org/changeset/base/347063
>>>>>
>>>>> Log:
>>>>>   Disallow excessively small times of day in clock_settime(2).
>>>>>   
>>>>>   Reported by:	syzkaller
>>>>>   Reviewed by:	cem, kib
>>>>>   MFC after:	1 week
>>>>>   Sponsored by:	The FreeBSD Foundation
>>>>>   Differential Revision:	https://reviews.freebsd.org/D20151
>>>>>
>>>>> Modified:
>>>>>   head/sys/kern/kern_time.c
>>>>>
>>>>> Modified: head/sys/kern/kern_time.c
>>>>> ==============================================================================
>>>>> --- head/sys/kern/kern_time.c	Fri May  3 21:13:09 2019	(r347062)
>>>>> +++ head/sys/kern/kern_time.c	Fri May  3 21:26:44 2019	(r347063)
>>>>> @@ -412,7 +412,9 @@ kern_clock_settime(struct thread *td, clockid_t clock_
>>>>>  	if (ats->tv_nsec < 0 || ats->tv_nsec >= 1000000000 ||
>>>>>  	    ats->tv_sec < 0)
>>>>>  		return (EINVAL);
>>>>> -	if (!allow_insane_settime && ats->tv_sec > 8000ULL * 365 * 24 * 60 * 60)
>>>>> +	if (!allow_insane_settime &&
>>>>> +	    (ats->tv_sec > 8000ULL * 365 * 24 * 60 * 60 ||
>>>>> +	    ats->tv_sec < utc_offset()))
>>>>>  		return (EINVAL);
>>>>>  	/* XXX Don't convert nsec->usec and back */
>>>>>  	TIMESPEC_TO_TIMEVAL(&atv, ats);
>>>>
>>>> Pardon my ignorance, but I can't see why you are checking against utc_offset()
>>>> vs some small constant?  None of the discussion in the review mentioned the
>>>> reason for using this particular value, and I didn't see any comparisons
>>>> against utc_offset or kernadjtz in kern_clock_setttime() or settime() that
>>>> would have underflowed or panicked.  Can you give a bit more detail on why
>>>> utc_offset() is the lower bound?  Thanks.
>>>
>>> I chose it because we subtract utc_offset() from the time passed in to
>>> clock_settime(); see settime_task_func().  That subtraction caused the
>>> underflow that later caused the observed panics.
>>
>> Ok, thanks.  A few things I didn't see anyone else note in the review then:
>>
>> 1) This subtraction is actually not done for all rtc drivers, so it seems
>>    like we might block small times for RTC clocks that set
>>    CLOCKF_GETTIME_NO_ADJ.
> 
> The drivers that set NO_ADJ still account for the offset in their
> individual settime methods.  I don't see how it can be correct for any
> driver to ignore adjkerntz?
> 
>> 2) utc_offset can be negative for machines using local time in timezones
>>    "before" UTC.
> 
> Hmm, I believe the patch still handles this case?

Yes, it's just redundant as the earlier 'tv_sec < 0' check means we'll
never see a value < utc_offset.  One thought I had was similar to what
Warner suggested of just using the max value of '24 * 60 * 60', though
I suppose the maximal utc_offset is more like '12 * 60 * 60'?  utc_offset
is probably good enough as is.

>> I suppose we don't think any FreeBSD machines actually need to set the
>> running clock to 0 anyway so fixing it here rather than rejecting invalid
>> values only for RTCs that can't handle it is probably ok, but the
>> connection doesn't feel obvious that we are rejecting times that might
>> be non-representable in RTCs.
> 
> I can add a comment explaining where the comment comes from, assuming
> there are no objections to keeping the existing change.  The placement
> of the check was motivated by the placement of the pre-existing bounds
> check, and the fact that we have no good way to signal an error after
> setting the clock.

Well, one route we could have chosen would be to let the kernel run at
a time the RTC can't represent and in that case we just don't rewrite
the RTC and the time will reset to the RTC's time when we reboot.  This
would rely on the individual RTC drivers ignoring non-representable
times in their settime methods.  However, the fact that we already
ignore times < 0 means we already have an arbitrary lower bound, so I
ended up concluding that what you committed was fine.  If you can think
of a good comment to add that might be helpful to future readers.

-- 
John Baldwin