linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH ghak10 v7 0/2] audit: Log changes that can affect the system clock
@ 2019-04-09 12:31 Ondrej Mosnacek
  2019-04-09 12:31 ` [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments Ondrej Mosnacek
  2019-04-09 12:31 ` [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment Ondrej Mosnacek
  0 siblings, 2 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-09 12:31 UTC (permalink / raw)
  To: linux-audit
  Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
	John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel

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 (may affect system time indirectly):
    - 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

Audit userspace PR: https://github.com/linux-audit/audit-userspace/pull/89
Audit testsuite PR: https://github.com/linux-audit/audit-testsuite/pull/82

Testing: Passed audit-testuite (including new functional test from PR#82)

Changes in v7:
  - Removed #include <uapi/linux/timex.h> from audit.h, since it is not
    needed
  - Moved audit_log() calls out of locked sections and switched audit
    allocations to GFP_KERNEL
  - Rebased onto latest audit/next
  - Switched the general NTP value type to long long (was s64),
    which corresponds better with the actual timex field types
  - Simplified commit log messages

v6: https://www.redhat.com/archives/linux-audit/2019-March/msg00016.html
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

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      | 68 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/audit.h |  2 ++
 kernel/auditsc.c           | 29 ++++++++++++++++
 kernel/time/ntp.c          | 22 ++++++++++--
 kernel/time/ntp_internal.h |  4 ++-
 kernel/time/timekeeping.c  | 13 +++++++-
 6 files changed, 133 insertions(+), 5 deletions(-)

-- 
2.20.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments
  2019-04-09 12:31 [PATCH ghak10 v7 0/2] audit: Log changes that can affect the system clock Ondrej Mosnacek
@ 2019-04-09 12:31 ` Ondrej Mosnacek
  2019-04-09 14:26   ` Richard Guy Briggs
  2019-04-10  6:25   ` Thomas Gleixner
  2019-04-09 12:31 ` [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment Ondrej Mosnacek
  1 sibling, 2 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-09 12:31 UTC (permalink / raw)
  To: linux-audit
  Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
	John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel

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

Example record (time was shifted backwards by ~16.125 seconds):

type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145

The records of this type will be associated with the corresponding
syscall records.

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

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 1e69d9fe16da..2c62c0468888 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -365,6 +365,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 +468,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 +591,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 3901c51c0b93..ab58d67baf4d 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 51a2ceb3a1ca..80dfe0cdc636 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2512,6 +2512,12 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+void __audit_tk_injoffset(struct timespec64 offset)
+{
+	audit_log(audit_context(), GFP_KERNEL, 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 f986e1918d12..3d24be4cd607 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 __kernel_timex *txc)
 		ret = timekeeping_inject_offset(&delta);
 		if (ret)
 			return ret;
+
+		audit_tk_injoffset(delta);
 	}
 
 	ktime_get_real_ts64(&ts);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment
  2019-04-09 12:31 [PATCH ghak10 v7 0/2] audit: Log changes that can affect the system clock Ondrej Mosnacek
  2019-04-09 12:31 ` [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments Ondrej Mosnacek
@ 2019-04-09 12:31 ` Ondrej Mosnacek
  2019-04-09 14:40   ` Richard Guy Briggs
  2019-04-10  6:24   ` Thomas Gleixner
  1 sibling, 2 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-09 12:31 UTC (permalink / raw)
  To: linux-audit
  Cc: Paul Moore, Richard Guy Briggs, Steve Grubb, Miroslav Lichvar,
	John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel

Emit an audit record every time selected NTP parameters are modified
from userspace (via adjtimex(2) or clock_adjtime(2)). These parameters
may be used to indirectly change system clock, and thus their
modifications should be audited.

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

Example records:

type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000

The records of this type will be associated with the corresponding
syscall records.

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      | 54 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/audit.h |  1 +
 kernel/auditsc.c           | 23 ++++++++++++++++
 kernel/time/ntp.c          | 22 +++++++++++++---
 kernel/time/ntp_internal.h |  4 ++-
 kernel/time/timekeeping.c  |  7 ++++-
 6 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 2c62c0468888..1c372ad7ebe9 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -86,6 +86,26 @@ struct audit_field {
 	u32				op;
 };
 
+#define AUDIT_NTP_OFFSET	0
+#define AUDIT_NTP_FREQ		1
+#define AUDIT_NTP_STATUS	2
+#define AUDIT_NTP_TAI		3
+#define AUDIT_NTP_TICK		4
+#define AUDIT_NTP_ADJUST	5
+#define AUDIT_NTP_NVALS		6 /* count */
+
+#ifdef CONFIG_AUDITSYSCALL
+struct audit_ntp_val {
+	s64 oldval, newval;
+};
+
+struct audit_ntp_data {
+	struct audit_ntp_val vals[AUDIT_NTP_NVALS];
+};
+#else
+struct audit_ntp_data {};
+#endif
+
 extern int is_audit_feature_set(int which);
 
 extern int __init audit_register_class(int class, unsigned *list);
@@ -366,6 +386,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_log(const struct audit_ntp_data *ad);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -478,6 +499,27 @@ static inline void audit_tk_injoffset(struct timespec64 offset)
 		__audit_tk_injoffset(offset);
 }
 
+static inline void audit_ntp_init(struct audit_ntp_data *ad)
+{
+	memset(ad, 0, sizeof(*ad));
+}
+
+static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
+{
+	ad->vals[id].oldval = val;
+}
+
+static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
+{
+	ad->vals[id].newval = val;
+}
+
+static inline void audit_ntp_log(const struct audit_ntp_data *ad)
+{
+	if (!audit_dummy_context())
+		__audit_ntp_log(ad);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -594,6 +636,18 @@ static inline void audit_fanotify(unsigned int response)
 static inline void audit_tk_injoffset(struct timespec64 offset)
 { }
 
+static inline void audit_ntp_init(struct audit_ntp_data *ad)
+{ }
+
+static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
+{ }
+
+static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
+{ }
+
+static inline void audit_ntp_log(const struct audit_ntp_data *ad)
+{ }
+
 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 ab58d67baf4d..a1280af20336 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 80dfe0cdc636..519ada7522ad 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2518,6 +2518,29 @@ void __audit_tk_injoffset(struct timespec64 offset)
 		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
 }
 
+static void audit_log_ntp_val(const struct audit_ntp_data *ad,
+			      const char *type, int id)
+{
+	const struct audit_ntp_val *val = &ad->vals[id];
+
+	if (val->newval == val->oldval)
+		return;
+
+	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
+		  "op=%s old=%lli new=%lli", type,
+		  (long long)val->oldval, (long long)val->newval);
+}
+
+void __audit_ntp_log(const struct audit_ntp_data *ad)
+{
+	audit_log_ntp_val(ad, "offset",	AUDIT_NTP_OFFSET);
+	audit_log_ntp_val(ad, "freq",	AUDIT_NTP_FREQ);
+	audit_log_ntp_val(ad, "status",	AUDIT_NTP_STATUS);
+	audit_log_ntp_val(ad, "tai",	AUDIT_NTP_TAI);
+	audit_log_ntp_val(ad, "tick",	AUDIT_NTP_TICK);
+	audit_log_ntp_val(ad, "adjust",	AUDIT_NTP_ADJUST);
+}
+
 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 92a90014a925..ac5555e25733 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"
@@ -709,7 +710,7 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
  * kernel time-keeping variables. used by xntpd.
  */
 int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
-		  s32 *time_tai)
+		  s32 *time_tai, struct audit_ntp_data *ad)
 {
 	int result;
 
@@ -720,14 +721,29 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
 			/* adjtime() is independent from ntp_adjtime() */
 			time_adjust = txc->offset;
 			ntp_update_frequency();
+
+			audit_ntp_set_old(ad, AUDIT_NTP_ADJUST,	save_adjust);
+			audit_ntp_set_new(ad, AUDIT_NTP_ADJUST,	time_adjust);
 		}
 		txc->offset = save_adjust;
 	} else {
-
 		/* If there are input parameters, then process them: */
-		if (txc->modes)
+		if (txc->modes) {
+			audit_ntp_set_old(ad, AUDIT_NTP_OFFSET,	time_offset);
+			audit_ntp_set_old(ad, AUDIT_NTP_FREQ,	time_freq);
+			audit_ntp_set_old(ad, AUDIT_NTP_STATUS,	time_status);
+			audit_ntp_set_old(ad, AUDIT_NTP_TAI,	*time_tai);
+			audit_ntp_set_old(ad, AUDIT_NTP_TICK,	tick_usec);
+
 			process_adjtimex_modes(txc, time_tai);
 
+			audit_ntp_set_new(ad, AUDIT_NTP_OFFSET,	time_offset);
+			audit_ntp_set_new(ad, AUDIT_NTP_FREQ,	time_freq);
+			audit_ntp_set_new(ad, AUDIT_NTP_STATUS,	time_status);
+			audit_ntp_set_new(ad, AUDIT_NTP_TAI,	*time_tai);
+			audit_ntp_set_new(ad, AUDIT_NTP_TICK,	tick_usec);
+		}
+
 		txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
 				  NTP_SCALE_SHIFT);
 		if (!(time_status & STA_NANO))
diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
index 40e6122e634e..908ecaa65fc3 100644
--- a/kernel/time/ntp_internal.h
+++ b/kernel/time/ntp_internal.h
@@ -8,6 +8,8 @@ extern void ntp_clear(void);
 extern u64 ntp_tick_length(void);
 extern ktime_t ntp_get_next_leap(void);
 extern int second_overflow(time64_t secs);
-extern int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts, s32 *time_tai);
+extern int __do_adjtimex(struct __kernel_timex *txc,
+			 const struct timespec64 *ts,
+			 s32 *time_tai, struct audit_ntp_data *ad);
 extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);
 #endif /* _LINUX_NTP_INTERNAL_H */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3d24be4cd607..f366f2fdf1b0 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2307,6 +2307,7 @@ static int timekeeping_validate_timex(const struct __kernel_timex *txc)
 int do_adjtimex(struct __kernel_timex *txc)
 {
 	struct timekeeper *tk = &tk_core.timekeeper;
+	struct audit_ntp_data ad;
 	unsigned long flags;
 	struct timespec64 ts;
 	s32 orig_tai, tai;
@@ -2330,13 +2331,15 @@ int do_adjtimex(struct __kernel_timex *txc)
 		audit_tk_injoffset(delta);
 	}
 
+	audit_ntp_init(&ad);
+
 	ktime_get_real_ts64(&ts);
 
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 
 	orig_tai = tai = tk->tai_offset;
-	ret = __do_adjtimex(txc, &ts, &tai);
+	ret = __do_adjtimex(txc, &ts, &tai, &ad);
 
 	if (tai != orig_tai) {
 		__timekeeping_set_tai_offset(tk, tai);
@@ -2347,6 +2350,8 @@ int do_adjtimex(struct __kernel_timex *txc)
 	write_seqcount_end(&tk_core.seq);
 	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
 
+	audit_ntp_log(&ad);
+
 	/* Update the multiplier immediately if frequency was set directly */
 	if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
 		timekeeping_advance(TK_ADV_FREQ);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments
  2019-04-09 12:31 ` [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments Ondrej Mosnacek
@ 2019-04-09 14:26   ` Richard Guy Briggs
  2019-04-09 15:05     ` Ondrej Mosnacek
  2019-04-10  6:25   ` Thomas Gleixner
  1 sibling, 1 reply; 10+ messages in thread
From: Richard Guy Briggs @ 2019-04-09 14:26 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: linux-audit, Paul Moore, Steve Grubb, Miroslav Lichvar,
	John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel

On 2019-04-09 14:31, 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
> 
> Example record (time was shifted backwards by ~16.125 seconds):
> 
> type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145

I have a minor nit about the interpretation of these values.  I realize
the values printed are the ones that are supplied.  When the sec
value is negative, does that imply that the nsec value has the same
sign?  IOW, are the two values added with a multiplier to the nsec
value, or are they concatenated with a decimal in the middle?  I expect
the latter.  In the current userspace support, there is no explicit type
support included yet for interpreted fields.  This is just a minor
documentation issue to me.  

> The records of this type will be associated with the corresponding
> syscall records.
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>

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

> ---
>  include/linux/audit.h      | 14 ++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/auditsc.c           |  6 ++++++
>  kernel/time/timekeeping.c  |  6 ++++++
>  4 files changed, 27 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 1e69d9fe16da..2c62c0468888 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -365,6 +365,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 +468,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 +591,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 3901c51c0b93..ab58d67baf4d 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 51a2ceb3a1ca..80dfe0cdc636 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2512,6 +2512,12 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
>  
> +void __audit_tk_injoffset(struct timespec64 offset)
> +{
> +	audit_log(audit_context(), GFP_KERNEL, 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 f986e1918d12..3d24be4cd607 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 __kernel_timex *txc)
>  		ret = timekeeping_inject_offset(&delta);
>  		if (ret)
>  			return ret;
> +
> +		audit_tk_injoffset(delta);
>  	}
>  
>  	ktime_get_real_ts64(&ts);
> -- 
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment
  2019-04-09 12:31 ` [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment Ondrej Mosnacek
@ 2019-04-09 14:40   ` Richard Guy Briggs
  2019-04-09 15:10     ` Ondrej Mosnacek
  2019-04-10  6:24   ` Thomas Gleixner
  1 sibling, 1 reply; 10+ messages in thread
From: Richard Guy Briggs @ 2019-04-09 14:40 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: linux-audit, Paul Moore, Steve Grubb, Miroslav Lichvar,
	John Stultz, Thomas Gleixner, Stephen Boyd, linux-kernel

On 2019-04-09 14:31, Ondrej Mosnacek wrote:
> Emit an audit record every time selected NTP parameters are modified
> from userspace (via adjtimex(2) or clock_adjtime(2)). These parameters
> may be used to indirectly change system clock, and thus their
> modifications should be audited.
> 
> 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
> 
> Example records:
> 
> type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
> type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000
> 
> The records of this type will be associated with the corresponding
> syscall records.
> 
> 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%

(I think you want an "(AUDITED)" at the end of the "time_freq" line.)

>     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>

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

> ---
>  include/linux/audit.h      | 54 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/auditsc.c           | 23 ++++++++++++++++
>  kernel/time/ntp.c          | 22 +++++++++++++---
>  kernel/time/ntp_internal.h |  4 ++-
>  kernel/time/timekeeping.c  |  7 ++++-
>  6 files changed, 106 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 2c62c0468888..1c372ad7ebe9 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -86,6 +86,26 @@ struct audit_field {
>  	u32				op;
>  };
>  
> +#define AUDIT_NTP_OFFSET	0
> +#define AUDIT_NTP_FREQ		1
> +#define AUDIT_NTP_STATUS	2
> +#define AUDIT_NTP_TAI		3
> +#define AUDIT_NTP_TICK		4
> +#define AUDIT_NTP_ADJUST	5
> +#define AUDIT_NTP_NVALS		6 /* count */
> +
> +#ifdef CONFIG_AUDITSYSCALL
> +struct audit_ntp_val {
> +	s64 oldval, newval;
> +};
> +
> +struct audit_ntp_data {
> +	struct audit_ntp_val vals[AUDIT_NTP_NVALS];
> +};
> +#else
> +struct audit_ntp_data {};
> +#endif
> +
>  extern int is_audit_feature_set(int which);
>  
>  extern int __init audit_register_class(int class, unsigned *list);
> @@ -366,6 +386,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_log(const struct audit_ntp_data *ad);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -478,6 +499,27 @@ static inline void audit_tk_injoffset(struct timespec64 offset)
>  		__audit_tk_injoffset(offset);
>  }
>  
> +static inline void audit_ntp_init(struct audit_ntp_data *ad)
> +{
> +	memset(ad, 0, sizeof(*ad));
> +}
> +
> +static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
> +{
> +	ad->vals[id].oldval = val;
> +}
> +
> +static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
> +{
> +	ad->vals[id].newval = val;
> +}
> +
> +static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> +{
> +	if (!audit_dummy_context())
> +		__audit_ntp_log(ad);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -594,6 +636,18 @@ static inline void audit_fanotify(unsigned int response)
>  static inline void audit_tk_injoffset(struct timespec64 offset)
>  { }
>  
> +static inline void audit_ntp_init(struct audit_ntp_data *ad)
> +{ }
> +
> +static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
> +{ }
> +
> +static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
> +{ }
> +
> +static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> +{ }
> +
>  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 ab58d67baf4d..a1280af20336 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 80dfe0cdc636..519ada7522ad 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2518,6 +2518,29 @@ void __audit_tk_injoffset(struct timespec64 offset)
>  		  "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
>  }
>  
> +static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> +			      const char *type, int id)
> +{
> +	const struct audit_ntp_val *val = &ad->vals[id];
> +
> +	if (val->newval == val->oldval)
> +		return;
> +
> +	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
> +		  "op=%s old=%lli new=%lli", type,
> +		  (long long)val->oldval, (long long)val->newval);
> +}
> +
> +void __audit_ntp_log(const struct audit_ntp_data *ad)
> +{
> +	audit_log_ntp_val(ad, "offset",	AUDIT_NTP_OFFSET);
> +	audit_log_ntp_val(ad, "freq",	AUDIT_NTP_FREQ);
> +	audit_log_ntp_val(ad, "status",	AUDIT_NTP_STATUS);
> +	audit_log_ntp_val(ad, "tai",	AUDIT_NTP_TAI);
> +	audit_log_ntp_val(ad, "tick",	AUDIT_NTP_TICK);
> +	audit_log_ntp_val(ad, "adjust",	AUDIT_NTP_ADJUST);
> +}
> +
>  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 92a90014a925..ac5555e25733 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"
> @@ -709,7 +710,7 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
>   * kernel time-keeping variables. used by xntpd.
>   */
>  int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
> -		  s32 *time_tai)
> +		  s32 *time_tai, struct audit_ntp_data *ad)
>  {
>  	int result;
>  
> @@ -720,14 +721,29 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
>  			/* adjtime() is independent from ntp_adjtime() */
>  			time_adjust = txc->offset;
>  			ntp_update_frequency();
> +
> +			audit_ntp_set_old(ad, AUDIT_NTP_ADJUST,	save_adjust);
> +			audit_ntp_set_new(ad, AUDIT_NTP_ADJUST,	time_adjust);
>  		}
>  		txc->offset = save_adjust;
>  	} else {
> -
>  		/* If there are input parameters, then process them: */
> -		if (txc->modes)
> +		if (txc->modes) {
> +			audit_ntp_set_old(ad, AUDIT_NTP_OFFSET,	time_offset);
> +			audit_ntp_set_old(ad, AUDIT_NTP_FREQ,	time_freq);
> +			audit_ntp_set_old(ad, AUDIT_NTP_STATUS,	time_status);
> +			audit_ntp_set_old(ad, AUDIT_NTP_TAI,	*time_tai);
> +			audit_ntp_set_old(ad, AUDIT_NTP_TICK,	tick_usec);
> +
>  			process_adjtimex_modes(txc, time_tai);
>  
> +			audit_ntp_set_new(ad, AUDIT_NTP_OFFSET,	time_offset);
> +			audit_ntp_set_new(ad, AUDIT_NTP_FREQ,	time_freq);
> +			audit_ntp_set_new(ad, AUDIT_NTP_STATUS,	time_status);
> +			audit_ntp_set_new(ad, AUDIT_NTP_TAI,	*time_tai);
> +			audit_ntp_set_new(ad, AUDIT_NTP_TICK,	tick_usec);
> +		}
> +
>  		txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
>  				  NTP_SCALE_SHIFT);
>  		if (!(time_status & STA_NANO))
> diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
> index 40e6122e634e..908ecaa65fc3 100644
> --- a/kernel/time/ntp_internal.h
> +++ b/kernel/time/ntp_internal.h
> @@ -8,6 +8,8 @@ extern void ntp_clear(void);
>  extern u64 ntp_tick_length(void);
>  extern ktime_t ntp_get_next_leap(void);
>  extern int second_overflow(time64_t secs);
> -extern int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts, s32 *time_tai);
> +extern int __do_adjtimex(struct __kernel_timex *txc,
> +			 const struct timespec64 *ts,
> +			 s32 *time_tai, struct audit_ntp_data *ad);
>  extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);
>  #endif /* _LINUX_NTP_INTERNAL_H */
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 3d24be4cd607..f366f2fdf1b0 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -2307,6 +2307,7 @@ static int timekeeping_validate_timex(const struct __kernel_timex *txc)
>  int do_adjtimex(struct __kernel_timex *txc)
>  {
>  	struct timekeeper *tk = &tk_core.timekeeper;
> +	struct audit_ntp_data ad;
>  	unsigned long flags;
>  	struct timespec64 ts;
>  	s32 orig_tai, tai;
> @@ -2330,13 +2331,15 @@ int do_adjtimex(struct __kernel_timex *txc)
>  		audit_tk_injoffset(delta);
>  	}
>  
> +	audit_ntp_init(&ad);
> +
>  	ktime_get_real_ts64(&ts);
>  
>  	raw_spin_lock_irqsave(&timekeeper_lock, flags);
>  	write_seqcount_begin(&tk_core.seq);
>  
>  	orig_tai = tai = tk->tai_offset;
> -	ret = __do_adjtimex(txc, &ts, &tai);
> +	ret = __do_adjtimex(txc, &ts, &tai, &ad);
>  
>  	if (tai != orig_tai) {
>  		__timekeeping_set_tai_offset(tk, tai);
> @@ -2347,6 +2350,8 @@ int do_adjtimex(struct __kernel_timex *txc)
>  	write_seqcount_end(&tk_core.seq);
>  	raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
>  
> +	audit_ntp_log(&ad);
> +
>  	/* Update the multiplier immediately if frequency was set directly */
>  	if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
>  		timekeeping_advance(TK_ADV_FREQ);
> -- 
> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments
  2019-04-09 14:26   ` Richard Guy Briggs
@ 2019-04-09 15:05     ` Ondrej Mosnacek
  0 siblings, 0 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-09 15:05 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, Paul Moore, Steve Grubb,
	Miroslav Lichvar, John Stultz, Thomas Gleixner, Stephen Boyd,
	Linux kernel mailing list

On Tue, Apr 9, 2019 at 4:26 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-04-09 14:31, 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
> >
> > Example record (time was shifted backwards by ~16.125 seconds):
> >
> > type=TIME_INJOFFSET msg=audit(1530616049.652:13): sec=-16 nsec=124887145
>
> I have a minor nit about the interpretation of these values.  I realize
> the values printed are the ones that are supplied.  When the sec
> value is negative, does that imply that the nsec value has the same
> sign?  IOW, are the two values added with a multiplier to the nsec
> value, or are they concatenated with a decimal in the middle?  I expect
> the latter.  In the current userspace support, there is no explicit type
> support included yet for interpreted fields.  This is just a minor
> documentation issue to me.

Good point! I wrote the sentence too quickly without thinking about
it... I actually had to find this out for the tests (and looking at it
again, I got it wrong there as well, although in a different way...) -
it is actually the former variant (multiplier + addition). So the
"~16.125" should actually be "~15.875" (and I need to fix the
testsuite pull request...).

> > The records of this type will be associated with the corresponding
> > syscall records.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
>
> > ---
> >  include/linux/audit.h      | 14 ++++++++++++++
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/auditsc.c           |  6 ++++++
> >  kernel/time/timekeeping.c  |  6 ++++++
> >  4 files changed, 27 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 1e69d9fe16da..2c62c0468888 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -365,6 +365,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 +468,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 +591,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 3901c51c0b93..ab58d67baf4d 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 51a2ceb3a1ca..80dfe0cdc636 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2512,6 +2512,12 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +void __audit_tk_injoffset(struct timespec64 offset)
> > +{
> > +     audit_log(audit_context(), GFP_KERNEL, 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 f986e1918d12..3d24be4cd607 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 __kernel_timex *txc)
> >               ret = timekeeping_inject_offset(&delta);
> >               if (ret)
> >                       return ret;
> > +
> > +             audit_tk_injoffset(delta);
> >       }
> >
> >       ktime_get_real_ts64(&ts);
> > --
> > 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>
Software Engineer, Security Technologies
Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment
  2019-04-09 14:40   ` Richard Guy Briggs
@ 2019-04-09 15:10     ` Ondrej Mosnacek
  0 siblings, 0 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-09 15:10 UTC (permalink / raw)
  To: Richard Guy Briggs
  Cc: Linux-Audit Mailing List, Paul Moore, Steve Grubb,
	Miroslav Lichvar, John Stultz, Thomas Gleixner, Stephen Boyd,
	Linux kernel mailing list

On Tue, Apr 9, 2019 at 4:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-04-09 14:31, Ondrej Mosnacek wrote:
> > Emit an audit record every time selected NTP parameters are modified
> > from userspace (via adjtimex(2) or clock_adjtime(2)). These parameters
> > may be used to indirectly change system clock, and thus their
> > modifications should be audited.
> >
> > 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
> >
> > Example records:
> >
> > type=TIME_ADJNTPVAL msg=audit(1530616044.507:7): op=status old=64 new=8256
> > type=TIME_ADJNTPVAL msg=audit(1530616044.511:11): op=freq old=0 new=49180377088000
> >
> > The records of this type will be associated with the corresponding
> > syscall records.
> >
> > 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%
>
> (I think you want an "(AUDITED)" at the end of the "time_freq" line.)

Argh, yeah... Paul, would you be OK with fixing up these nits manually
or should I rather send an updated v8 (if there are no change requests
from the timekeeping folks)?

>
> >     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>
>
> Reviewed-by: Richard Guy Briggs <rgb@redhat.com>
>
> > ---
> >  include/linux/audit.h      | 54 ++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/audit.h |  1 +
> >  kernel/auditsc.c           | 23 ++++++++++++++++
> >  kernel/time/ntp.c          | 22 +++++++++++++---
> >  kernel/time/ntp_internal.h |  4 ++-
> >  kernel/time/timekeeping.c  |  7 ++++-
> >  6 files changed, 106 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 2c62c0468888..1c372ad7ebe9 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -86,6 +86,26 @@ struct audit_field {
> >       u32                             op;
> >  };
> >
> > +#define AUDIT_NTP_OFFSET     0
> > +#define AUDIT_NTP_FREQ               1
> > +#define AUDIT_NTP_STATUS     2
> > +#define AUDIT_NTP_TAI                3
> > +#define AUDIT_NTP_TICK               4
> > +#define AUDIT_NTP_ADJUST     5
> > +#define AUDIT_NTP_NVALS              6 /* count */
> > +
> > +#ifdef CONFIG_AUDITSYSCALL
> > +struct audit_ntp_val {
> > +     s64 oldval, newval;
> > +};
> > +
> > +struct audit_ntp_data {
> > +     struct audit_ntp_val vals[AUDIT_NTP_NVALS];
> > +};
> > +#else
> > +struct audit_ntp_data {};
> > +#endif
> > +
> >  extern int is_audit_feature_set(int which);
> >
> >  extern int __init audit_register_class(int class, unsigned *list);
> > @@ -366,6 +386,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_log(const struct audit_ntp_data *ad);
> >
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -478,6 +499,27 @@ static inline void audit_tk_injoffset(struct timespec64 offset)
> >               __audit_tk_injoffset(offset);
> >  }
> >
> > +static inline void audit_ntp_init(struct audit_ntp_data *ad)
> > +{
> > +     memset(ad, 0, sizeof(*ad));
> > +}
> > +
> > +static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
> > +{
> > +     ad->vals[id].oldval = val;
> > +}
> > +
> > +static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
> > +{
> > +     ad->vals[id].newval = val;
> > +}
> > +
> > +static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> > +{
> > +     if (!audit_dummy_context())
> > +             __audit_ntp_log(ad);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -594,6 +636,18 @@ static inline void audit_fanotify(unsigned int response)
> >  static inline void audit_tk_injoffset(struct timespec64 offset)
> >  { }
> >
> > +static inline void audit_ntp_init(struct audit_ntp_data *ad)
> > +{ }
> > +
> > +static inline void audit_ntp_set_old(struct audit_ntp_data *ad, int id, s64 val)
> > +{ }
> > +
> > +static inline void audit_ntp_set_new(struct audit_ntp_data *ad, int id, s64 val)
> > +{ }
> > +
> > +static inline void audit_ntp_log(const struct audit_ntp_data *ad)
> > +{ }
> > +
> >  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 ab58d67baf4d..a1280af20336 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 80dfe0cdc636..519ada7522ad 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2518,6 +2518,29 @@ void __audit_tk_injoffset(struct timespec64 offset)
> >                 "sec=%lli nsec=%li", (long long)offset.tv_sec, offset.tv_nsec);
> >  }
> >
> > +static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> > +                           const char *type, int id)
> > +{
> > +     const struct audit_ntp_val *val = &ad->vals[id];
> > +
> > +     if (val->newval == val->oldval)
> > +             return;
> > +
> > +     audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
> > +               "op=%s old=%lli new=%lli", type,
> > +               (long long)val->oldval, (long long)val->newval);
> > +}
> > +
> > +void __audit_ntp_log(const struct audit_ntp_data *ad)
> > +{
> > +     audit_log_ntp_val(ad, "offset", AUDIT_NTP_OFFSET);
> > +     audit_log_ntp_val(ad, "freq",   AUDIT_NTP_FREQ);
> > +     audit_log_ntp_val(ad, "status", AUDIT_NTP_STATUS);
> > +     audit_log_ntp_val(ad, "tai",    AUDIT_NTP_TAI);
> > +     audit_log_ntp_val(ad, "tick",   AUDIT_NTP_TICK);
> > +     audit_log_ntp_val(ad, "adjust", AUDIT_NTP_ADJUST);
> > +}
> > +
> >  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 92a90014a925..ac5555e25733 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"
> > @@ -709,7 +710,7 @@ static inline void process_adjtimex_modes(const struct __kernel_timex *txc,
> >   * kernel time-keeping variables. used by xntpd.
> >   */
> >  int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
> > -               s32 *time_tai)
> > +               s32 *time_tai, struct audit_ntp_data *ad)
> >  {
> >       int result;
> >
> > @@ -720,14 +721,29 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
> >                       /* adjtime() is independent from ntp_adjtime() */
> >                       time_adjust = txc->offset;
> >                       ntp_update_frequency();
> > +
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_ADJUST, save_adjust);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_ADJUST, time_adjust);
> >               }
> >               txc->offset = save_adjust;
> >       } else {
> > -
> >               /* If there are input parameters, then process them: */
> > -             if (txc->modes)
> > +             if (txc->modes) {
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_OFFSET, time_offset);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_FREQ,   time_freq);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_STATUS, time_status);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_TAI,    *time_tai);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_TICK,   tick_usec);
> > +
> >                       process_adjtimex_modes(txc, time_tai);
> >
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_OFFSET, time_offset);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_FREQ,   time_freq);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_STATUS, time_status);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_TAI,    *time_tai);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_TICK,   tick_usec);
> > +             }
> > +
> >               txc->offset = shift_right(time_offset * NTP_INTERVAL_FREQ,
> >                                 NTP_SCALE_SHIFT);
> >               if (!(time_status & STA_NANO))
> > diff --git a/kernel/time/ntp_internal.h b/kernel/time/ntp_internal.h
> > index 40e6122e634e..908ecaa65fc3 100644
> > --- a/kernel/time/ntp_internal.h
> > +++ b/kernel/time/ntp_internal.h
> > @@ -8,6 +8,8 @@ extern void ntp_clear(void);
> >  extern u64 ntp_tick_length(void);
> >  extern ktime_t ntp_get_next_leap(void);
> >  extern int second_overflow(time64_t secs);
> > -extern int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts, s32 *time_tai);
> > +extern int __do_adjtimex(struct __kernel_timex *txc,
> > +                      const struct timespec64 *ts,
> > +                      s32 *time_tai, struct audit_ntp_data *ad);
> >  extern void __hardpps(const struct timespec64 *phase_ts, const struct timespec64 *raw_ts);
> >  #endif /* _LINUX_NTP_INTERNAL_H */
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index 3d24be4cd607..f366f2fdf1b0 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -2307,6 +2307,7 @@ static int timekeeping_validate_timex(const struct __kernel_timex *txc)
> >  int do_adjtimex(struct __kernel_timex *txc)
> >  {
> >       struct timekeeper *tk = &tk_core.timekeeper;
> > +     struct audit_ntp_data ad;
> >       unsigned long flags;
> >       struct timespec64 ts;
> >       s32 orig_tai, tai;
> > @@ -2330,13 +2331,15 @@ int do_adjtimex(struct __kernel_timex *txc)
> >               audit_tk_injoffset(delta);
> >       }
> >
> > +     audit_ntp_init(&ad);
> > +
> >       ktime_get_real_ts64(&ts);
> >
> >       raw_spin_lock_irqsave(&timekeeper_lock, flags);
> >       write_seqcount_begin(&tk_core.seq);
> >
> >       orig_tai = tai = tk->tai_offset;
> > -     ret = __do_adjtimex(txc, &ts, &tai);
> > +     ret = __do_adjtimex(txc, &ts, &tai, &ad);
> >
> >       if (tai != orig_tai) {
> >               __timekeeping_set_tai_offset(tk, tai);
> > @@ -2347,6 +2350,8 @@ int do_adjtimex(struct __kernel_timex *txc)
> >       write_seqcount_end(&tk_core.seq);
> >       raw_spin_unlock_irqrestore(&timekeeper_lock, flags);
> >
> > +     audit_ntp_log(&ad);
> > +
> >       /* Update the multiplier immediately if frequency was set directly */
> >       if (txc->modes & (ADJ_FREQUENCY | ADJ_TICK))
> >               timekeeping_advance(TK_ADV_FREQ);
> > --
> > 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>
Software Engineer, Security Technologies
Red Hat, Inc.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment
  2019-04-09 12:31 ` [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment Ondrej Mosnacek
  2019-04-09 14:40   ` Richard Guy Briggs
@ 2019-04-10  6:24   ` Thomas Gleixner
  2019-04-10  7:29     ` Ondrej Mosnacek
  1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2019-04-10  6:24 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: linux-audit, Paul Moore, Richard Guy Briggs, Steve Grubb,
	Miroslav Lichvar, John Stultz, Stephen Boyd, linux-kernel

On Tue, 9 Apr 2019, Ondrej Mosnacek wrote:
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 2c62c0468888..1c372ad7ebe9 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -86,6 +86,26 @@ struct audit_field {
>  	u32				op;
>  };
>  
> +#define AUDIT_NTP_OFFSET	0
> +#define AUDIT_NTP_FREQ		1
> +#define AUDIT_NTP_STATUS	2
> +#define AUDIT_NTP_TAI		3
> +#define AUDIT_NTP_TICK		4
> +#define AUDIT_NTP_ADJUST	5
> +#define AUDIT_NTP_NVALS		6 /* count */

That should be an named enum and the id argument should be the same type.

> @@ -720,14 +721,29 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
>  			/* adjtime() is independent from ntp_adjtime() */
>  			time_adjust = txc->offset;
>  			ntp_update_frequency();
> +
> +			audit_ntp_set_old(ad, AUDIT_NTP_ADJUST,	save_adjust);
> +			audit_ntp_set_new(ad, AUDIT_NTP_ADJUST,	time_adjust);
>  		}
>  		txc->offset = save_adjust;
>  	} else {
> -
>  		/* If there are input parameters, then process them: */
> -		if (txc->modes)
> +		if (txc->modes) {
> +			audit_ntp_set_old(ad, AUDIT_NTP_OFFSET,	time_offset);
> +			audit_ntp_set_old(ad, AUDIT_NTP_FREQ,	time_freq);
> +			audit_ntp_set_old(ad, AUDIT_NTP_STATUS,	time_status);
> +			audit_ntp_set_old(ad, AUDIT_NTP_TAI,	*time_tai);
> +			audit_ntp_set_old(ad, AUDIT_NTP_TICK,	tick_usec);
> +
>  			process_adjtimex_modes(txc, time_tai);
>  
> +			audit_ntp_set_new(ad, AUDIT_NTP_OFFSET,	time_offset);
> +			audit_ntp_set_new(ad, AUDIT_NTP_FREQ,	time_freq);
> +			audit_ntp_set_new(ad, AUDIT_NTP_STATUS,	time_status);
> +			audit_ntp_set_new(ad, AUDIT_NTP_TAI,	*time_tai);
> +			audit_ntp_set_new(ad, AUDIT_NTP_TICK,	tick_usec);
> +		}

Yes, that looks much more palatable! Nice work!

With the above addressed:

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments
  2019-04-09 12:31 ` [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments Ondrej Mosnacek
  2019-04-09 14:26   ` Richard Guy Briggs
@ 2019-04-10  6:25   ` Thomas Gleixner
  1 sibling, 0 replies; 10+ messages in thread
From: Thomas Gleixner @ 2019-04-10  6:25 UTC (permalink / raw)
  To: Ondrej Mosnacek
  Cc: linux-audit, Paul Moore, Richard Guy Briggs, Steve Grubb,
	Miroslav Lichvar, John Stultz, Stephen Boyd, linux-kernel

On Tue, 9 Apr 2019, Ondrej Mosnacek wrote:
> --- 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 __kernel_timex *txc)
>  		ret = timekeeping_inject_offset(&delta);
>  		if (ret)
>  			return ret;
> +
> +		audit_tk_injoffset(delta);
>  	}
>  
>  	ktime_get_real_ts64(&ts);

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment
  2019-04-10  6:24   ` Thomas Gleixner
@ 2019-04-10  7:29     ` Ondrej Mosnacek
  0 siblings, 0 replies; 10+ messages in thread
From: Ondrej Mosnacek @ 2019-04-10  7:29 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Linux-Audit Mailing List, Paul Moore, Richard Guy Briggs,
	Steve Grubb, Miroslav Lichvar, John Stultz, Stephen Boyd,
	Linux kernel mailing list

On Wed, Apr 10, 2019 at 9:03 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 9 Apr 2019, Ondrej Mosnacek wrote:
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 2c62c0468888..1c372ad7ebe9 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -86,6 +86,26 @@ struct audit_field {
> >       u32                             op;
> >  };
> >
> > +#define AUDIT_NTP_OFFSET     0
> > +#define AUDIT_NTP_FREQ               1
> > +#define AUDIT_NTP_STATUS     2
> > +#define AUDIT_NTP_TAI                3
> > +#define AUDIT_NTP_TICK               4
> > +#define AUDIT_NTP_ADJUST     5
> > +#define AUDIT_NTP_NVALS              6 /* count */
>
> That should be an named enum and the id argument should be the same type.

I kind of instinctively went for macros to be consistent with the rest
of the file, but on second thought that's really not a good reason to
keep doing the wrong thing... I'll change it to enum in v8.

>
> > @@ -720,14 +721,29 @@ int __do_adjtimex(struct __kernel_timex *txc, const struct timespec64 *ts,
> >                       /* adjtime() is independent from ntp_adjtime() */
> >                       time_adjust = txc->offset;
> >                       ntp_update_frequency();
> > +
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_ADJUST, save_adjust);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_ADJUST, time_adjust);
> >               }
> >               txc->offset = save_adjust;
> >       } else {
> > -
> >               /* If there are input parameters, then process them: */
> > -             if (txc->modes)
> > +             if (txc->modes) {
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_OFFSET, time_offset);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_FREQ,   time_freq);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_STATUS, time_status);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_TAI,    *time_tai);
> > +                     audit_ntp_set_old(ad, AUDIT_NTP_TICK,   tick_usec);
> > +
> >                       process_adjtimex_modes(txc, time_tai);
> >
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_OFFSET, time_offset);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_FREQ,   time_freq);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_STATUS, time_status);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_TAI,    *time_tai);
> > +                     audit_ntp_set_new(ad, AUDIT_NTP_TICK,   tick_usec);
> > +             }
>
> Yes, that looks much more palatable! Nice work!
>
> With the above addressed:
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
>
>

Thanks!

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-04-10  7:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-09 12:31 [PATCH ghak10 v7 0/2] audit: Log changes that can affect the system clock Ondrej Mosnacek
2019-04-09 12:31 ` [PATCH ghak10 v7 1/2] timekeeping: Audit clock adjustments Ondrej Mosnacek
2019-04-09 14:26   ` Richard Guy Briggs
2019-04-09 15:05     ` Ondrej Mosnacek
2019-04-10  6:25   ` Thomas Gleixner
2019-04-09 12:31 ` [PATCH ghak10 v7 2/2] ntp: Audit NTP parameters adjustment Ondrej Mosnacek
2019-04-09 14:40   ` Richard Guy Briggs
2019-04-09 15:10     ` Ondrej Mosnacek
2019-04-10  6:24   ` Thomas Gleixner
2019-04-10  7:29     ` Ondrej Mosnacek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).