Bug 214300 - Integer truncation issues lead to out-of-bounds kernel reads and panics in clock_settime().
Summary: Integer truncation issues lead to out-of-bounds kernel reads and panics in cl...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 10.3-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Conrad Meyer
URL:
Keywords: patch
: 211960 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-11-07 21:50 UTC by Tim Newsham
Modified: 2017-01-24 18:06 UTC (History)
3 users (show)

See Also:


Attachments
reproduction program (3.04 KB, text/x-csrc)
2016-11-07 21:50 UTC, Tim Newsham
no flags Details
Proposed workaround (1.35 KB, patch)
2016-11-08 12:08 UTC, Konstantin Belousov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Newsham 2016-11-07 21:50:25 UTC
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
Comment 1 Konstantin Belousov freebsd_committer freebsd_triage 2016-11-08 12:07:38 UTC
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.
Comment 2 Konstantin Belousov freebsd_committer freebsd_triage 2016-11-08 12:08:13 UTC
Created attachment 176773 [details]
Proposed workaround
Comment 3 Conrad Meyer freebsd_committer freebsd_triage 2017-01-24 00:28:12 UTC
*** Bug 211960 has been marked as a duplicate of this bug. ***
Comment 4 Conrad Meyer freebsd_committer freebsd_triage 2017-01-24 00:31:31 UTC
(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.
Comment 5 Konstantin Belousov freebsd_committer freebsd_triage 2017-01-24 11:38:44 UTC
(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.
Comment 6 Conrad Meyer freebsd_committer freebsd_triage 2017-01-24 16:16:37 UTC
(In reply to Konstantin Belousov from comment #5)
Oh, ok.
Comment 7 Conrad Meyer freebsd_committer freebsd_triage 2017-01-24 17:13:59 UTC
Added to https://reviews.freebsd.org/D9279 .
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-01-24 18:06:16 UTC
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