Created attachment 176751 [details] reproduction program Integer truncation issues in clock_ts_to_ct() result in out-of-bounds results. These results are trusted by atrtc_settime() which calls bin2bcd(ct.day). bin2bcd is implemented with a table lookup, and when ct.day is out-of-bounds, it results in an out-of-bounds read to the bin2bcd_data[] table. This can lead to a page fault in the kernel and a panic. To trigger this issue you must have the PRIV_CLOCK_SETTIME privilege. Specifically when ts->tv_sec is large or negative on 64-bit platforms, the clock_ts_to_ct function calculations: days = secs / SECDAY; can result in an incorrect value that is negative. This can later result in several incorrect calculations in the result including ct->dow = day_of_week(days); which performs a modulus operation on a negative value, an integer overflow in ct->year, and an out-of-range value in ct->day, ct->hour = rsec / 3600; rsec = rsec % 3600; ct->min = rsec / 60; rsec = rsec % 60; ct->sec = rsec; and out-of-range values for ct->min and ct->sec (due to modulus operations on negative values). Verified on FreeBSD 10.3-RELEASE kernel on amd64 platform with QEMU "hardware". Recommendation: The "years" and "days" variables in clock_ts_to_ct() should be "long" and not "int". This function should not be called when the input "ts.tv_sec" field is negative, or it should return an error, or the function should be updated to properly handle negative second values. It should be carefully written to avoid signed-overflows and signed-modulus operations in this case. Reproduction: # cc -Wall crash_clock_settime.c -o crash_clock_settime # ./crash_clock_settime Fatal trap 12: page fault while in kernel mode cpuid = 0; apic id = 00 fault virtual address = 0xffffffff7eab4242 fault code = supervisor read data, page not present instruction pointer = 0x20:0xffffffff80e6e6c8 stack pointer = 0x28:0xfffffe00002169d0 frame pointer = 0x28:0xfffffe0000216a10 code segment = base rx0, limit 0xfffff, type 0x1b = DPL 0, pres 1, long 1, def32 0, gran 1 processor eflags = interrupt enabled, IOPL = 0 current process = 22 (a.out) trap number = 12 panic: page fault cpuid = 0 KDB: stack backtrace: #0 0xffffffff8098e390 at kdb_backtrace+0x60 #1 0xffffffff80951066 at vpanic+0x126 #2 0xffffffff80950f33 at panic+0x43 #3 0xffffffff80d55f7b at trap_fatal+0x36b #4 0xffffffff80d5627d at trap_pfault+0x2ed #5 0xffffffff80d558fa at trap+0x47a #6 0xffffffff80d3b8d2 at calltrap+0x8 #7 0xffffffff809993f1 at resettodr+0xc1 #8 0xffffffff80963ea0 at settime+0x180 #9 0xffffffff80963c96 at sys_clock_settime+0x86 #10 0xffffffff80d5694f at amd64_syscall+0x40f #11 0xffffffff80d3bbbb at Xfast_syscall+0xfb
The real problem is that clock_ts_to_ct() does not return an error, which means that an update to the function which returns error sometimes requires similar update to all two dozens of callers, including rare platforms. There are more problems, e.g. typical RTC year register only has three or four bcd digits, so that values cannot be stored, but we currently do not check for that. Due to algorithm of clock_ts_to_ct(), insanely large values would be handled quite long, with the type of local vars fixed. IMO fixing all the issues is relatively large work for almost no benefit. I propose, instead, to limit the range of valid setclock(2) values, by e.g. coarse approximating four bcd digits in the year value. Also, since you already diagnosed and noted it, change the type of the year and days variables in clock_ts_to_ct(). I put a sysctl to allow experimentation.
Created attachment 176773 [details] Proposed workaround
*** Bug 211960 has been marked as a duplicate of this bug. ***
(In reply to Konstantin Belousov from comment #2) Any reason to use "long year, days;" instead of time_t? Otherwise, I am happy with this patch.
(In reply to Conrad Meyer from comment #4) I mean, add this change (or appropriate update of it) to your revision. In particular, the checked value in kern_clock_settime() should be aligned with asserted range.
(In reply to Konstantin Belousov from comment #5) Oh, ok.
Added to https://reviews.freebsd.org/D9279 .
A commit references this bug: Author: cem Date: Tue Jan 24 18:05:30 UTC 2017 New revision: 312702 URL: https://svnweb.freebsd.org/changeset/base/312702 Log: Use time_t for intermediate values to avoid overflow in clock_ts_to_ct Add additionally safety and overflow checks to clock_ts_to_ct and the BCD routines while we're here. 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. 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 Changes: head/sys/kern/kern_time.c head/sys/kern/subr_clock.c head/sys/libkern/bcd.c head/sys/sys/libkern.h