[RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Ondrej Mosnacek]

This patchset implements auditing of (syscall-triggered) changes that
can modify or indirectly affect the system clock. Some of these
changes can already be detected by simply logging relevant syscalls,
but this has some disadvantages:
  a) It is usually not possible to find out from the syscall records
     the amount by which the time was shifted.
  b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
     for read-only operations, which might flood the audit log with
     false positives. (Note that these patches don't solve this
     problem yet due to the limitations of current record filtering
     capabilities.)

The main motivation is to provide better reliability of timestamps
on the system as mandated by the FPT_STM.1 security functional
requirement from Common Criteria. This requirement apparently demands
that it is possible to reconstruct from audit trail the old and new
values of the time when it is adjusted (see [1]).

The current version of the patchset logs the following changes:
  - direct setting of system time to a given value
  - direct injection of timekeeping offset
  - adjustment of timekeeping's TAI offset
  - NTP value adjustments:
    - time_offset
    - time_freq
    - time_status
    - time_adjust
    - tick_usec

Changes to the following NTP values are not logged, as they are not
important for security:
  - time_maxerror
  - time_esterror
  - time_constant

Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock

Testing: Passed audit-testuite; functional tests TBD

Changes in v6:
  - Reorganized the patches to group changes by record type, not
    kernel subsytem, as suggested in earlier discussions
  - Added checks to ignore no-change events (new value == old value)
  - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
    syscalls such as settimeofday(2), stime(2), clock_settime(2)
  - Created an RFE page on audit-kernel GitHub
TODO:
  - tests for audit-testsuite

v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
Changes in v5:
  - Dropped logging of some less important changes and update commit messages
  - No longer mark the patchset as RFC

v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
Changes in v4:
  - Squashed first two patches into one
  - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
    conventions
  - Minor commit message editing
  - Cc timekeeping/NTP people for feedback

v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
Changes in v3:
  - Switched to separate records for each variable
  - Both old and new value is now reported for each change
  - Injecting offset is reported via a separate record (since this
    offset consists of two values and is added directly to the clock,
    i.e. it doesn't make sense to log old and new value)
  - Added example records produced by chronyd -q (see the commit message
    of the last patch)

v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
Changes in v2:
  - The audit_adjtime() function has been modified to only log those
    fields that contain values that are actually used, resulting in more
    compact records.
  - The audit_adjtime() call has been moved to do_adjtimex() in
    timekeeping.c
  - Added an additional patch (for review) that simplifies the detection
    if the syscall is read-only.

v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html

[1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
    table 4

Ondrej Mosnacek (2):
  timekeeping: Audit clock adjustments
  ntp: Audit NTP parameters adjustment

 include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
 include/uapi/linux/audit.h |  2 ++
 kernel/auditsc.c           | 15 +++++++++++++++
 kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
 kernel/time/timekeeping.c  |  6 ++++++
 5 files changed, 82 insertions(+), 8 deletions(-)

-- 
2.20.1

[RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Ondrej Mosnacek]

Emit an audit record whenever the system clock is changed (i.e. shifted
by a non-zero offset) by a syscall from userspace. The syscalls than can
(at the time of writing) trigger such record are:
  - settimeofday(2), stime(2), clock_settime(2) -- via
    do_settimeofday64()
  - adjtimex(2), clock_adjtime(2) -- via do_adjtimex()

The new records have type AUDIT_TIME_INJOFFSET and contain the following
fields:
  - sec -- the 'seconds' part of the offset
  - nsec -- the 'nanoseconds' part of the offset

For reference, running the following commands:

    auditctl -D
    auditctl -a exit,always -F arch=b64 -S adjtimex
    chronyd -q

triggers (among others) a syscall that produces audit records like this:

type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71 cd /home/omosnace/Dokumenty/Kernel/worktrees/audit/src/kernel/time
s

The above records have been produced by the following syscall from
chronyd (as per strace output):

adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)

(The struct timex fields above are from *after* the syscall was
executed, so they contain the current (new) values as set from the
kernel, except of the 'modes' field, which contains the original value
sent by the caller.)

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/audit.h      | 15 +++++++++++++++
 include/uapi/linux/audit.h |  1 +
 kernel/auditsc.c           |  8 ++++++++
 kernel/time/timekeeping.c  |  6 ++++++
 4 files changed, 30 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 1e69d9fe16da..43a60fbe74be 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -27,6 +27,7 @@
 #include <linux/ptrace.h>
 #include <linux/namei.h>  /* LOOKUP_* */
 #include <uapi/linux/audit.h>
+#include <uapi/linux/timex.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -365,6 +366,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
 extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_log_kern_module(char *name);
 extern void __audit_fanotify(unsigned int response);
+extern void __audit_tk_injoffset(struct timespec64 offset);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -467,6 +469,16 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
+static inline void audit_tk_injoffset(struct timespec64 offset)
+{
+	/* ignore no-op events */
+	if (offset.tv_sec == 0 && offset.tv_nsec == 0)
+		return;
+
+	if (!audit_dummy_context())
+		__audit_tk_injoffset(offset);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -580,6 +592,9 @@ static inline void audit_log_kern_module(char *name)
 static inline void audit_fanotify(unsigned int response)
 { }
 
+static inline void audit_tk_injoffset(struct timespec64 offset)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 36a7e3f18e69..2167d55bc800 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -114,6 +114,7 @@
 #define AUDIT_REPLACE		1329	/* Replace auditd if this packet unanswerd */
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
+#define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index d1eab1d4a930..781336d0f2de 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+/* We need to allocate with GFP_ATOMIC here, since these two functions will be
+ * called while holding the timekeeping lock: */
+void __audit_tk_injoffset(struct timespec64 offset)
+{
+	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
+		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ac5dbf2cd4a2..0f0b566afe61 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -21,6 +21,7 @@
 #include <linux/stop_machine.h>
 #include <linux/pvclock_gtod.h>
 #include <linux/compiler.h>
+#include <linux/audit.h>
 
 #include "tick-internal.h"
 #include "ntp_internal.h"
@@ -1250,6 +1251,9 @@ out:
 	/* signal hrtimers about time change */
 	clock_was_set();
 
+	if (!ret)
+		audit_tk_injoffset(ts_delta);
+
 	return ret;
 }
 EXPORT_SYMBOL(do_settimeofday64);
@@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
 		ret = timekeeping_inject_offset(&delta);
 		if (ret)
 			return ret;
+
+		audit_tk_injoffset(delta);
 	}
 
 	ktime_get_real_ts64(&ts);
-- 
2.20.1

[RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Ondrej Mosnacek]

Emit an audit record every time selected NTP parameters are modified
from userspace (via adjtimex(2) or clock_adjtime(2)).

Such events will now generate records of type AUDIT_TIME_ADJNTPVAL
containing the following fields:
  - op -- which value was adjusted:
    - offset -- corresponding to the time_offset variable
    - freq   -- corresponding to the time_freq variable
    - status -- corresponding to the time_status variable
    - adjust -- corresponding to the time_adjust variable
    - tick   -- corresponding to the tick_usec variable
    - tai    -- corresponding to the timekeeping's TAI offset
  - old -- the old value
  - new -- the new value

For reference, running the following commands:

    auditctl -D
    auditctl -a exit,always -F arch=b64 -S adjtimex
    chronyd -q

produces audit records like this:

type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:5): proctitle=6368726F6E7964002D71
type=SYSCALL msg=audit(1530616044.507:6): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:6): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
type=SYSCALL msg=audit(1530616044.507:7): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:7): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): op=status old=8256 new=8257
type=SYSCALL msg=audit(1530616044.507:8): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:8): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.507:9): op=status old=8257 new=64
type=SYSCALL msg=audit(1530616044.507:9): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:9): proctitle=6368726F6E7964002D71
type=SYSCALL msg=audit(1530616044.507:10): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78a70 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.507:10): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000
type=SYSCALL msg=audit(1530616044.511:11): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ad0 a1=0 a2=2710 a3=f42f82a800000 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.511:11): proctitle=6368726F6E7964002D71
type=SYSCALL msg=audit(1530616044.521:12): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78b40 a1=1 a2=40 a3=f91f6ef84fbab items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616044.521:12): proctitle=6368726F6E7964002D71
type=TIME_ADJNTPVAL msg=audit(1530616049.652:13): op=status old=64 new=8256
type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71
type=SYSCALL msg=audit(1530616033.783:14): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78bc0 a1=0 a2=2710 a3=0 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
type=PROCTITLE msg=audit(1530616033.783:14): proctitle=6368726F6E7964002D71

The chronyd command that produced the above records executed the
following adjtimex(2) syscalls (as per strace output):

adjtimex({modes=ADJ_OFFSET|0x8000, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507215}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_MAXERROR, offset=0, freq=0, maxerror=0, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507438}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507604737}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_OFFSET|ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_PLL|STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507698330}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507792}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=0, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=508000}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=512146}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_MAXERROR|ADJ_ESTERROR|ADJ_STATUS, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=522506}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=784644657}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)

(The struct timex fields above are from *after* the syscall was
executed, so they contain the current (new) values as set from the
kernel, except of the 'modes' field, which contains the original value
sent by the caller.)

The changes to the time_maxerror, time_esterror, and time_constant
variables are not logged, as these are not important for security. Also,
no-op adjustments that do not actually change the value are not logged.

An overview of parameter changes that can be done via do_adjtimex()
(based on information from Miroslav Lichvar) and whether they are
audited:
  __timekeeping_set_tai_offset() -- sets the offset from the
                                    International Atomic Time
                                    (AUDITED)
  NTP variables:
    time_offset -- can adjust the clock by up to 0.5 seconds per call
                   and also speed it up or slow down by up to about
                   0.05% (43 seconds per day) (AUDITED)
    time_freq -- can speed up or slow down by up to about 0.05%
    time_status -- can insert/delete leap seconds and it also enables/
                   disables synchronization of the hardware real-time
                   clock (AUDITED)
    time_maxerror, time_esterror -- change error estimates used to
                                    inform userspace applications
                                    (NOT AUDITED)
    time_constant -- controls the speed of the clock adjustments that
                     are made when time_offset is set (NOT AUDITED)
    time_adjust -- can temporarily speed up or slow down the clock by up
                   to 0.05% (AUDITED)
    tick_usec -- a more extreme version of time_freq; can speed up or
                 slow down the clock by up to 10% (AUDITED)

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/audit.h      | 14 ++++++++++++++
 include/uapi/linux/audit.h |  1 +
 kernel/auditsc.c           |  7 +++++++
 kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
 4 files changed, 52 insertions(+), 8 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 43a60fbe74be..0f67964544cc 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -367,6 +367,7 @@ extern void __audit_mmap_fd(int fd, int flags);
 extern void __audit_log_kern_module(char *name);
 extern void __audit_fanotify(unsigned int response);
 extern void __audit_tk_injoffset(struct timespec64 offset);
+extern void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -479,6 +480,16 @@ static inline void audit_tk_injoffset(struct timespec64 offset)
 		__audit_tk_injoffset(offset);
 }
 
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+	/* ignore no-op events */
+	if (newval == oldval)
+		return;
+
+	if (!audit_dummy_context())
+		__audit_ntp_adjust(type, oldval, newval);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -595,6 +606,9 @@ static inline void audit_fanotify(unsigned int response)
 static inline void audit_tk_injoffset(struct timespec64 offset)
 { }
 
+static inline void audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 2167d55bc800..e9781f0385eb 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -115,6 +115,7 @@
 #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
 #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
 #define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
+#define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 781336d0f2de..946806174cd9 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2520,6 +2520,13 @@ void __audit_tk_injoffset(struct timespec64 offset)
 		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
 }
 
+void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
+{
+	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,
+		  "op=%s old=%lli new=%lli", type,
+		  (long long)oldval, (long long)newval);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
index 36a2bef00125..5f456a84151a 100644
--- a/kernel/time/ntp.c
+++ b/kernel/time/ntp.c
@@ -17,6 +17,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/rtc.h>
+#include <linux/audit.h>
 
 #include "ntp_internal.h"
 #include "timekeeping_internal.h"
@@ -293,6 +294,8 @@ static inline s64 ntp_update_offset_fll(s64 offset64, long secs)
 
 static void ntp_update_offset(long offset)
 {
+	s64 old_offset = time_offset;
+	s64 old_freq = time_freq;
 	s64 freq_adj;
 	s64 offset64;
 	long secs;
@@ -341,6 +344,9 @@ static void ntp_update_offset(long offset)
 	time_freq   = max(freq_adj, -MAXFREQ_SCALED);
 
 	time_offset = div_s64(offset64 << NTP_SCALE_SHIFT, NTP_INTERVAL_FREQ);
+
+	audit_ntp_adjust("offset", old_offset, time_offset);
+	audit_ntp_adjust("freq", old_freq, time_freq);
 }
 
 /**
@@ -658,21 +664,31 @@ static inline void process_adj_status(const struct timex *txc)
 
 static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai)
 {
-	if (txc->modes & ADJ_STATUS)
-		process_adj_status(txc);
+	if (txc->modes & (ADJ_STATUS | ADJ_NANO | ADJ_MICRO)) {
+		int old_status = time_status;
+
+		if (txc->modes & ADJ_STATUS)
+			process_adj_status(txc);
 
-	if (txc->modes & ADJ_NANO)
-		time_status |= STA_NANO;
+		if (txc->modes & ADJ_NANO)
+			time_status |= STA_NANO;
 
-	if (txc->modes & ADJ_MICRO)
-		time_status &= ~STA_NANO;
+		if (txc->modes & ADJ_MICRO)
+			time_status &= ~STA_NANO;
+
+		audit_ntp_adjust("status", old_status, time_status);
+	}
 
 	if (txc->modes & ADJ_FREQUENCY) {
+		s64 old_freq = time_freq;
+
 		time_freq = txc->freq * PPM_SCALE;
 		time_freq = min(time_freq, MAXFREQ_SCALED);
 		time_freq = max(time_freq, -MAXFREQ_SCALED);
 		/* update pps_freq */
 		pps_set_freq(time_freq);
+
+		audit_ntp_adjust("freq", old_freq, time_freq);
 	}
 
 	if (txc->modes & ADJ_MAXERROR)
@@ -689,14 +705,18 @@ static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai
 		time_constant = max(time_constant, 0l);
 	}
 
-	if (txc->modes & ADJ_TAI && txc->constant > 0)
+	if (txc->modes & ADJ_TAI && txc->constant > 0) {
+		audit_ntp_adjust("tai", *time_tai, txc->constant);
 		*time_tai = txc->constant;
+	}
 
 	if (txc->modes & ADJ_OFFSET)
 		ntp_update_offset(txc->offset);
 
-	if (txc->modes & ADJ_TICK)
+	if (txc->modes & ADJ_TICK) {
+		audit_ntp_adjust("tick", tick_usec, txc->tick);
 		tick_usec = txc->tick;
+	}
 
 	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
 		ntp_update_frequency();
@@ -718,6 +738,8 @@ int __do_adjtimex(struct timex *txc, const struct timespec64 *ts, s32 *time_tai)
 			/* adjtime() is independent from ntp_adjtime() */
 			time_adjust = txc->offset;
 			ntp_update_frequency();
+
+			audit_ntp_adjust("adjust", save_adjust, txc->offset);
 		}
 		txc->offset = save_adjust;
 	} else {
-- 
2.20.1

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Steve Grubb]

On Thursday, March 7, 2019 7:32:53 AM EST Ondrej Mosnacek wrote:
> Emit an audit record whenever the system clock is changed (i.e. shifted
> by a non-zero offset) by a syscall from userspace. The syscalls than can
> (at the time of writing) trigger such record are:
>   - settimeofday(2), stime(2), clock_settime(2) -- via
>     do_settimeofday64()
>   - adjtimex(2), clock_adjtime(2) -- via do_adjtimex()
> 
> The new records have type AUDIT_TIME_INJOFFSET and contain the following
> fields:
>   - sec -- the 'seconds' part of the offset
>   - nsec -- the 'nanoseconds' part of the offset
> 
> For reference, running the following commands:
> 
>     auditctl -D
>     auditctl -a exit,always -F arch=b64 -S adjtimex
>     chronyd -q
> 
> triggers (among others) a syscall that produces audit records like this:
> 
> type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
> type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159
> success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0
> a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385
> suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616049.652:13):
> proctitle=6368726F6E7964002D71 cd
> /home/omosnace/Dokumenty/Kernel/worktrees/audit/src/kernel/time s

This is needed for common criteria. Requirements are getting stricter in 
certifications of IT products that are time stamp sensitive. The record format 
looks fine to me.

Ack for the record format.

-Steve

> The above records have been produced by the following syscall from
> chronyd (as per strace output):
> 
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033,
> tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> 
> (The struct timex fields above are from *after* the syscall was
> executed, so they contain the current (new) values as set from the
> kernel, except of the 'modes' field, which contains the original value
> sent by the caller.)
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h      | 15 +++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/auditsc.c           |  8 ++++++++
>  kernel/time/timekeeping.c  |  6 ++++++
>  4 files changed, 30 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..43a60fbe74be 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -27,6 +27,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/namei.h>  /* LOOKUP_* */
>  #include <uapi/linux/audit.h>
> +#include <uapi/linux/timex.h>
> 
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -365,6 +366,7 @@ extern void __audit_log_capset(const struct cred *new,
> const struct cred *old); extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_log_kern_module(char *name);
>  extern void __audit_fanotify(unsigned int response);
> +extern void __audit_tk_injoffset(struct timespec64 offset);
> 
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -467,6 +469,16 @@ static inline void audit_fanotify(unsigned int
> response) __audit_fanotify(response);
>  }
> 
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{
> +	/* ignore no-op events */
> +	if (offset.tv_sec == 0 && offset.tv_nsec == 0)
> +		return;
> +
> +	if (!audit_dummy_context())
> +		__audit_tk_injoffset(offset);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -580,6 +592,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
> 
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 36a7e3f18e69..2167d55bc800 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -114,6 +114,7 @@
>  #define AUDIT_REPLACE		1329	/* Replace auditd if this packet 
unanswerd */
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
>  #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
> +#define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
> 
>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1eab1d4a930..781336d0f2de 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
> 
> +/* We need to allocate with GFP_ATOMIC here, since these two functions
> will be + * called while holding the timekeeping lock: */
> +void __audit_tk_injoffset(struct timespec64 offset)
> +{
> +	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> +		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index ac5dbf2cd4a2..0f0b566afe61 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -21,6 +21,7 @@
>  #include <linux/stop_machine.h>
>  #include <linux/pvclock_gtod.h>
>  #include <linux/compiler.h>
> +#include <linux/audit.h>
> 
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1250,6 +1251,9 @@ out:
>  	/* signal hrtimers about time change */
>  	clock_was_set();
> 
> +	if (!ret)
> +		audit_tk_injoffset(ts_delta);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(do_settimeofday64);
> @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
>  		ret = timekeeping_inject_offset(&delta);
>  		if (ret)
>  			return ret;
> +
> +		audit_tk_injoffset(delta);
>  	}
> 
>  	ktime_get_real_ts64(&ts);

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Steve Grubb]

On Thursday, March 7, 2019 7:32:54 AM EST Ondrej Mosnacek wrote:
> Emit an audit record every time selected NTP parameters are modified
> from userspace (via adjtimex(2) or clock_adjtime(2)).
> 
> Such events will now generate records of type AUDIT_TIME_ADJNTPVAL
> containing the following fields:
>   - op -- which value was adjusted:
>     - offset -- corresponding to the time_offset variable
>     - freq   -- corresponding to the time_freq variable
>     - status -- corresponding to the time_status variable
>     - adjust -- corresponding to the time_adjust variable
>     - tick   -- corresponding to the tick_usec variable
>     - tai    -- corresponding to the timekeeping's TAI offset
>   - old -- the old value
>   - new -- the new value
> 
> For reference, running the following commands:
> 
>     auditctl -D
>     auditctl -a exit,always -F arch=b64 -S adjtimex
>     chronyd -q
> 
> produces audit records like this:
> 
> type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159
> success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0
> ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0
> fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd"
> subj=system_u:system_r:kernel_t:s0 key=(null) type=PROCTITLE
> msg=audit(1530616044.507:5): proctitle=6368726F6E7964002D71 type=SYSCALL
> msg=audit(1530616044.507:6): arch=c000003e syscall=159 success=yes exit=5
> a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0
> uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616044.507:6):
> proctitle=6368726F6E7964002D71 type=TIME_ADJNTPVAL
> msg=audit(1530616044.507:7): op=status old=64 new=8256 type=SYSCALL
> msg=audit(1530616044.507:7): arch=c000003e syscall=159 success=yes exit=5
> a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0
> uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616044.507:7):
> proctitle=6368726F6E7964002D71 type=TIME_ADJNTPVAL
> msg=audit(1530616044.507:8): op=status old=8256 new=8257 type=SYSCALL
> msg=audit(1530616044.507:8): arch=c000003e syscall=159 success=yes exit=5
> a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626
> pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd"
> subj=system_u:system_r:kernel_t:s0 key=(null) type=PROCTITLE
> msg=audit(1530616044.507:8): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:9): op=status old=8257 new=64
> type=SYSCALL msg=audit(1530616044.507:9): arch=c000003e syscall=159
> success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a
> items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0
> sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd"
> subj=system_u:system_r:kernel_t:s0 key=(null) type=PROCTITLE
> msg=audit(1530616044.507:9): proctitle=6368726F6E7964002D71 type=SYSCALL
> msg=audit(1530616044.507:10): arch=c000003e syscall=159 success=yes exit=5
> a0=7fff57e78a70 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626
> pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0
> tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd"
> subj=system_u:system_r:kernel_t:s0 key=(null) type=PROCTITLE
> msg=audit(1530616044.507:10): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0
> new=49180377088000 type=SYSCALL msg=audit(1530616044.511:11):
> arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ad0 a1=0 a2=2710
> a3=f42f82a800000 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385
> suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616044.511:11):
> proctitle=6368726F6E7964002D71 type=SYSCALL msg=audit(1530616044.521:12):
> arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78b40 a1=1 a2=40
> a3=f91f6ef84fbab items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385
> suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1
> comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0
> key=(null) type=PROCTITLE msg=audit(1530616044.521:12):
> proctitle=6368726F6E7964002D71 type=TIME_ADJNTPVAL
> msg=audit(1530616049.652:13): op=status old=64 new=8256 type=SYSCALL
> msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5
> a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0
> ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385
> egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd"
> exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616049.652:13):
> proctitle=6368726F6E7964002D71 type=SYSCALL msg=audit(1530616033.783:14):
> arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78bc0 a1=0 a2=2710
> a3=0 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385
> fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd"
> exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616033.783:14):
> proctitle=6368726F6E7964002D71
> 
> The chronyd command that produced the above records executed the
> following adjtimex(2) syscalls (as per strace output):
> 
> adjtimex({modes=ADJ_OFFSET|0x8000, offset=0, freq=0, maxerror=16000000,
> esterror=16000000, status=STA_UNSYNC, constant=2, precision=1,
> tolerance=32768000, time={tv_sec=1530616044, tv_usec=507215}, tick=10000,
> ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0,
> stbcnt=0, tai=0}) = 5 (TIME_ERROR) adjtimex({modes=ADJ_MAXERROR, offset=0,
> freq=0, maxerror=0, esterror=16000000, status=STA_UNSYNC, constant=2,
> precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507438},
> tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0,
> errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=0,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044,
> tv_usec=507604737}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_OFFSET|ADJ_STATUS, offset=0, freq=0,
> maxerror=16000000, esterror=16000000, status=STA_PLL|STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044,
> tv_usec=507698330}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_STATUS, offset=0, freq=0, maxerror=16000000,
> esterror=16000000, status=STA_UNSYNC, constant=2, precision=1,
> tolerance=32768000, time={tv_sec=1530616044, tv_usec=507792}, tick=10000,
> ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0,
> stbcnt=0, tai=0}) = 5 (TIME_ERROR) adjtimex({modes=0, offset=0, freq=0,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2,
> precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=508000},
> tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0,
> errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2,
> precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=512146},
> tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0,
> errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_MAXERROR|ADJ_ESTERROR|ADJ_STATUS, offset=0,
> freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044,
> tv_usec=522506}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033,
> tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433,
> maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO,
> constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033,
> tv_usec=784644657}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0,
> jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)

This is needed for common criteria. Requirements are getting stricter in 
certifications of IT products that are time stamp sensitive. The new record 
format looks fine to me.

Ack for the record format.

-Steve

> (The struct timex fields above are from *after* the syscall was
> executed, so they contain the current (new) values as set from the
> kernel, except of the 'modes' field, which contains the original value
> sent by the caller.)
> 
> The changes to the time_maxerror, time_esterror, and time_constant
> variables are not logged, as these are not important for security. Also,
> no-op adjustments that do not actually change the value are not logged.
> 
> An overview of parameter changes that can be done via do_adjtimex()
> (based on information from Miroslav Lichvar) and whether they are
> audited:
>   __timekeeping_set_tai_offset() -- sets the offset from the
>                                     International Atomic Time
>                                     (AUDITED)
>   NTP variables:
>     time_offset -- can adjust the clock by up to 0.5 seconds per call
>                    and also speed it up or slow down by up to about
>                    0.05% (43 seconds per day) (AUDITED)
>     time_freq -- can speed up or slow down by up to about 0.05%
>     time_status -- can insert/delete leap seconds and it also enables/
>                    disables synchronization of the hardware real-time
>                    clock (AUDITED)
>     time_maxerror, time_esterror -- change error estimates used to
>                                     inform userspace applications
>                                     (NOT AUDITED)
>     time_constant -- controls the speed of the clock adjustments that
>                      are made when time_offset is set (NOT AUDITED)
>     time_adjust -- can temporarily speed up or slow down the clock by up
>                    to 0.05% (AUDITED)
>     tick_usec -- a more extreme version of time_freq; can speed up or
>                  slow down the clock by up to 10% (AUDITED)
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h      | 14 ++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/auditsc.c           |  7 +++++++
>  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
>  4 files changed, 52 insertions(+), 8 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 43a60fbe74be..0f67964544cc 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -367,6 +367,7 @@ extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_log_kern_module(char *name);
>  extern void __audit_fanotify(unsigned int response);
>  extern void __audit_tk_injoffset(struct timespec64 offset);
> +extern void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval);
> 
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -479,6 +480,16 @@ static inline void audit_tk_injoffset(struct
> timespec64 offset) __audit_tk_injoffset(offset);
>  }
> 
> +static inline void audit_ntp_adjust(const char *type, s64 oldval, s64
> newval) +{
> +	/* ignore no-op events */
> +	if (newval == oldval)
> +		return;
> +
> +	if (!audit_dummy_context())
> +		__audit_ntp_adjust(type, oldval, newval);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -595,6 +606,9 @@ static inline void audit_fanotify(unsigned int
> response) static inline void audit_tk_injoffset(struct timespec64 offset)
>  { }
> 
> +static inline void audit_ntp_adjust(const char *type, s64 oldval, s64
> newval) +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 2167d55bc800..e9781f0385eb 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -115,6 +115,7 @@
>  #define AUDIT_KERN_MODULE	1330	/* Kernel Module events */
>  #define AUDIT_FANOTIFY		1331	/* Fanotify access decision */
>  #define AUDIT_TIME_INJOFFSET	1332	/* Timekeeping offset injected */
> +#define AUDIT_TIME_ADJNTPVAL	1333	/* NTP value adjustment */
> 
>  #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 781336d0f2de..946806174cd9 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2520,6 +2520,13 @@ void __audit_tk_injoffset(struct timespec64 offset)
>  		  "sec=%lli nsec=%li", (long long)offset.tv_sec, 
offset.tv_nsec);
>  }
> 
> +void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
> +{
> +	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,
> +		  "op=%s old=%lli new=%lli", type,
> +		  (long long)oldval, (long long)newval);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 36a2bef00125..5f456a84151a 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -17,6 +17,7 @@
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/rtc.h>
> +#include <linux/audit.h>
> 
>  #include "ntp_internal.h"
>  #include "timekeeping_internal.h"
> @@ -293,6 +294,8 @@ static inline s64 ntp_update_offset_fll(s64 offset64,
> long secs)
> 
>  static void ntp_update_offset(long offset)
>  {
> +	s64 old_offset = time_offset;
> +	s64 old_freq = time_freq;
>  	s64 freq_adj;
>  	s64 offset64;
>  	long secs;
> @@ -341,6 +344,9 @@ static void ntp_update_offset(long offset)
>  	time_freq   = max(freq_adj, -MAXFREQ_SCALED);
> 
>  	time_offset = div_s64(offset64 << NTP_SCALE_SHIFT, NTP_INTERVAL_FREQ);
> +
> +	audit_ntp_adjust("offset", old_offset, time_offset);
> +	audit_ntp_adjust("freq", old_freq, time_freq);
>  }
> 
>  /**
> @@ -658,21 +664,31 @@ static inline void process_adj_status(const struct
> timex *txc)
> 
>  static inline void process_adjtimex_modes(const struct timex *txc, s32
> *time_tai) {
> -	if (txc->modes & ADJ_STATUS)
> -		process_adj_status(txc);
> +	if (txc->modes & (ADJ_STATUS | ADJ_NANO | ADJ_MICRO)) {
> +		int old_status = time_status;
> +
> +		if (txc->modes & ADJ_STATUS)
> +			process_adj_status(txc);
> 
> -	if (txc->modes & ADJ_NANO)
> -		time_status |= STA_NANO;
> +		if (txc->modes & ADJ_NANO)
> +			time_status |= STA_NANO;
> 
> -	if (txc->modes & ADJ_MICRO)
> -		time_status &= ~STA_NANO;
> +		if (txc->modes & ADJ_MICRO)
> +			time_status &= ~STA_NANO;
> +
> +		audit_ntp_adjust("status", old_status, time_status);
> +	}
> 
>  	if (txc->modes & ADJ_FREQUENCY) {
> +		s64 old_freq = time_freq;
> +
>  		time_freq = txc->freq * PPM_SCALE;
>  		time_freq = min(time_freq, MAXFREQ_SCALED);
>  		time_freq = max(time_freq, -MAXFREQ_SCALED);
>  		/* update pps_freq */
>  		pps_set_freq(time_freq);
> +
> +		audit_ntp_adjust("freq", old_freq, time_freq);
>  	}
> 
>  	if (txc->modes & ADJ_MAXERROR)
> @@ -689,14 +705,18 @@ static inline void process_adjtimex_modes(const
> struct timex *txc, s32 *time_tai time_constant = max(time_constant, 0l);
>  	}
> 
> -	if (txc->modes & ADJ_TAI && txc->constant > 0)
> +	if (txc->modes & ADJ_TAI && txc->constant > 0) {
> +		audit_ntp_adjust("tai", *time_tai, txc->constant);
>  		*time_tai = txc->constant;
> +	}
> 
>  	if (txc->modes & ADJ_OFFSET)
>  		ntp_update_offset(txc->offset);
> 
> -	if (txc->modes & ADJ_TICK)
> +	if (txc->modes & ADJ_TICK) {
> +		audit_ntp_adjust("tick", tick_usec, txc->tick);
>  		tick_usec = txc->tick;
> +	}
> 
>  	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
>  		ntp_update_frequency();
> @@ -718,6 +738,8 @@ int __do_adjtimex(struct timex *txc, const struct
> timespec64 *ts, s32 *time_tai) /* adjtime() is independent from
> ntp_adjtime() */
>  			time_adjust = txc->offset;
>  			ntp_update_frequency();
> +
> +			audit_ntp_adjust("adjust", save_adjust, txc->offset);
>  		}
>  		txc->offset = save_adjust;
>  	} else {

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Richard Guy Briggs]

On 2019-03-07 13:32, Ondrej Mosnacek wrote:
> This patchset implements auditing of (syscall-triggered) changes that
> can modify or indirectly affect the system clock. Some of these
> changes can already be detected by simply logging relevant syscalls,
> but this has some disadvantages:
>   a) It is usually not possible to find out from the syscall records
>      the amount by which the time was shifted.
>   b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
>      for read-only operations, which might flood the audit log with
>      false positives. (Note that these patches don't solve this
>      problem yet due to the limitations of current record filtering
>      capabilities.)
> 
> The main motivation is to provide better reliability of timestamps
> on the system as mandated by the FPT_STM.1 security functional
> requirement from Common Criteria. This requirement apparently demands
> that it is possible to reconstruct from audit trail the old and new
> values of the time when it is adjusted (see [1]).
> 
> The current version of the patchset logs the following changes:
>   - direct setting of system time to a given value
>   - direct injection of timekeeping offset
>   - adjustment of timekeeping's TAI offset
>   - NTP value adjustments:
>     - time_offset
>     - time_freq
>     - time_status
>     - time_adjust
>     - tick_usec
> 
> Changes to the following NTP values are not logged, as they are not
> important for security:
>   - time_maxerror
>   - time_esterror
>   - time_constant
> 
> Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
> Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock
> 
> Testing: Passed audit-testuite; functional tests TBD

Reviewed-by: Richard Guy Briggs <rgb@redhat.com>

How do you plan to test this in the audit-testsuite?

> Changes in v6:
>   - Reorganized the patches to group changes by record type, not
>     kernel subsytem, as suggested in earlier discussions
>   - Added checks to ignore no-change events (new value == old value)
>   - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
>     syscalls such as settimeofday(2), stime(2), clock_settime(2)
>   - Created an RFE page on audit-kernel GitHub
> TODO:
>   - tests for audit-testsuite
> 
> v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
> Changes in v5:
>   - Dropped logging of some less important changes and update commit messages
>   - No longer mark the patchset as RFC
> 
> v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
> Changes in v4:
>   - Squashed first two patches into one
>   - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
>     conventions
>   - Minor commit message editing
>   - Cc timekeeping/NTP people for feedback
> 
> v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
> Changes in v3:
>   - Switched to separate records for each variable
>   - Both old and new value is now reported for each change
>   - Injecting offset is reported via a separate record (since this
>     offset consists of two values and is added directly to the clock,
>     i.e. it doesn't make sense to log old and new value)
>   - Added example records produced by chronyd -q (see the commit message
>     of the last patch)
> 
> v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
> Changes in v2:
>   - The audit_adjtime() function has been modified to only log those
>     fields that contain values that are actually used, resulting in more
>     compact records.
>   - The audit_adjtime() call has been moved to do_adjtimex() in
>     timekeeping.c
>   - Added an additional patch (for review) that simplifies the detection
>     if the syscall is read-only.
> 
> v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
> 
> [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
>     table 4
> 
> Ondrej Mosnacek (2):
>   timekeeping: Audit clock adjustments
>   ntp: Audit NTP parameters adjustment
> 
>  include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c           | 15 +++++++++++++++
>  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
>  kernel/time/timekeeping.c  |  6 ++++++
>  5 files changed, 82 insertions(+), 8 deletions(-)
> 
> -- 
> 2.20.1
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Ondrej Mosnacek]

On Fri, Mar 8, 2019 at 9:26 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-03-07 13:32, Ondrej Mosnacek wrote:
> > This patchset implements auditing of (syscall-triggered) changes that
> > can modify or indirectly affect the system clock. Some of these
> > changes can already be detected by simply logging relevant syscalls,
> > but this has some disadvantages:
> >   a) It is usually not possible to find out from the syscall records
> >      the amount by which the time was shifted.
> >   b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
> >      for read-only operations, which might flood the audit log with
> >      false positives. (Note that these patches don't solve this
> >      problem yet due to the limitations of current record filtering
> >      capabilities.)
> >
> > The main motivation is to provide better reliability of timestamps
> > on the system as mandated by the FPT_STM.1 security functional
> > requirement from Common Criteria. This requirement apparently demands
> > that it is possible to reconstruct from audit trail the old and new
> > values of the time when it is adjusted (see [1]).
> >
> > The current version of the patchset logs the following changes:
> >   - direct setting of system time to a given value
> >   - direct injection of timekeeping offset
> >   - adjustment of timekeeping's TAI offset
> >   - NTP value adjustments:
> >     - time_offset
> >     - time_freq
> >     - time_status
> >     - time_adjust
> >     - tick_usec
> >
> > Changes to the following NTP values are not logged, as they are not
> > important for security:
> >   - time_maxerror
> >   - time_esterror
> >   - time_constant
> >
> > Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
> > Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock
> >
> > Testing: Passed audit-testuite; functional tests TBD
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
>
> How do you plan to test this in the audit-testsuite?

My plan is to add a new subtest which will use a short C program that
calls the relevant syscalls (they are listed in patch 1/2) directly
and will check if they produce the expected records. I outlined some
specific things to be tested in the RFE page.

>
> > Changes in v6:
> >   - Reorganized the patches to group changes by record type, not
> >     kernel subsytem, as suggested in earlier discussions
> >   - Added checks to ignore no-change events (new value == old value)
> >   - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
> >     syscalls such as settimeofday(2), stime(2), clock_settime(2)
> >   - Created an RFE page on audit-kernel GitHub
> > TODO:
> >   - tests for audit-testsuite
> >
> > v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
> > Changes in v5:
> >   - Dropped logging of some less important changes and update commit messages
> >   - No longer mark the patchset as RFC
> >
> > v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
> > Changes in v4:
> >   - Squashed first two patches into one
> >   - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
> >     conventions
> >   - Minor commit message editing
> >   - Cc timekeeping/NTP people for feedback
> >
> > v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
> > Changes in v3:
> >   - Switched to separate records for each variable
> >   - Both old and new value is now reported for each change
> >   - Injecting offset is reported via a separate record (since this
> >     offset consists of two values and is added directly to the clock,
> >     i.e. it doesn't make sense to log old and new value)
> >   - Added example records produced by chronyd -q (see the commit message
> >     of the last patch)
> >
> > v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
> > Changes in v2:
> >   - The audit_adjtime() function has been modified to only log those
> >     fields that contain values that are actually used, resulting in more
> >     compact records.
> >   - The audit_adjtime() call has been moved to do_adjtimex() in
> >     timekeeping.c
> >   - Added an additional patch (for review) that simplifies the detection
> >     if the syscall is read-only.
> >
> > v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
> >
> > [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
> >     table 4
> >
> > Ondrej Mosnacek (2):
> >   timekeeping: Audit clock adjustments
> >   ntp: Audit NTP parameters adjustment
> >
> >  include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c           | 15 +++++++++++++++
> >  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
> >  kernel/time/timekeeping.c  |  6 ++++++
> >  5 files changed, 82 insertions(+), 8 deletions(-)
> >
> > --
> > 2.20.1
> >
>
> - RGB
>
> --
> Richard Guy Briggs <rgb@redhat.com>
> Sr. S/W Engineer, Kernel Security, Base Operating Systems
> Remote, Ottawa, Red Hat Canada
> IRC: rgb, SunRaycer
> Voice: +1.647.777.2635, Internal: (81) 32635

--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Paul Moore]

On Thu, Mar 7, 2019 at 7:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> This patchset implements auditing of (syscall-triggered) changes that
> can modify or indirectly affect the system clock. Some of these
> changes can already be detected by simply logging relevant syscalls,
> but this has some disadvantages:
>   a) It is usually not possible to find out from the syscall records
>      the amount by which the time was shifted.
>   b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
>      for read-only operations, which might flood the audit log with
>      false positives. (Note that these patches don't solve this
>      problem yet due to the limitations of current record filtering
>      capabilities.)
>
> The main motivation is to provide better reliability of timestamps
> on the system as mandated by the FPT_STM.1 security functional
> requirement from Common Criteria. This requirement apparently demands
> that it is possible to reconstruct from audit trail the old and new
> values of the time when it is adjusted (see [1]).
>
> The current version of the patchset logs the following changes:
>   - direct setting of system time to a given value
>   - direct injection of timekeeping offset
>   - adjustment of timekeeping's TAI offset
>   - NTP value adjustments:
>     - time_offset
>     - time_freq
>     - time_status
>     - time_adjust
>     - tick_usec
>
> Changes to the following NTP values are not logged, as they are not
> important for security:
>   - time_maxerror
>   - time_esterror
>   - time_constant
>
> Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
> Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock
>
> Testing: Passed audit-testuite; functional tests TBD
>
> Changes in v6:
>   - Reorganized the patches to group changes by record type, not
>     kernel subsytem, as suggested in earlier discussions
>   - Added checks to ignore no-change events (new value == old value)
>   - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
>     syscalls such as settimeofday(2), stime(2), clock_settime(2)
>   - Created an RFE page on audit-kernel GitHub
> TODO:
>   - tests for audit-testsuite
>
> v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
> Changes in v5:
>   - Dropped logging of some less important changes and update commit messages
>   - No longer mark the patchset as RFC
>
> v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
> Changes in v4:
>   - Squashed first two patches into one
>   - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
>     conventions
>   - Minor commit message editing
>   - Cc timekeeping/NTP people for feedback
>
> v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
> Changes in v3:
>   - Switched to separate records for each variable
>   - Both old and new value is now reported for each change
>   - Injecting offset is reported via a separate record (since this
>     offset consists of two values and is added directly to the clock,
>     i.e. it doesn't make sense to log old and new value)
>   - Added example records produced by chronyd -q (see the commit message
>     of the last patch)
>
> v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
> Changes in v2:
>   - The audit_adjtime() function has been modified to only log those
>     fields that contain values that are actually used, resulting in more
>     compact records.
>   - The audit_adjtime() call has been moved to do_adjtimex() in
>     timekeeping.c
>   - Added an additional patch (for review) that simplifies the detection
>     if the syscall is read-only.
>
> v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
>
> [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
>     table 4
>
> Ondrej Mosnacek (2):
>   timekeeping: Audit clock adjustments
>   ntp: Audit NTP parameters adjustment
>
>  include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
>  include/uapi/linux/audit.h |  2 ++
>  kernel/auditsc.c           | 15 +++++++++++++++
>  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
>  kernel/time/timekeeping.c  |  6 ++++++
>  5 files changed, 82 insertions(+), 8 deletions(-)

These patches look fine to me, but it would be really nice to get an
ACK from the time folks before I merge this into audit/next.  Time
folks, I know you've looked at previous versions of this patchset, can
you give this a quick look to make sure everything is still okay from
your perspective?

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Paul Moore]

On Mon, Mar 25, 2019 at 10:50 AM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Mar 7, 2019 at 7:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > This patchset implements auditing of (syscall-triggered) changes that
> > can modify or indirectly affect the system clock. Some of these
> > changes can already be detected by simply logging relevant syscalls,
> > but this has some disadvantages:
> >   a) It is usually not possible to find out from the syscall records
> >      the amount by which the time was shifted.
> >   b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
> >      for read-only operations, which might flood the audit log with
> >      false positives. (Note that these patches don't solve this
> >      problem yet due to the limitations of current record filtering
> >      capabilities.)
> >
> > The main motivation is to provide better reliability of timestamps
> > on the system as mandated by the FPT_STM.1 security functional
> > requirement from Common Criteria. This requirement apparently demands
> > that it is possible to reconstruct from audit trail the old and new
> > values of the time when it is adjusted (see [1]).
> >
> > The current version of the patchset logs the following changes:
> >   - direct setting of system time to a given value
> >   - direct injection of timekeeping offset
> >   - adjustment of timekeeping's TAI offset
> >   - NTP value adjustments:
> >     - time_offset
> >     - time_freq
> >     - time_status
> >     - time_adjust
> >     - tick_usec
> >
> > Changes to the following NTP values are not logged, as they are not
> > important for security:
> >   - time_maxerror
> >   - time_esterror
> >   - time_constant
> >
> > Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
> > Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock
> >
> > Testing: Passed audit-testuite; functional tests TBD
> >
> > Changes in v6:
> >   - Reorganized the patches to group changes by record type, not
> >     kernel subsytem, as suggested in earlier discussions
> >   - Added checks to ignore no-change events (new value == old value)
> >   - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
> >     syscalls such as settimeofday(2), stime(2), clock_settime(2)
> >   - Created an RFE page on audit-kernel GitHub
> > TODO:
> >   - tests for audit-testsuite
> >
> > v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
> > Changes in v5:
> >   - Dropped logging of some less important changes and update commit messages
> >   - No longer mark the patchset as RFC
> >
> > v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
> > Changes in v4:
> >   - Squashed first two patches into one
> >   - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
> >     conventions
> >   - Minor commit message editing
> >   - Cc timekeeping/NTP people for feedback
> >
> > v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
> > Changes in v3:
> >   - Switched to separate records for each variable
> >   - Both old and new value is now reported for each change
> >   - Injecting offset is reported via a separate record (since this
> >     offset consists of two values and is added directly to the clock,
> >     i.e. it doesn't make sense to log old and new value)
> >   - Added example records produced by chronyd -q (see the commit message
> >     of the last patch)
> >
> > v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
> > Changes in v2:
> >   - The audit_adjtime() function has been modified to only log those
> >     fields that contain values that are actually used, resulting in more
> >     compact records.
> >   - The audit_adjtime() call has been moved to do_adjtimex() in
> >     timekeeping.c
> >   - Added an additional patch (for review) that simplifies the detection
> >     if the syscall is read-only.
> >
> > v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
> >
> > [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
> >     table 4
> >
> > Ondrej Mosnacek (2):
> >   timekeeping: Audit clock adjustments
> >   ntp: Audit NTP parameters adjustment
> >
> >  include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
> >  include/uapi/linux/audit.h |  2 ++
> >  kernel/auditsc.c           | 15 +++++++++++++++
> >  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
> >  kernel/time/timekeeping.c  |  6 ++++++
> >  5 files changed, 82 insertions(+), 8 deletions(-)
>
> These patches look fine to me, but it would be really nice to get an
> ACK from the time folks before I merge this into audit/next.  Time
> folks, I know you've looked at previous versions of this patchset, can
> you give this a quick look to make sure everything is still okay from
> your perspective?

Ondrej, please don't let the lack of response from the time folks keep
you from working on the tests.  If you can get the tests ready in
time, I see no reason why this couldn't get merged before the next
merge window.

-- 
paul moore
www.paul-moore.com

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[John Stultz]

On Thu, Mar 7, 2019 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Emit an audit record whenever the system clock is changed (i.e. shifted
> by a non-zero offset) by a syscall from userspace. The syscalls than can
> (at the time of writing) trigger such record are:
>   - settimeofday(2), stime(2), clock_settime(2) -- via
>     do_settimeofday64()
>   - adjtimex(2), clock_adjtime(2) -- via do_adjtimex()
>
> The new records have type AUDIT_TIME_INJOFFSET and contain the following
> fields:
>   - sec -- the 'seconds' part of the offset
>   - nsec -- the 'nanoseconds' part of the offset
>
> For reference, running the following commands:
>
>     auditctl -D
>     auditctl -a exit,always -F arch=b64 -S adjtimex
>     chronyd -q
>
> triggers (among others) a syscall that produces audit records like this:
>
> type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
> type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71 cd /home/omosnace/Dokumenty/Kernel/worktrees/audit/src/kernel/time
> s

Hrm.  Ugly log spam aside (I get it sort of goes with the territory
:), I could imagine someone looking at this and wanting to also know
when the injection was applied. Obviously the whole point of the
offset injection is we don't really care about the when, we only want
a fixed offset to be made atomically.  That said, if someone did try
to add such a timestamp on the log, there's the problem of trying to
calculate the time while holding the timekeeping locks.   So, are you
certain the next request won't be to try to to also calculate a
timestamp and push it into the audit_log() call as well?

Also, we have to be careful with anything we call from the timekeeping
code, its really easy for some corner case to trip something that then
tries to read the time and we deadlock, particularly with rare cases
like time adjustments.  I'm not familiar with the audit subsystem, but
from a maintenance point of view, can we make sure there's enough
documentation so that audit_log() and everything it calls will never
in the future call back into the timekeeping code?   I suspect this is
already covered, so apologies for the boilerplate concern, but I want
to be sure.


> The above records have been produced by the following syscall from
> chronyd (as per strace output):
>
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
>
> (The struct timex fields above are from *after* the syscall was
> executed, so they contain the current (new) values as set from the
> kernel, except of the 'modes' field, which contains the original value
> sent by the caller.)
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h      | 15 +++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/auditsc.c           |  8 ++++++++
>  kernel/time/timekeeping.c  |  6 ++++++
>  4 files changed, 30 insertions(+)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..43a60fbe74be 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -27,6 +27,7 @@
>  #include <linux/ptrace.h>
>  #include <linux/namei.h>  /* LOOKUP_* */
>  #include <uapi/linux/audit.h>
> +#include <uapi/linux/timex.h>
>
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -365,6 +366,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
>  extern void __audit_mmap_fd(int fd, int flags);
>  extern void __audit_log_kern_module(char *name);
>  extern void __audit_fanotify(unsigned int response);
> +extern void __audit_tk_injoffset(struct timespec64 offset);
>
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -467,6 +469,16 @@ static inline void audit_fanotify(unsigned int response)
>                 __audit_fanotify(response);
>  }
>
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{
> +       /* ignore no-op events */
> +       if (offset.tv_sec == 0 && offset.tv_nsec == 0)
> +               return;
> +
> +       if (!audit_dummy_context())
> +               __audit_tk_injoffset(offset);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -580,6 +592,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
>
> +static inline void audit_tk_injoffset(struct timespec64 offset)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 36a7e3f18e69..2167d55bc800 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -114,6 +114,7 @@
>  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
>  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
>  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
> +#define AUDIT_TIME_INJOFFSET   1332    /* Timekeeping offset injected */
>
>  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
>  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index d1eab1d4a930..781336d0f2de 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
>                 AUDIT_FANOTIFY, "resp=%u", response);
>  }
>
> +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> + * called while holding the timekeeping lock: */
> +void __audit_tk_injoffset(struct timespec64 offset)
> +{
> +       audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> +                 "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>         kuid_t auid, uid;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index ac5dbf2cd4a2..0f0b566afe61 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -21,6 +21,7 @@
>  #include <linux/stop_machine.h>
>  #include <linux/pvclock_gtod.h>
>  #include <linux/compiler.h>
> +#include <linux/audit.h>
>
>  #include "tick-internal.h"
>  #include "ntp_internal.h"
> @@ -1250,6 +1251,9 @@ out:
>         /* signal hrtimers about time change */
>         clock_was_set();
>
> +       if (!ret)
> +               audit_tk_injoffset(ts_delta);
> +
>         return ret;
>  }
>  EXPORT_SYMBOL(do_settimeofday64);
> @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
>                 ret = timekeeping_inject_offset(&delta);
>                 if (ret)
>                         return ret;
> +
> +               audit_tk_injoffset(delta);
>         }
>
>         ktime_get_real_ts64(&ts);

Ignoring my worry above, from a quick read over, the code here looks
ok to me. Though I've not actually applied and tinkered with it.

Acked-by: John Stultz <john.stultz@linaro.org>

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[John Stultz]

On Thu, Mar 7, 2019 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Emit an audit record every time selected NTP parameters are modified
> from userspace (via adjtimex(2) or clock_adjtime(2)).
>
> Such events will now generate records of type AUDIT_TIME_ADJNTPVAL
> containing the following fields:
>   - op -- which value was adjusted:
>     - offset -- corresponding to the time_offset variable
>     - freq   -- corresponding to the time_freq variable
>     - status -- corresponding to the time_status variable
>     - adjust -- corresponding to the time_adjust variable
>     - tick   -- corresponding to the tick_usec variable
>     - tai    -- corresponding to the timekeeping's TAI offset
>   - old -- the old value
>   - new -- the new value
>
> For reference, running the following commands:
>
>     auditctl -D
>     auditctl -a exit,always -F arch=b64 -S adjtimex
>     chronyd -q
>
> produces audit records like this:
>
> type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:5): proctitle=6368726F6E7964002D71
> type=SYSCALL msg=audit(1530616044.507:6): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:6): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
> type=SYSCALL msg=audit(1530616044.507:7): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=1 a2=1 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:7): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:8): op=status old=8256 new=8257
> type=SYSCALL msg=audit(1530616044.507:8): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:8): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:9): op=status old=8257 new=64
> type=SYSCALL msg=audit(1530616044.507:9): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ab0 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:9): proctitle=6368726F6E7964002D71
> type=SYSCALL msg=audit(1530616044.507:10): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78a70 a1=0 a2=55e129c850c0 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.507:10): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000
> type=SYSCALL msg=audit(1530616044.511:11): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78ad0 a1=0 a2=2710 a3=f42f82a800000 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.511:11): proctitle=6368726F6E7964002D71
> type=SYSCALL msg=audit(1530616044.521:12): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78b40 a1=1 a2=40 a3=f91f6ef84fbab items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616044.521:12): proctitle=6368726F6E7964002D71
> type=TIME_ADJNTPVAL msg=audit(1530616049.652:13): op=status old=64 new=8256
> type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71
> type=SYSCALL msg=audit(1530616033.783:14): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78bc0 a1=0 a2=2710 a3=0 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> type=PROCTITLE msg=audit(1530616033.783:14): proctitle=6368726F6E7964002D71
>
> The chronyd command that produced the above records executed the
> following adjtimex(2) syscalls (as per strace output):
>
> adjtimex({modes=ADJ_OFFSET|0x8000, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507215}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_MAXERROR, offset=0, freq=0, maxerror=0, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507438}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507604737}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_OFFSET|ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_PLL|STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507698330}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_STATUS, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=507792}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=0, offset=0, freq=0, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=508000}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=512146}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_MAXERROR|ADJ_ESTERROR|ADJ_STATUS, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616044, tv_usec=522506}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> adjtimex({modes=ADJ_FREQUENCY|ADJ_TICK, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=784644657}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
>
> (The struct timex fields above are from *after* the syscall was
> executed, so they contain the current (new) values as set from the
> kernel, except of the 'modes' field, which contains the original value
> sent by the caller.)
>
> The changes to the time_maxerror, time_esterror, and time_constant
> variables are not logged, as these are not important for security. Also,
> no-op adjustments that do not actually change the value are not logged.
>
> An overview of parameter changes that can be done via do_adjtimex()
> (based on information from Miroslav Lichvar) and whether they are
> audited:
>   __timekeeping_set_tai_offset() -- sets the offset from the
>                                     International Atomic Time
>                                     (AUDITED)
>   NTP variables:
>     time_offset -- can adjust the clock by up to 0.5 seconds per call
>                    and also speed it up or slow down by up to about
>                    0.05% (43 seconds per day) (AUDITED)
>     time_freq -- can speed up or slow down by up to about 0.05%
>     time_status -- can insert/delete leap seconds and it also enables/
>                    disables synchronization of the hardware real-time
>                    clock (AUDITED)
>     time_maxerror, time_esterror -- change error estimates used to
>                                     inform userspace applications
>                                     (NOT AUDITED)
>     time_constant -- controls the speed of the clock adjustments that
>                      are made when time_offset is set (NOT AUDITED)
>     time_adjust -- can temporarily speed up or slow down the clock by up
>                    to 0.05% (AUDITED)
>     tick_usec -- a more extreme version of time_freq; can speed up or
>                  slow down the clock by up to 10% (AUDITED)
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

Again, lightly looked over, and I don't see anything to object to.

Acked-by: John Stultz <john.stultz@linaro.org>

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[John Stultz]

On Mon, Mar 25, 2019 at 7:50 AM Paul Moore <paul@paul-moore.com> wrote:
> These patches look fine to me, but it would be really nice to get an
> ACK from the time folks before I merge this into audit/next.  Time
> folks, I know you've looked at previous versions of this patchset, can
> you give this a quick look to make sure everything is still okay from
> your perspective?
>

Sorry for the slow response.  I briefly looked it over and it seems
simple enough, so don't really have any objections. Though I don't
have as sharp an eye (or opinion :) as Thomas, so no promises he won't
step in with an issue.

I do want to make sure folks are careful with future changes to
audit_log(), but that's my usual hand-wringing.

thanks
-john

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Thomas Gleixner]

On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
>  
> +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> + * called while holding the timekeeping lock: */

Audit is no justification for doing ATOMIC allocations just because it's
convenient in the middle of code which blocks every concurrent reader. 

Please find a place outside of the timekeeper lock to do that audit
logging. Either that or allocate your buffer upfront in a preemptible
section and commit after the critical section.

/*
 * Aside of that please use proper multiline comment style and not this
 * horrible other one.
 */

> +void __audit_tk_injoffset(struct timespec64 offset)
> +{
> +	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> +		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> +}
> +
> @@ -1250,6 +1251,9 @@ out:
>  	/* signal hrtimers about time change */
>  	clock_was_set();
>  
> +	if (!ret)
> +		audit_tk_injoffset(ts_delta);

This one does not need GFP_ATOMIC at all.

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(do_settimeofday64);
> @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
>  		ret = timekeeping_inject_offset(&delta);
>  		if (ret)
>  			return ret;
> +
> +		audit_tk_injoffset(delta);
>  	}
>  
>  	ktime_get_real_ts64(&ts);

This can be done at the end of do_adjtimex() quite nicely in preemptible
context.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Thomas Gleixner]

On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:

> Emit an audit record every time selected NTP parameters are modified
> from userspace (via adjtimex(2) or clock_adjtime(2)).
> 
> Such events will now generate records of type AUDIT_TIME_ADJNTPVAL
> containing the following fields:
>   - op -- which value was adjusted:
>     - offset -- corresponding to the time_offset variable
>     - freq   -- corresponding to the time_freq variable
>     - status -- corresponding to the time_status variable
>     - adjust -- corresponding to the time_adjust variable
>     - tick   -- corresponding to the tick_usec variable
>     - tai    -- corresponding to the timekeeping's TAI offset
>   - old -- the old value
>   - new -- the new value
> 
> For reference, running the following commands:
> 
>     auditctl -D
>     auditctl -a exit,always -F arch=b64 -S adjtimex
>     chronyd -q
> 
> produces audit records like this:
> 
> type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)

<SNIP gazillions of lines of unparseable garbage>

Is it really necessary to put this into the changelog?

>  
> +void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
> +{
> +	audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,

No.

> +		  "op=%s old=%lli new=%lli", type,
> +		  (long long)oldval, (long long)newval);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;
> diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> index 36a2bef00125..5f456a84151a 100644
> --- a/kernel/time/ntp.c
> +++ b/kernel/time/ntp.c
> @@ -17,6 +17,7 @@
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/rtc.h>
> +#include <linux/audit.h>
>  
>  #include "ntp_internal.h"
>  #include "timekeeping_internal.h"
> @@ -293,6 +294,8 @@ static inline s64 ntp_update_offset_fll(s64 offset64, long secs)
>  
>  static void ntp_update_offset(long offset)
>  {
> +	s64 old_offset = time_offset;
> +	s64 old_freq = time_freq;
>  	s64 freq_adj;
>  	s64 offset64;
>  	long secs;
> @@ -341,6 +344,9 @@ static void ntp_update_offset(long offset)
>  	time_freq   = max(freq_adj, -MAXFREQ_SCALED);
>  
>  	time_offset = div_s64(offset64 << NTP_SCALE_SHIFT, NTP_INTERVAL_FREQ);
> +
> +	audit_ntp_adjust("offset", old_offset, time_offset);
> +	audit_ntp_adjust("freq", old_freq, time_freq);
>  }
>  
>  /**
> @@ -658,21 +664,31 @@ static inline void process_adj_status(const struct timex *txc)
>  
>  static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai)
>  {
> -	if (txc->modes & ADJ_STATUS)
> -		process_adj_status(txc);
> +	if (txc->modes & (ADJ_STATUS | ADJ_NANO | ADJ_MICRO)) {
> +		int old_status = time_status;
> +
> +		if (txc->modes & ADJ_STATUS)
> +			process_adj_status(txc);
> -	if (txc->modes & ADJ_NANO)
> -		time_status |= STA_NANO;
> +		if (txc->modes & ADJ_NANO)
> +			time_status |= STA_NANO;
>  
> -	if (txc->modes & ADJ_MICRO)
> -		time_status &= ~STA_NANO;
> +		if (txc->modes & ADJ_MICRO)
> +			time_status &= ~STA_NANO;
> +
> +		audit_ntp_adjust("status", old_status, time_status);
> +	}
>  
>  	if (txc->modes & ADJ_FREQUENCY) {
> +		s64 old_freq = time_freq;
> +
>  		time_freq = txc->freq * PPM_SCALE;
>  		time_freq = min(time_freq, MAXFREQ_SCALED);
>  		time_freq = max(time_freq, -MAXFREQ_SCALED);
>  		/* update pps_freq */
>  		pps_set_freq(time_freq);
> +
> +		audit_ntp_adjust("freq", old_freq, time_freq);
>  	}
>  
>  	if (txc->modes & ADJ_MAXERROR)
> @@ -689,14 +705,18 @@ static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai
>  		time_constant = max(time_constant, 0l);
>  	}
>  
> -	if (txc->modes & ADJ_TAI && txc->constant > 0)
> +	if (txc->modes & ADJ_TAI && txc->constant > 0) {
> +		audit_ntp_adjust("tai", *time_tai, txc->constant);
>  		*time_tai = txc->constant;
> +	}
>  
>  	if (txc->modes & ADJ_OFFSET)
>  		ntp_update_offset(txc->offset);
>  
> -	if (txc->modes & ADJ_TICK)
> +	if (txc->modes & ADJ_TICK) {
> +		audit_ntp_adjust("tick", tick_usec, txc->tick);
>  		tick_usec = txc->tick;
> +	}
>  
>  	if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
>  		ntp_update_frequency();
> @@ -718,6 +738,8 @@ int __do_adjtimex(struct timex *txc, const struct timespec64 *ts, s32 *time_tai)
>  			/* adjtime() is independent from ntp_adjtime() */
>  			time_adjust = txc->offset;
>  			ntp_update_frequency();
> +
> +			audit_ntp_adjust("adjust", save_adjust, txc->offset);
>  		}
>  		txc->offset = save_adjust;
>  	} else {

Not going to happen. We are not reshuffling all that code just to
accomodate random audit log invocations in a critical section plus having a
gazillion of GFP_ATOMIC allocation in the critical section just because.

The whole information can be reconstructed after the fact:

   1) Copy the user space supplied struct timex to a buffer

   2) Retrieve the current timex information _before_ invoking
      do_adjtimex().

   3) Look at the ret value and the resulting struct timex which is going
      to be copied back to user space and figure out with the help of #1
      and #2 what you need to log.

That does not even need a single line of change in the NTP code and almost
everything happens in fully preemptible context.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Thomas Gleixner]

On Wed, 27 Mar 2019, John Stultz wrote:
> On Mon, Mar 25, 2019 at 7:50 AM Paul Moore <paul@paul-moore.com> wrote:
> > These patches look fine to me, but it would be really nice to get an
> > ACK from the time folks before I merge this into audit/next.  Time
> > folks, I know you've looked at previous versions of this patchset, can
> > you give this a quick look to make sure everything is still okay from
> > your perspective?
> >
> 
> Sorry for the slow response.  I briefly looked it over and it seems
> simple enough, so don't really have any objections. Though I don't
> have as sharp an eye (or opinion :) as Thomas, so no promises he won't
> step in with an issue.

I did already :)

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Thomas Gleixner]

On Thu, 28 Mar 2019, Thomas Gleixner wrote:
> On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
> >  		AUDIT_FANOTIFY,	"resp=%u", response);
> >  }
> >  
> > +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> > + * called while holding the timekeeping lock: */
> 
> Audit is no justification for doing ATOMIC allocations just because it's
> convenient in the middle of code which blocks every concurrent reader. 

Aside of that you might talk to your colleagues working on Preempt-RT about
this. I'm pretty sure they are going to have opinions simply because you
cannot do ATOMIC allocations in those contexts on RT.

Logging needs to be unintrusive and allocations are definitely not.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Thomas Gleixner]

Folks,

either lift the subscriber only limitation for your audit mailing list or
simply not Cc it when posting on LKML.

These bounce messages are annoying and if the moderator sleeps then the
people on that list who might be interested but not Cc'ed and not
subscribed to LKML (which I can understand) are not going to get the info
until someone moderates it.

Please stop that nonsense.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Ondrej Mosnacek]

On Thu, Mar 28, 2019 at 1:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
>
> > Emit an audit record every time selected NTP parameters are modified
> > from userspace (via adjtimex(2) or clock_adjtime(2)).
> >
> > Such events will now generate records of type AUDIT_TIME_ADJNTPVAL
> > containing the following fields:
> >   - op -- which value was adjusted:
> >     - offset -- corresponding to the time_offset variable
> >     - freq   -- corresponding to the time_freq variable
> >     - status -- corresponding to the time_status variable
> >     - adjust -- corresponding to the time_adjust variable
> >     - tick   -- corresponding to the tick_usec variable
> >     - tai    -- corresponding to the timekeeping's TAI offset
> >   - old -- the old value
> >   - new -- the new value
> >
> > For reference, running the following commands:
> >
> >     auditctl -D
> >     auditctl -a exit,always -F arch=b64 -S adjtimex
> >     chronyd -q
> >
> > produces audit records like this:
> >
> > type=SYSCALL msg=audit(1530616044.507:5): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78c00 a1=0 a2=4 a3=7f754ae28c0a items=0 ppid=626 pid=629 auid=0 uid=0 gid=0 euid=0 suid=0 fsuid=0 egid=0 sgid=0 fsgid=0 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
>
> <SNIP gazillions of lines of unparseable garbage>
>
> Is it really necessary to put this into the changelog?

Yeah, sorry, I went a bit overboard with the record examples... I'll
try to provide simpler and less verbose examples in the next version.

>
> >
> > +void __audit_ntp_adjust(const char *type, s64 oldval, s64 newval)
> > +{
> > +     audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_ADJNTPVAL,
>
> No.
>
> > +               "op=%s old=%lli new=%lli", type,
> > +               (long long)oldval, (long long)newval);
> > +}
> > +
> >  static void audit_log_task(struct audit_buffer *ab)
> >  {
> >       kuid_t auid, uid;
> > diff --git a/kernel/time/ntp.c b/kernel/time/ntp.c
> > index 36a2bef00125..5f456a84151a 100644
> > --- a/kernel/time/ntp.c
> > +++ b/kernel/time/ntp.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/mm.h>
> >  #include <linux/module.h>
> >  #include <linux/rtc.h>
> > +#include <linux/audit.h>
> >
> >  #include "ntp_internal.h"
> >  #include "timekeeping_internal.h"
> > @@ -293,6 +294,8 @@ static inline s64 ntp_update_offset_fll(s64 offset64, long secs)
> >
> >  static void ntp_update_offset(long offset)
> >  {
> > +     s64 old_offset = time_offset;
> > +     s64 old_freq = time_freq;
> >       s64 freq_adj;
> >       s64 offset64;
> >       long secs;
> > @@ -341,6 +344,9 @@ static void ntp_update_offset(long offset)
> >       time_freq   = max(freq_adj, -MAXFREQ_SCALED);
> >
> >       time_offset = div_s64(offset64 << NTP_SCALE_SHIFT, NTP_INTERVAL_FREQ);
> > +
> > +     audit_ntp_adjust("offset", old_offset, time_offset);
> > +     audit_ntp_adjust("freq", old_freq, time_freq);
> >  }
> >
> >  /**
> > @@ -658,21 +664,31 @@ static inline void process_adj_status(const struct timex *txc)
> >
> >  static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai)
> >  {
> > -     if (txc->modes & ADJ_STATUS)
> > -             process_adj_status(txc);
> > +     if (txc->modes & (ADJ_STATUS | ADJ_NANO | ADJ_MICRO)) {
> > +             int old_status = time_status;
> > +
> > +             if (txc->modes & ADJ_STATUS)
> > +                     process_adj_status(txc);
> > -     if (txc->modes & ADJ_NANO)
> > -             time_status |= STA_NANO;
> > +             if (txc->modes & ADJ_NANO)
> > +                     time_status |= STA_NANO;
> >
> > -     if (txc->modes & ADJ_MICRO)
> > -             time_status &= ~STA_NANO;
> > +             if (txc->modes & ADJ_MICRO)
> > +                     time_status &= ~STA_NANO;
> > +
> > +             audit_ntp_adjust("status", old_status, time_status);
> > +     }
> >
> >       if (txc->modes & ADJ_FREQUENCY) {
> > +             s64 old_freq = time_freq;
> > +
> >               time_freq = txc->freq * PPM_SCALE;
> >               time_freq = min(time_freq, MAXFREQ_SCALED);
> >               time_freq = max(time_freq, -MAXFREQ_SCALED);
> >               /* update pps_freq */
> >               pps_set_freq(time_freq);
> > +
> > +             audit_ntp_adjust("freq", old_freq, time_freq);
> >       }
> >
> >       if (txc->modes & ADJ_MAXERROR)
> > @@ -689,14 +705,18 @@ static inline void process_adjtimex_modes(const struct timex *txc, s32 *time_tai
> >               time_constant = max(time_constant, 0l);
> >       }
> >
> > -     if (txc->modes & ADJ_TAI && txc->constant > 0)
> > +     if (txc->modes & ADJ_TAI && txc->constant > 0) {
> > +             audit_ntp_adjust("tai", *time_tai, txc->constant);
> >               *time_tai = txc->constant;
> > +     }
> >
> >       if (txc->modes & ADJ_OFFSET)
> >               ntp_update_offset(txc->offset);
> >
> > -     if (txc->modes & ADJ_TICK)
> > +     if (txc->modes & ADJ_TICK) {
> > +             audit_ntp_adjust("tick", tick_usec, txc->tick);
> >               tick_usec = txc->tick;
> > +     }
> >
> >       if (txc->modes & (ADJ_TICK|ADJ_FREQUENCY|ADJ_OFFSET))
> >               ntp_update_frequency();
> > @@ -718,6 +738,8 @@ int __do_adjtimex(struct timex *txc, const struct timespec64 *ts, s32 *time_tai)
> >                       /* adjtime() is independent from ntp_adjtime() */
> >                       time_adjust = txc->offset;
> >                       ntp_update_frequency();
> > +
> > +                     audit_ntp_adjust("adjust", save_adjust, txc->offset);
> >               }
> >               txc->offset = save_adjust;
> >       } else {
>
> Not going to happen. We are not reshuffling all that code just to
> accomodate random audit log invocations in a critical section plus having a
> gazillion of GFP_ATOMIC allocation in the critical section just because.

OK, seems I underestimated the consequences of putting the logging
calls directly in there. While I was offline over the weekend I
already came up with a cleaner version that collects the changes in a
structure and does the logging outside of the critical section. I
currently does a few unnecessary writes into memory under
CONFIG_AUDIT=n, but if that is an issue I can boost the abstraction or
just add some #ifdefs to avoid that.

>
> The whole information can be reconstructed after the fact:
>
>    1) Copy the user space supplied struct timex to a buffer
>
>    2) Retrieve the current timex information _before_ invoking
>       do_adjtimex().
>
>    3) Look at the ret value and the resulting struct timex which is going
>       to be copied back to user space and figure out with the help of #1
>       and #2 what you need to log.
>
> That does not even need a single line of change in the NTP code and almost
> everything happens in fully preemptible context.

I worry that extracting everything from the timex structures might get
a little too complicated... Hopefully you'll be OK with the solution I
am preparing for v8 - it still adds a bit of code to ntp.c, but it's
much less intrusive.

Thanks a lot for the review!


--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Ondrej Mosnacek]

On Thu, Mar 28, 2019 at 12:27 AM John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Mar 7, 2019 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Emit an audit record whenever the system clock is changed (i.e. shifted
> > by a non-zero offset) by a syscall from userspace. The syscalls than can
> > (at the time of writing) trigger such record are:
> >   - settimeofday(2), stime(2), clock_settime(2) -- via
> >     do_settimeofday64()
> >   - adjtimex(2), clock_adjtime(2) -- via do_adjtimex()
> >
> > The new records have type AUDIT_TIME_INJOFFSET and contain the following
> > fields:
> >   - sec -- the 'seconds' part of the offset
> >   - nsec -- the 'nanoseconds' part of the offset
> >
> > For reference, running the following commands:
> >
> >     auditctl -D
> >     auditctl -a exit,always -F arch=b64 -S adjtimex
> >     chronyd -q
> >
> > triggers (among others) a syscall that produces audit records like this:
> >
> > type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
> > type=SYSCALL msg=audit(1530616049.652:13): arch=c000003e syscall=159 success=yes exit=5 a0=7fff57e78270 a1=1 a2=fffffffffffffff0 a3=137b828205ca12 items=0 ppid=626 pid=629 auid=0 uid=385 gid=382 euid=385 suid=385 fsuid=385 egid=382 sgid=382 fsgid=382 tty=(none) ses=1 comm="chronyd" exe="/usr/sbin/chronyd" subj=system_u:system_r:kernel_t:s0 key=(null)
> > type=PROCTITLE msg=audit(1530616049.652:13): proctitle=6368726F6E7964002D71 cd /home/omosnace/Dokumenty/Kernel/worktrees/audit/src/kernel/time
> > s
>
> Hrm.  Ugly log spam aside (I get it sort of goes with the territory
> :), I could imagine someone looking at this and wanting to also know
> when the injection was applied. Obviously the whole point of the
> offset injection is we don't really care about the when, we only want
> a fixed offset to be made atomically.  That said, if someone did try
> to add such a timestamp on the log, there's the problem of trying to
> calculate the time while holding the timekeeping locks.   So, are you
> certain the next request won't be to try to to also calculate a
> timestamp and push it into the audit_log() call as well?

Well, __audit_syscall_entry() already logs the timestamp of the
syscall entry (this is what ends up also in the TIME_INJOFFSET
syscall-associated record as the timestamp, actually), so even though
it is not precisely the moment when the change happened, it should be
good enough in most cases. I'm not sure if it is worth it to add
another slightly duplicit but exact timestamp... Steve Grubb is our
local expert on certifications, so unless he tells me this is likely
required, I don't feel like adding that extra information there right
now...

>
> Also, we have to be careful with anything we call from the timekeeping
> code, its really easy for some corner case to trip something that then
> tries to read the time and we deadlock, particularly with rare cases
> like time adjustments.  I'm not familiar with the audit subsystem, but
> from a maintenance point of view, can we make sure there's enough
> documentation so that audit_log() and everything it calls will never
> in the future call back into the timekeeping code?   I suspect this is
> already covered, so apologies for the boilerplate concern, but I want
> to be sure.

Yes, this turns out to be a bigger issue than I'd thought, see my
reply to Thomas in the thread for 2/2.



>
>
> > The above records have been produced by the following syscall from
> > chronyd (as per strace output):
> >
> > adjtimex({modes=ADJ_SETOFFSET|ADJ_NANO, offset=0, freq=750433, maxerror=16000000, esterror=16000000, status=STA_UNSYNC|STA_NANO, constant=2, precision=1, tolerance=32768000, time={tv_sec=1530616033, tv_usec=778717675}, tick=10000, ppsfreq=0, jitter=0, shift=0, stabil=0, jitcnt=0, calcnt=0, errcnt=0, stbcnt=0, tai=0}) = 5 (TIME_ERROR)
> >
> > (The struct timex fields above are from *after* the syscall was
> > executed, so they contain the current (new) values as set from the
> > kernel, except of the 'modes' field, which contains the original value
> > sent by the caller.)
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/audit.h      | 15 +++++++++++++++
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/auditsc.c           |  8 ++++++++
> >  kernel/time/timekeeping.c  |  6 ++++++
> >  4 files changed, 30 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 1e69d9fe16da..43a60fbe74be 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -27,6 +27,7 @@
> >  #include <linux/ptrace.h>
> >  #include <linux/namei.h>  /* LOOKUP_* */
> >  #include <uapi/linux/audit.h>
> > +#include <uapi/linux/timex.h>
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -365,6 +366,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old);
> >  extern void __audit_mmap_fd(int fd, int flags);
> >  extern void __audit_log_kern_module(char *name);
> >  extern void __audit_fanotify(unsigned int response);
> > +extern void __audit_tk_injoffset(struct timespec64 offset);
> >
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -467,6 +469,16 @@ static inline void audit_fanotify(unsigned int response)
> >                 __audit_fanotify(response);
> >  }
> >
> > +static inline void audit_tk_injoffset(struct timespec64 offset)
> > +{
> > +       /* ignore no-op events */
> > +       if (offset.tv_sec == 0 && offset.tv_nsec == 0)
> > +               return;
> > +
> > +       if (!audit_dummy_context())
> > +               __audit_tk_injoffset(offset);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -580,6 +592,9 @@ static inline void audit_log_kern_module(char *name)
> >  static inline void audit_fanotify(unsigned int response)
> >  { }
> >
> > +static inline void audit_tk_injoffset(struct timespec64 offset)
> > +{ }
> > +
> >  static inline void audit_ptrace(struct task_struct *t)
> >  { }
> >  #define audit_n_rules 0
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index 36a7e3f18e69..2167d55bc800 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -114,6 +114,7 @@
> >  #define AUDIT_REPLACE          1329    /* Replace auditd if this packet unanswerd */
> >  #define AUDIT_KERN_MODULE      1330    /* Kernel Module events */
> >  #define AUDIT_FANOTIFY         1331    /* Fanotify access decision */
> > +#define AUDIT_TIME_INJOFFSET   1332    /* Timekeeping offset injected */
> >
> >  #define AUDIT_AVC              1400    /* SE Linux avc denial or grant */
> >  #define AUDIT_SELINUX_ERR      1401    /* Internal SE Linux Errors */
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index d1eab1d4a930..781336d0f2de 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
> >                 AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> > + * called while holding the timekeeping lock: */
> > +void __audit_tk_injoffset(struct timespec64 offset)
> > +{
> > +       audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> > +                 "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> > +}
> > +
> >  static void audit_log_task(struct audit_buffer *ab)
> >  {
> >         kuid_t auid, uid;
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index ac5dbf2cd4a2..0f0b566afe61 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -21,6 +21,7 @@
> >  #include <linux/stop_machine.h>
> >  #include <linux/pvclock_gtod.h>
> >  #include <linux/compiler.h>
> > +#include <linux/audit.h>
> >
> >  #include "tick-internal.h"
> >  #include "ntp_internal.h"
> > @@ -1250,6 +1251,9 @@ out:
> >         /* signal hrtimers about time change */
> >         clock_was_set();
> >
> > +       if (!ret)
> > +               audit_tk_injoffset(ts_delta);
> > +
> >         return ret;
> >  }
> >  EXPORT_SYMBOL(do_settimeofday64);
> > @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
> >                 ret = timekeeping_inject_offset(&delta);
> >                 if (ret)
> >                         return ret;
> > +
> > +               audit_tk_injoffset(delta);
> >         }
> >
> >         ktime_get_real_ts64(&ts);
>
> Ignoring my worry above, from a quick read over, the code here looks
> ok to me. Though I've not actually applied and tinkered with it.
>
> Acked-by: John Stultz <john.stultz@linaro.org>

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Ondrej Mosnacek]

On Thu, Mar 28, 2019 at 1:09 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2512,6 +2512,14 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +/* We need to allocate with GFP_ATOMIC here, since these two functions will be
> > + * called while holding the timekeeping lock: */
>
> Audit is no justification for doing ATOMIC allocations just because it's
> convenient in the middle of code which blocks every concurrent reader.
>
> Please find a place outside of the timekeeper lock to do that audit
> logging. Either that or allocate your buffer upfront in a preemptible
> section and commit after the critical section.
>
> /*
>  * Aside of that please use proper multiline comment style and not this
>  * horrible other one.
>  */

Oh, sorry, I wrote that code last summer, when I didn't quite have the
kernel coding style in my blood yet :) But fortunately I shouldn't
need that comment at all in the next version...

>
> > +void __audit_tk_injoffset(struct timespec64 offset)
> > +{
> > +     audit_log(audit_context(), GFP_ATOMIC, AUDIT_TIME_INJOFFSET,
> > +               "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> > +}
> > +
> > @@ -1250,6 +1251,9 @@ out:
> >       /* signal hrtimers about time change */
> >       clock_was_set();
> >
> > +     if (!ret)
> > +             audit_tk_injoffset(ts_delta);
>
> This one does not need GFP_ATOMIC at all.
>
> > +
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(do_settimeofday64);
> > @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
> >               ret = timekeeping_inject_offset(&delta);
> >               if (ret)
> >                       return ret;
> > +
> > +             audit_tk_injoffset(delta);
> >       }
> >
> >       ktime_get_real_ts64(&ts);
>
> This can be done at the end of do_adjtimex() quite nicely in preemptible
> context.

But wait, isn't this call outside of the critical section as well? (I
must have been moving the call around when I was writing the code and
didn't realize that this function actually doesn't need GFP_ATOMIC at
all...) Or am I missing something?


--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak10 v6 0/2] audit: Log changes that can affect the system clock

[Ondrej Mosnacek]

On Thu, Mar 28, 2019 at 12:00 AM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Mar 25, 2019 at 10:50 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Thu, Mar 7, 2019 at 7:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > This patchset implements auditing of (syscall-triggered) changes that
> > > can modify or indirectly affect the system clock. Some of these
> > > changes can already be detected by simply logging relevant syscalls,
> > > but this has some disadvantages:
> > >   a) It is usually not possible to find out from the syscall records
> > >      the amount by which the time was shifted.
> > >   b) Syscalls like adjtimex(2) or clock_adjtime(2) can be used also
> > >      for read-only operations, which might flood the audit log with
> > >      false positives. (Note that these patches don't solve this
> > >      problem yet due to the limitations of current record filtering
> > >      capabilities.)
> > >
> > > The main motivation is to provide better reliability of timestamps
> > > on the system as mandated by the FPT_STM.1 security functional
> > > requirement from Common Criteria. This requirement apparently demands
> > > that it is possible to reconstruct from audit trail the old and new
> > > values of the time when it is adjusted (see [1]).
> > >
> > > The current version of the patchset logs the following changes:
> > >   - direct setting of system time to a given value
> > >   - direct injection of timekeeping offset
> > >   - adjustment of timekeeping's TAI offset
> > >   - NTP value adjustments:
> > >     - time_offset
> > >     - time_freq
> > >     - time_status
> > >     - time_adjust
> > >     - tick_usec
> > >
> > > Changes to the following NTP values are not logged, as they are not
> > > important for security:
> > >   - time_maxerror
> > >   - time_esterror
> > >   - time_constant
> > >
> > > Audit kernel GitHub issue: https://github.com/linux-audit/audit-kernel/issues/10
> > > Audit kernel RFE page: https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock
> > >
> > > Testing: Passed audit-testuite; functional tests TBD
> > >
> > > Changes in v6:
> > >   - Reorganized the patches to group changes by record type, not
> > >     kernel subsytem, as suggested in earlier discussions
> > >   - Added checks to ignore no-change events (new value == old value)
> > >   - Added TIME_INJOFFSET logging also to do_settimeofday64() to cover
> > >     syscalls such as settimeofday(2), stime(2), clock_settime(2)
> > >   - Created an RFE page on audit-kernel GitHub
> > > TODO:
> > >   - tests for audit-testsuite
> > >
> > > v5: https://www.redhat.com/archives/linux-audit/2018-August/msg00039.html
> > > Changes in v5:
> > >   - Dropped logging of some less important changes and update commit messages
> > >   - No longer mark the patchset as RFC
> > >
> > > v4: https://www.redhat.com/archives/linux-audit/2018-August/msg00023.html
> > > Changes in v4:
> > >   - Squashed first two patches into one
> > >   - Renamed ADJNTPVAL's "type" field to "op" to align with audit record
> > >     conventions
> > >   - Minor commit message editing
> > >   - Cc timekeeping/NTP people for feedback
> > >
> > > v3: https://www.redhat.com/archives/linux-audit/2018-July/msg00001.html
> > > Changes in v3:
> > >   - Switched to separate records for each variable
> > >   - Both old and new value is now reported for each change
> > >   - Injecting offset is reported via a separate record (since this
> > >     offset consists of two values and is added directly to the clock,
> > >     i.e. it doesn't make sense to log old and new value)
> > >   - Added example records produced by chronyd -q (see the commit message
> > >     of the last patch)
> > >
> > > v2: https://www.redhat.com/archives/linux-audit/2018-June/msg00114.html
> > > Changes in v2:
> > >   - The audit_adjtime() function has been modified to only log those
> > >     fields that contain values that are actually used, resulting in more
> > >     compact records.
> > >   - The audit_adjtime() call has been moved to do_adjtimex() in
> > >     timekeeping.c
> > >   - Added an additional patch (for review) that simplifies the detection
> > >     if the syscall is read-only.
> > >
> > > v1: https://www.redhat.com/archives/linux-audit/2018-June/msg00095.html
> > >
> > > [1] https://www.niap-ccevs.org/MMO/PP/pp_ca_v2.1.pdf -- section 5.1,
> > >     table 4
> > >
> > > Ondrej Mosnacek (2):
> > >   timekeeping: Audit clock adjustments
> > >   ntp: Audit NTP parameters adjustment
> > >
> > >  include/linux/audit.h      | 29 +++++++++++++++++++++++++++++
> > >  include/uapi/linux/audit.h |  2 ++
> > >  kernel/auditsc.c           | 15 +++++++++++++++
> > >  kernel/time/ntp.c          | 38 ++++++++++++++++++++++++++++++--------
> > >  kernel/time/timekeeping.c  |  6 ++++++
> > >  5 files changed, 82 insertions(+), 8 deletions(-)
> >
> > These patches look fine to me, but it would be really nice to get an
> > ACK from the time folks before I merge this into audit/next.  Time
> > folks, I know you've looked at previous versions of this patchset, can
> > you give this a quick look to make sure everything is still okay from
> > your perspective?
>
> Ondrej, please don't let the lack of response from the time folks keep
> you from working on the tests.  If you can get the tests ready in
> time, I see no reason why this couldn't get merged before the next
> merge window.

Sure, I already have them about 50% done. I hope to have them finished
sometime this week.

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Thomas Gleixner]

On Mon, 1 Apr 2019, Ondrej Mosnacek wrote:
> On Thu, Mar 28, 2019 at 1:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> > >                       /* adjtime() is independent from ntp_adjtime() */
> > >                       time_adjust = txc->offset;
> > >                       ntp_update_frequency();
> > > +
> > > +                     audit_ntp_adjust("adjust", save_adjust, txc->offset);
> > >               }
> > >               txc->offset = save_adjust;
> > >       } else {
> >
> > Not going to happen. We are not reshuffling all that code just to
> > accomodate random audit log invocations in a critical section plus having a
> > gazillion of GFP_ATOMIC allocation in the critical section just because.
> 
> OK, seems I underestimated the consequences of putting the logging
> calls directly in there. While I was offline over the weekend I
> already came up with a cleaner version that collects the changes in a
> structure and does the logging outside of the critical section. I
> currently does a few unnecessary writes into memory under
> CONFIG_AUDIT=n, but if that is an issue I can boost the abstraction or
> just add some #ifdefs to avoid that.

No ifdefs please. Aside of that, why do you need all those details of the
ntp internals in the first place? The changelog does not give me an answer
to that.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 1/2] timekeeping: Audit clock adjustments

[Thomas Gleixner]

On Mon, 1 Apr 2019, Ondrej Mosnacek wrote:
> On Thu, Mar 28, 2019 at 1:09 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > >  EXPORT_SYMBOL(do_settimeofday64);
> > > @@ -2322,6 +2326,8 @@ int do_adjtimex(struct timex *txc)
> > >               ret = timekeeping_inject_offset(&delta);
> > >               if (ret)
> > >                       return ret;
> > > +
> > > +             audit_tk_injoffset(delta);
> > >       }
> > >
> > >       ktime_get_real_ts64(&ts);
> >
> > This can be done at the end of do_adjtimex() quite nicely in preemptible
> > context.
> 
> But wait, isn't this call outside of the critical section as well? (I
> must have been moving the call around when I was writing the code and
> didn't realize that this function actually doesn't need GFP_ATOMIC at
> all...) Or am I missing something?

Nah. I was misreading it. Just it does not need GFP_ATOMIC at all. But then
you might just combine it with your new struct storage which you want to do
for __do_adjtimex() anyway.

Thanks,

	tglx

Re: [RFC PATCH ghak10 v6 2/2] ntp: Audit NTP parameters adjustment

[Ondrej Mosnacek]

On Tue, Apr 2, 2019 at 11:33 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Mon, 1 Apr 2019, Ondrej Mosnacek wrote:
> > On Thu, Mar 28, 2019 at 1:02 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> > > On Thu, 7 Mar 2019, Ondrej Mosnacek wrote:
> > > >                       /* adjtime() is independent from ntp_adjtime() */
> > > >                       time_adjust = txc->offset;
> > > >                       ntp_update_frequency();
> > > > +
> > > > +                     audit_ntp_adjust("adjust", save_adjust, txc->offset);
> > > >               }
> > > >               txc->offset = save_adjust;
> > > >       } else {
> > >
> > > Not going to happen. We are not reshuffling all that code just to
> > > accomodate random audit log invocations in a critical section plus having a
> > > gazillion of GFP_ATOMIC allocation in the critical section just because.
> >
> > OK, seems I underestimated the consequences of putting the logging
> > calls directly in there. While I was offline over the weekend I
> > already came up with a cleaner version that collects the changes in a
> > structure and does the logging outside of the critical section. I
> > currently does a few unnecessary writes into memory under
> > CONFIG_AUDIT=n, but if that is an issue I can boost the abstraction or
> > just add some #ifdefs to avoid that.
>
> No ifdefs please.

Yep, no worries, I already found a clean way to make all the audit
operations a no-op under CONFIG_AUDITSYSCALL=n that keeps all the
#ifdefs contained within <linux/audit.h>.

> Aside of that, why do you need all those details of the
> ntp internals in the first place? The changelog does not give me an answer
> to that.

Yeah, the changelog should be more explanatory, I'll try to improve
that in the next revision. I actually stated the justification in the
RFE page [1] (paragraphs 3-4) linked from the cover letter (I don't
blame you for not finding it...). The idea is that it is (or at least
seems) possible to do some evil changes to the clock indirectly by
modifying these parameters as well. Maybe it is a bit overkill, but
it's better to be safe than sorry... If anyone doesn't want the extra
records, they can be easily filtered out by adding a simple audit
rule.

[1] https://github.com/linux-audit/audit-kernel/wiki/RFE-More-detailed-auditing-of-changes-to-system-clock#feature-design

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.