Two bugs in kernel time code

Ulrich Windl (ulrich.windl@rz.uni-regensburg.de)
Mon, 15 Sep 1997 09:08:46 +0200


The patch below is actually just an extract from my new PPSkit-0.3.0 that I'll
announce a bit later.

There's a very old bug in the kernel that rarely occurs (who's using NTP?):
The code to update the CMOS clock does not consider the hour field. I had no
clever idea to fix it, so I simply added a message to log the problem (well,
a part of it).

In adjtimex() several parameters were not checked, making it possibly to pass
invalid values to the kernel. Also the plain old adjtim() was dependent on the
time_status & STA_PLL, which should be completely independent of adjtimex()
(ntp_adjtime).

Especially it was allowed to change the read-only bits of time_status...

The function now even returns TIME_ERROR if the flags in time_state indicate an
error condition.

It should go into 2.1 as well I guess...

Ulrich

diff -r -u --new-file /usr/src/linux-2.0.31-pre9/arch/i386/kernel/time.c
/usr/src/linux/arch/i386/kernel/time.c
--- /usr/src/linux-2.0.31-pre9/arch/i386/kernel/time.c Sun Nov 10 18:40:53 1996
+++ /usr/src/linux/arch/i386/kernel/time.c Fri Sep 12 21:35:53 1997
@@ -282,6 +286,9 @@
* nowtime is written into the registers of the CMOS clock, it will
* jump to the next second precisely 500 ms later. Check the Motorola
* MC146818A or Dallas DS12887 data sheet for details.
+ *
+ * BUG: This routine does not handle hour overflow properly; it just
+ * sets the minutes. Usually you'll only notice that after reboot!
*/
static int set_rtc_mmss(unsigned long nowtime)
{
@@ -318,8 +325,12 @@
}
CMOS_WRITE(real_seconds,RTC_SECONDS);
CMOS_WRITE(real_minutes,RTC_MINUTES);
- } else
+ } else {
+ printk(KERN_WARNING
+ "set_rtc_mmss: can't update from %d to %d\n",
+ cmos_minutes, real_minutes);
retval = -1;
+ }

/* The following flags have to be released exactly in this order,
* otherwise the DS12887 (popular MC146818A clone with integrated
diff -r -u --new-file /usr/src/linux-2.0.31-pre9/include/linux/timex.h
/usr/src/linux/include/linux/timex.h
--- /usr/src/linux-2.0.31-pre9/include/linux/timex.h Sat May 4 18:39:22 1996
+++ /usr/src/linux/include/linux/timex.h Tue Sep 9 21:17:46 1997
@@ -38,6 +38,8 @@
* Derived linux/timex.h
* 1995-08-13 Torsten Duwe
* kernel PLL updated to 1994-12-13 specs (rfc-1589)
+ * 1997-08-30 Ulrich Windl
+ * Added new constant NTP_PHASE_LIMIT
*/
#ifndef _LINUX_TIMEX_H
#define _LINUX_TIMEX_H
@@ -95,6 +97,7 @@
#define MAXTIME (200L << PPS_AVG) /* max PPS error (jitter) (200 us) */
#define MINSEC 16L /* min interval between updates (s) */
#define MAXSEC 1200L /* max interval between updates (s) */
+#define NTP_PHASE_LIMIT (MAXPHASE << 5) /* beyond max. dispersion */

/*
* The following defines are used only if a pulse-per-second (PPS)
diff -r -u --new-file /usr/src/linux-2.0.31-pre9/kernel/time.c
/usr/src/linux/kernel/time.c
--- /usr/src/linux-2.0.31-pre9/kernel/time.c Tue Sep 9 19:09:49 1997
+++ /usr/src/linux/kernel/time.c Sat Sep 13 19:48:48 1997
@@ -195,14 +494,14 @@
asmlinkage int sys_adjtimex(struct timex *txc_p)
{
long ltemp, mtemp, save_adjust;
- int error;
+ int error = 0;

/* Local copy of parameter */
struct timex txc;

error = verify_area(VERIFY_WRITE, txc_p, sizeof(struct timex));
if (error)
- return error;
+ return error; /* do not write results */

/* Copy the user data space into the kernel copy
* structure. But bear in mind that the structures
@@ -216,112 +515,139 @@

/* Now we validate the data before disabling interrupts
*/
-
if (txc.modes != ADJ_OFFSET_SINGLESHOT && (txc.modes & ADJ_OFFSET))
- /* adjustment Offset limited to +- .512 seconds */
- if (txc.offset <= - MAXPHASE || txc.offset >= MAXPHASE )
- return -EINVAL;
-
- /* if the quartz is off by more than 10% something is VERY wrong ! */
- if (txc.modes & ADJ_TICK)
- if (txc.tick < 900000/HZ || txc.tick > 1100000/HZ)
- return -EINVAL;
+ /* adjustment Offset limited to +- .512 seconds */
+ if (txc.offset <= - MAXPHASE || txc.offset >= MAXPHASE )
+ return -EINVAL;

cli();

- /* Save for later - semantics of adjtime is to return old value */
+ /* Save for later - semantics of adjtime() is to return old value */
save_adjust = time_adjust;

/* If there are input parameters, then process them */
if (txc.modes)
{
- if (time_state == TIME_BAD)
- time_state = TIME_OK;
-
- if (txc.modes & ADJ_STATUS)
- time_status = txc.status;
+ if (time_state == TIME_ERROR)
+ time_state = TIME_OK; /* reset error -- why? */

- if (txc.modes & ADJ_FREQUENCY)
+ if (txc.modes & ADJ_STATUS) /* only set allowed bits */
+ time_status = (txc.status & ~STA_RONLY) |
+ (time_status & STA_RONLY);
+
+ if (txc.modes & ADJ_FREQUENCY) { /* p. 22 */
+ if (txc.freq > MAXFREQ || txc.freq < -MAXFREQ) {
+ error = -EINVAL;
+ goto leave;
+ }
time_freq = txc.freq;
+ }

- if (txc.modes & ADJ_MAXERROR)
+ if (txc.modes & ADJ_MAXERROR) {
+ if (txc.maxerror < 0 || txc.maxerror >= NTP_PHASE_LIMIT) {
+ error = -EINVAL;
+ goto leave;
+ }
time_maxerror = txc.maxerror;
+ }

- if (txc.modes & ADJ_ESTERROR)
+ if (txc.modes & ADJ_ESTERROR) {
+ if (txc.esterror < 0 || txc.esterror >= NTP_PHASE_LIMIT) {
+ error = -EINVAL;
+ goto leave;
+ }
time_esterror = txc.esterror;
+ }

- if (txc.modes & ADJ_TIMECONST)
+ if (txc.modes & ADJ_TIMECONST) { /* p. 24 */
+ if (txc.constant < 0 || txc.constant > MAXTC) {
+ error = -EINVAL;
+ goto leave;
+ }
time_constant = txc.constant;
+ }

- if (txc.modes & ADJ_OFFSET)
- if ((txc.modes == ADJ_OFFSET_SINGLESHOT)
- || !(time_status & STA_PLL))
- {
- time_adjust = txc.offset;
- }
- else if ((time_status & STA_PLL)||(time_status & STA_PPSTIME))
- {
- ltemp = (time_status & STA_PPSTIME &&
- time_status & STA_PPSSIGNAL) ?
- pps_offset : txc.offset;
-
- /*
- * Scale the phase adjustment and
- * clamp to the operating range.
- */
- if (ltemp > MAXPHASE)
- time_offset = MAXPHASE << SHIFT_UPDATE;
- else if (ltemp < -MAXPHASE)
- time_offset = -(MAXPHASE << SHIFT_UPDATE);
- else
- time_offset = ltemp << SHIFT_UPDATE;
-
- /*
- * Select whether the frequency is to be controlled and in which
- * mode (PLL or FLL). Clamp to the operating range. Ugly
- * multiply/divide should be replaced someday.
- */
-
- if (time_status & STA_FREQHOLD || time_reftime == 0)
+ if (txc.modes & ADJ_OFFSET) { /* values checked earlier */
+ if (txc.modes == ADJ_OFFSET_SINGLESHOT) {
+ /* adjtime() is independent from ntp_adjtime() */
+ time_adjust = txc.offset;
+ }
+ else if ( time_status & (STA_PLL | STA_PPSTIME) ) {
+ ltemp = (time_status & (STA_PPSTIME | STA_PPSSIGNAL)) ==
+ (STA_PPSTIME | STA_PPSSIGNAL) ?
+ pps_offset : txc.offset;
+
+ /*
+ * Scale the phase adjustment and
+ * clamp to the operating range.
+ */
+ if (ltemp > MAXPHASE)
+ time_offset = MAXPHASE << SHIFT_UPDATE;
+ else if (ltemp < -MAXPHASE)
+ time_offset = -(MAXPHASE << SHIFT_UPDATE);
+ else
+ time_offset = ltemp << SHIFT_UPDATE;
+
+ /*
+ * Select whether the frequency is to be controlled
+ * and in which mode (PLL or FLL). Clamp to the operating
+ * range. Ugly multiply/divide should be replaced someday.
+ */
+
+ if (time_status & STA_FREQHOLD || time_reftime == 0)
+ time_reftime = xtime.tv_sec;
+ mtemp = xtime.tv_sec - time_reftime;
time_reftime = xtime.tv_sec;
- mtemp = xtime.tv_sec - time_reftime;
- time_reftime = xtime.tv_sec;
- if (time_status & STA_FLL)
- {
- if (mtemp >= MINSEC)
- {
- ltemp = ((time_offset / mtemp) << (SHIFT_USEC -
- SHIFT_UPDATE));
- if (ltemp < 0)
- time_freq -= -ltemp >> SHIFT_KH;
- else
- time_freq += ltemp >> SHIFT_KH;
- }
- }
- else
- {
- if (mtemp < MAXSEC)
- {
- ltemp *= mtemp;
- if (ltemp < 0)
- time_freq -= -ltemp >> (time_constant +
- time_constant + SHIFT_KF -
- SHIFT_USEC);
- else
- time_freq += ltemp >> (time_constant +
- time_constant + SHIFT_KF -
- SHIFT_USEC);
- }
+ if (time_status & STA_FLL) {
+ if (mtemp >= MINSEC) {
+ ltemp = (time_offset / mtemp) << (SHIFT_USEC -
+ SHIFT_UPDATE);
+ if (ltemp < 0)
+ time_freq -= -ltemp >> SHIFT_KH;
+ else
+ time_freq += ltemp >> SHIFT_KH;
+ } else /* calibration interval too short (p. 12) */
+ time_state = TIME_ERROR;
+ } else { /* PLL mode */
+ if (mtemp < MAXSEC) {
+ ltemp *= mtemp;
+ if (ltemp < 0)
+ time_freq -= -ltemp >> (time_constant +
+ time_constant +
+ SHIFT_KF - SHIFT_USEC);
+ else
+ time_freq += ltemp >> (time_constant +
+ time_constant +
+ SHIFT_KF - SHIFT_USEC);
+ } else /* calibration interval too long (p. 12) */
+ time_state = TIME_ERROR;
}
- if (time_freq > time_tolerance)
- time_freq = time_tolerance;
- else if (time_freq < -time_tolerance)
- time_freq = -time_tolerance;
+ if (time_freq > time_tolerance)
+ time_freq = time_tolerance;
+ else if (time_freq < -time_tolerance)
+ time_freq = -time_tolerance;
} /* STA_PLL || STA_PPSTIME */
- if (txc.modes & ADJ_TICK)
- tick = txc.tick;
-
- }
+ } /* txc.modes & ADJ_OFFSET */
+ if (txc.modes & ADJ_TICK) {
+ /* if the quartz is off by more than 10% something is
+ VERY wrong ! */
+ if (txc.tick < 900000/HZ || txc.tick > 1100000/HZ) {
+ error = -EINVAL;
+ goto leave;
+ }
+ tick = txc.tick;
+ }
+ } /* txc.modes */
+leave: if (((time_status & STA_PPSSIGNAL) == 0
+ && (time_status & (STA_PPSFREQ|STA_PPSTIME)) != 0)
+ /* p. 24, (b) */
+ || ((time_status
+ & (STA_PPSTIME|STA_PPSJITTER)) == (STA_PPSTIME|STA_PPSJITTER))
+ /* p. 24, (c) */
+ || ((time_status & STA_PPSFREQ) != 0
+ && (time_status & (STA_PPSWANDER|STA_PPSERROR)) != 0))
+ /* p. 24, (d) */
+ time_state = TIME_ERROR;
txc.offset = save_adjust;
txc.freq = time_freq;
txc.maxerror = time_maxerror;
@@ -344,5 +670,5 @@
sti();

memcpy_tofs(txc_p, &txc, sizeof(struct timex));
- return time_state;
+ return error ? error :time_state;
}