[RFC PATCH ghak10 v2 0/5] audit: Log modifying adjtimex(2) calls

[RFC PATCH ghak10 v2 0/5] audit: Log modifying adjtimex(2) calls

[Ondrej Mosnacek]

This patchset adds a new AUDIT_TIME_ADJUSTED record type, which is 
associated to the adjtimex(2) syscall record whenever it is invoked in 
non-read-only mode.

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.

[RFC PATCH ghak10 v2 1/4] audit: Add AUDIT_TIME_ADJUSTED record type

[Ondrej Mosnacek]

This auxiliary record type will be used to annotate the adjtimex SYSCALL
records with the information that the clock has been adjusted. This
record shall be emitted only when the clock is modified (including
changes that have no effect, e.g. adjust by zero offset, etc.).

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/uapi/linux/audit.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index c35aee9ad4a6..d5aa26b02834 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_ADJUSTED	1332	/* Clock adjustment event (aux for SYSCALL) */
 
 #define AUDIT_AVC		1400	/* SE Linux avc denial or grant */
 #define AUDIT_SELINUX_ERR	1401	/* Internal SE Linux Errors */
-- 
2.17.1

[RFC PATCH ghak10 v2 2/4] audit: Add the audit_adjtime() function

[Ondrej Mosnacek]

This patch adds a new function that shall be used to log any
modification of the system's clock (via the adjtimex() syscall).

The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
following fields:
* txmodes  (the 'modes' field of struct timex; this is the only
  mandatory field)
* txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
  of struct timex (respectively))
* txoffset (the 'offset' field of struct timex)
* txfreq   (the 'freq' field of struct timex)
* txmaxerr (the 'maxerror' field of struct timex)
* txesterr (the 'esterror' field of struct timex)
* txstatus (the 'status' field of struct timex)
* txconst  (the 'constant' field of struct timex)
* txtick   (the 'tick' field of struct timex)

These fields allow to reconstruct what exactly was changed by the
adjtimex syscall and to what value(s). Note that the values reported are
taken directly from the structure as received from userspace. The
syscall handling code may do some clamping of the values internally
before actually changing the kernel variables. Also, the fact that this
record has been logged does not necessarily mean that some variable was
changed (it may have been set to the same value as the old value).

Quick overview of the 'struct timex' semantics:
  The 'modes' field is a bitmask specifying which time variables (if
  any) should be adjusted. The other fields (of those listed above)
  contain the values of the respective variables that should be set. If
  the corresponding bit is not set in the 'modes' field, then a field's
  value is not significant (it may be some garbage value passed from
  userspace).

  Note that after processing the input values from userspace, the
  handler writes (all) the current (new) internal values into the struct
  and hands it back to userspace. These values are not logged.

  Also note that 'txusec' may actually mean nanoseconds, not
  microseconds, depending on whether ADJ_NSEC is set in 'modes'.

Possible improvements:
* move the conditional for logging/not logging into audit_adjtime()
  - (see also next patch in series)
  - may not be desirable due to reasons above
  - we should probably either do both this and above or neither
* log also the old values + the actual values that get set
  - this would be rather difficult to do, probably not worth it

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 include/linux/audit.h | 11 +++++++++++
 kernel/auditsc.c      | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+)

diff --git a/include/linux/audit.h b/include/linux/audit.h
index 69c78477590b..26e9db46293c 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -26,6 +26,7 @@
 #include <linux/sched.h>
 #include <linux/ptrace.h>
 #include <uapi/linux/audit.h>
+#include <uapi/linux/timex.h>
 
 #define AUDIT_INO_UNSET ((unsigned long)-1)
 #define AUDIT_DEV_UNSET ((dev_t)-1)
@@ -353,6 +354,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_adjtime(const struct timex *txc);
 
 static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
 {
@@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int response)
 		__audit_fanotify(response);
 }
 
+static inline void audit_adjtime(const struct timex *txc)
+{
+	if (!audit_dummy_context())
+		__audit_adjtime(txc);
+}
+
 extern int audit_n_rules;
 extern int audit_signals;
 #else /* CONFIG_AUDITSYSCALL */
@@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
 static inline void audit_fanotify(unsigned int response)
 { }
 
+static inline void audit_adjtime(const struct timex *txc)
+{ }
+
 static inline void audit_ptrace(struct task_struct *t)
 { }
 #define audit_n_rules 0
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index ceb1c4596c51..9a1f3fcc377a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2422,6 +2422,44 @@ void __audit_fanotify(unsigned int response)
 		AUDIT_FANOTIFY,	"resp=%u", response);
 }
 
+void __audit_adjtime(const struct timex *txc)
+{
+	struct audit_buffer *ab;
+
+	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED);
+	if (unlikely(!ab))
+		return;
+
+	audit_log_format(ab, "txmodes=%u", txc->modes);
+
+	if (txc->modes & ADJ_SETOFFSET)
+		audit_log_format(ab, " txsec=%li txusec=%li",
+			(long)txc->time.tv_sec, (long)txc->time.tv_usec);
+
+	if (txc->modes & ADJ_OFFSET)
+		audit_log_format(ab, " txoffset=%li", txc->offset);
+
+	if (txc->modes & ADJ_FREQUENCY)
+		audit_log_format(ab, " txfreq=%li", txc->freq);
+
+	if (txc->modes & ADJ_MAXERROR)
+		audit_log_format(ab, " txmaxerr=%li", txc->maxerror);
+
+	if (txc->modes & ADJ_ESTERROR)
+		audit_log_format(ab, " txesterr=%li", txc->esterror);
+
+	if (txc->modes & ADJ_STATUS)
+		audit_log_format(ab, " txstatus=%li", txc->status);
+
+	if (txc->modes & (ADJ_TIMECONST | ADJ_TAI))
+		audit_log_format(ab, " txconst=%li", txc->constant);
+
+	if (txc->modes & ADJ_TICK)
+		audit_log_format(ab, " txtick=%li", txc->tick);
+
+	audit_log_end(ab);
+}
+
 static void audit_log_task(struct audit_buffer *ab)
 {
 	kuid_t auid, uid;
-- 
2.17.1

Re: [RFC PATCH ghak10 v2 2/4] audit: Add the audit_adjtime() function

[Richard Guy Briggs]

On 2018-06-19 15:58, Ondrej Mosnacek wrote:
> This patch adds a new function that shall be used to log any
> modification of the system's clock (via the adjtimex() syscall).
> 
> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> following fields:
> * txmodes  (the 'modes' field of struct timex; this is the only
>   mandatory field)
> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
>   of struct timex (respectively))
> * txoffset (the 'offset' field of struct timex)
> * txfreq   (the 'freq' field of struct timex)
> * txmaxerr (the 'maxerror' field of struct timex)
> * txesterr (the 'esterror' field of struct timex)
> * txstatus (the 'status' field of struct timex)
> * txconst  (the 'constant' field of struct timex)
> * txtick   (the 'tick' field of struct timex)

I see below that you conditionally log these fields if the flag is
present.  There has been concern of fields coming and going
(disappearing) so this could cause problems for parsers looking for
specific fields that aren't present.  The first version of the patch
listed them all, which may be overkill.  As has been suggested, limiting
it to security-affecting fields would be a first step.

Another possibility would be to list one changed field per record.  This
could work if multiple flags are infrequent in one call.  A format I'd
suggest might be txfield=mode old=<value> new=<value>.  (Note only the
single flag value rather than all modes flags.)  There would be more
overhead if there was more than one field present in a call, but would
address the disappearing fields issue.

> These fields allow to reconstruct what exactly was changed by the
> adjtimex syscall and to what value(s). Note that the values reported are
> taken directly from the structure as received from userspace. The
> syscall handling code may do some clamping of the values internally
> before actually changing the kernel variables. Also, the fact that this
> record has been logged does not necessarily mean that some variable was
> changed (it may have been set to the same value as the old value).

If the value is the same, that may still be relevant for reporting that
someone tried to set/change it.

> Quick overview of the 'struct timex' semantics:
>   The 'modes' field is a bitmask specifying which time variables (if
>   any) should be adjusted. The other fields (of those listed above)
>   contain the values of the respective variables that should be set. If
>   the corresponding bit is not set in the 'modes' field, then a field's
>   value is not significant (it may be some garbage value passed from
>   userspace).
> 
>   Note that after processing the input values from userspace, the
>   handler writes (all) the current (new) internal values into the struct
>   and hands it back to userspace. These values are not logged.
> 
>   Also note that 'txusec' may actually mean nanoseconds, not
>   microseconds, depending on whether ADJ_NSEC is set in 'modes'.
> 
> Possible improvements:
> * move the conditional for logging/not logging into audit_adjtime()
>   - (see also next patch in series)
>   - may not be desirable due to reasons above
>   - we should probably either do both this and above or neither
> * log also the old values + the actual values that get set
>   - this would be rather difficult to do, probably not worth it
> 
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  include/linux/audit.h | 11 +++++++++++
>  kernel/auditsc.c      | 38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 49 insertions(+)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 69c78477590b..26e9db46293c 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -26,6 +26,7 @@
>  #include <linux/sched.h>
>  #include <linux/ptrace.h>
>  #include <uapi/linux/audit.h>
> +#include <uapi/linux/timex.h>
>  
>  #define AUDIT_INO_UNSET ((unsigned long)-1)
>  #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -353,6 +354,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_adjtime(const struct timex *txc);
>  
>  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
>  {
> @@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int response)
>  		__audit_fanotify(response);
>  }
>  
> +static inline void audit_adjtime(const struct timex *txc)
> +{
> +	if (!audit_dummy_context())
> +		__audit_adjtime(txc);
> +}
> +
>  extern int audit_n_rules;
>  extern int audit_signals;
>  #else /* CONFIG_AUDITSYSCALL */
> @@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
>  static inline void audit_fanotify(unsigned int response)
>  { }
>  
> +static inline void audit_adjtime(const struct timex *txc)
> +{ }
> +
>  static inline void audit_ptrace(struct task_struct *t)
>  { }
>  #define audit_n_rules 0
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index ceb1c4596c51..9a1f3fcc377a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2422,6 +2422,44 @@ void __audit_fanotify(unsigned int response)
>  		AUDIT_FANOTIFY,	"resp=%u", response);
>  }
>  
> +void __audit_adjtime(const struct timex *txc)
> +{
> +	struct audit_buffer *ab;
> +
> +	ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED);
> +	if (unlikely(!ab))
> +		return;
> +
> +	audit_log_format(ab, "txmodes=%u", txc->modes);
> +
> +	if (txc->modes & ADJ_SETOFFSET)
> +		audit_log_format(ab, " txsec=%li txusec=%li",
> +			(long)txc->time.tv_sec, (long)txc->time.tv_usec);
> +
> +	if (txc->modes & ADJ_OFFSET)
> +		audit_log_format(ab, " txoffset=%li", txc->offset);
> +
> +	if (txc->modes & ADJ_FREQUENCY)
> +		audit_log_format(ab, " txfreq=%li", txc->freq);
> +
> +	if (txc->modes & ADJ_MAXERROR)
> +		audit_log_format(ab, " txmaxerr=%li", txc->maxerror);
> +
> +	if (txc->modes & ADJ_ESTERROR)
> +		audit_log_format(ab, " txesterr=%li", txc->esterror);
> +
> +	if (txc->modes & ADJ_STATUS)
> +		audit_log_format(ab, " txstatus=%li", txc->status);
> +
> +	if (txc->modes & (ADJ_TIMECONST | ADJ_TAI))
> +		audit_log_format(ab, " txconst=%li", txc->constant);
> +
> +	if (txc->modes & ADJ_TICK)
> +		audit_log_format(ab, " txtick=%li", txc->tick);
> +
> +	audit_log_end(ab);
> +}
> +
>  static void audit_log_task(struct audit_buffer *ab)
>  {
>  	kuid_t auid, uid;
> -- 
> 2.17.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 v2 2/4] audit: Add the audit_adjtime() function

[Ondrej Mosnacek]

ut 19. 6. 2018 o 17:04 Richard Guy Briggs <rgb@redhat.com> napísal(a):
>
> On 2018-06-19 15:58, Ondrej Mosnacek wrote:
> > This patch adds a new function that shall be used to log any
> > modification of the system's clock (via the adjtimex() syscall).
> >
> > The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> > following fields:
> > * txmodes  (the 'modes' field of struct timex; this is the only
> >   mandatory field)
> > * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
> >   of struct timex (respectively))
> > * txoffset (the 'offset' field of struct timex)
> > * txfreq   (the 'freq' field of struct timex)
> > * txmaxerr (the 'maxerror' field of struct timex)
> > * txesterr (the 'esterror' field of struct timex)
> > * txstatus (the 'status' field of struct timex)
> > * txconst  (the 'constant' field of struct timex)
> > * txtick   (the 'tick' field of struct timex)
>
> I see below that you conditionally log these fields if the flag is
> present.  There has been concern of fields coming and going
> (disappearing) so this could cause problems for parsers looking for
> specific fields that aren't present.  The first version of the patch
> listed them all, which may be overkill.  As has been suggested, limiting
> it to security-affecting fields would be a first step.

I assumed that it was normal for records to have some fields that are
optional (i.e. it is known and documented that they may be missing).
But I understand that it is quite inconvenient to handle such fields
and the desire to have all fields always there, even if with some
special 'empty' values...

> Another possibility would be to list one changed field per record.  This
> could work if multiple flags are infrequent in one call.  A format I'd
> suggest might be txfield=mode old=<value> new=<value>.  (Note only the
> single flag value rather than all modes flags.)  There would be more
> overhead if there was more than one field present in a call, but would
> address the disappearing fields issue.

Indeed this would be nicer, although more verbose... Also, detecting
old and new values would require a bit more intrusive code changes,
but it should be doable.

>
> > These fields allow to reconstruct what exactly was changed by the
> > adjtimex syscall and to what value(s). Note that the values reported are
> > taken directly from the structure as received from userspace. The
> > syscall handling code may do some clamping of the values internally
> > before actually changing the kernel variables. Also, the fact that this
> > record has been logged does not necessarily mean that some variable was
> > changed (it may have been set to the same value as the old value).
>
> If the value is the same, that may still be relevant for reporting that
> someone tried to set/change it.
>
> > Quick overview of the 'struct timex' semantics:
> >   The 'modes' field is a bitmask specifying which time variables (if
> >   any) should be adjusted. The other fields (of those listed above)
> >   contain the values of the respective variables that should be set. If
> >   the corresponding bit is not set in the 'modes' field, then a field's
> >   value is not significant (it may be some garbage value passed from
> >   userspace).
> >
> >   Note that after processing the input values from userspace, the
> >   handler writes (all) the current (new) internal values into the struct
> >   and hands it back to userspace. These values are not logged.
> >
> >   Also note that 'txusec' may actually mean nanoseconds, not
> >   microseconds, depending on whether ADJ_NSEC is set in 'modes'.
> >
> > Possible improvements:
> > * move the conditional for logging/not logging into audit_adjtime()
> >   - (see also next patch in series)
> >   - may not be desirable due to reasons above
> >   - we should probably either do both this and above or neither
> > * log also the old values + the actual values that get set
> >   - this would be rather difficult to do, probably not worth it
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  include/linux/audit.h | 11 +++++++++++
> >  kernel/auditsc.c      | 38 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 49 insertions(+)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 69c78477590b..26e9db46293c 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/sched.h>
> >  #include <linux/ptrace.h>
> >  #include <uapi/linux/audit.h>
> > +#include <uapi/linux/timex.h>
> >
> >  #define AUDIT_INO_UNSET ((unsigned long)-1)
> >  #define AUDIT_DEV_UNSET ((dev_t)-1)
> > @@ -353,6 +354,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_adjtime(const struct timex *txc);
> >
> >  static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp)
> >  {
> > @@ -455,6 +457,12 @@ static inline void audit_fanotify(unsigned int response)
> >               __audit_fanotify(response);
> >  }
> >
> > +static inline void audit_adjtime(const struct timex *txc)
> > +{
> > +     if (!audit_dummy_context())
> > +             __audit_adjtime(txc);
> > +}
> > +
> >  extern int audit_n_rules;
> >  extern int audit_signals;
> >  #else /* CONFIG_AUDITSYSCALL */
> > @@ -581,6 +589,9 @@ static inline void audit_log_kern_module(char *name)
> >  static inline void audit_fanotify(unsigned int response)
> >  { }
> >
> > +static inline void audit_adjtime(const struct timex *txc)
> > +{ }
> > +
> >  static inline void audit_ptrace(struct task_struct *t)
> >  { }
> >  #define audit_n_rules 0
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index ceb1c4596c51..9a1f3fcc377a 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2422,6 +2422,44 @@ void __audit_fanotify(unsigned int response)
> >               AUDIT_FANOTIFY, "resp=%u", response);
> >  }
> >
> > +void __audit_adjtime(const struct timex *txc)
> > +{
> > +     struct audit_buffer *ab;
> > +
> > +     ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJUSTED);
> > +     if (unlikely(!ab))
> > +             return;
> > +
> > +     audit_log_format(ab, "txmodes=%u", txc->modes);
> > +
> > +     if (txc->modes & ADJ_SETOFFSET)
> > +             audit_log_format(ab, " txsec=%li txusec=%li",
> > +                     (long)txc->time.tv_sec, (long)txc->time.tv_usec);
> > +
> > +     if (txc->modes & ADJ_OFFSET)
> > +             audit_log_format(ab, " txoffset=%li", txc->offset);
> > +
> > +     if (txc->modes & ADJ_FREQUENCY)
> > +             audit_log_format(ab, " txfreq=%li", txc->freq);
> > +
> > +     if (txc->modes & ADJ_MAXERROR)
> > +             audit_log_format(ab, " txmaxerr=%li", txc->maxerror);
> > +
> > +     if (txc->modes & ADJ_ESTERROR)
> > +             audit_log_format(ab, " txesterr=%li", txc->esterror);
> > +
> > +     if (txc->modes & ADJ_STATUS)
> > +             audit_log_format(ab, " txstatus=%li", txc->status);
> > +
> > +     if (txc->modes & (ADJ_TIMECONST | ADJ_TAI))
> > +             audit_log_format(ab, " txconst=%li", txc->constant);
> > +
> > +     if (txc->modes & ADJ_TICK)
> > +             audit_log_format(ab, " txtick=%li", txc->tick);
> > +
> > +     audit_log_end(ab);
> > +}
> > +
> >  static void audit_log_task(struct audit_buffer *ab)
> >  {
> >       kuid_t auid, uid;
> > --
> > 2.17.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.

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

[RFC PATCH ghak10 v2 3/4] timekeeping: Audit attempts to adjust the clock

[Ondrej Mosnacek]

(Intentionally not sending to the timekeeping maintainers just yet,
let's settle on the record contents/format first.)

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 kernel/time/timekeeping.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 4786df904c22..02097a7d225e 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -25,6 +25,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"
@@ -2315,6 +2316,16 @@ int do_adjtimex(struct timex *txc)
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 
+	/* Only log audit event if the clock was changed/attempted to be changed.
+	 * Based on the logic inside timekeeping_validate_timex().
+	 * NOTE: We need to log the event before any of the fields get
+	 * overwritten by the output values (__do_adjtimex() does not fail, so
+	 * it is OK to do it here). */
+	if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
+	    || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
+	    ||   (txc->modes & ADJ_SETOFFSET))
+		audit_adjtime(txc);
+
 	orig_tai = tai = tk->tai_offset;
 	ret = __do_adjtimex(txc, &ts, &tai);
 
-- 
2.17.1

[RFC PATCH ghak10 v2 4/4] [OPTIONAL] Determine read-only in the validation function

[Ondrej Mosnacek]

This is an optional patch that modifies the previous one to determine
whether the operation is read-only in the timekeeping_validate_timex()
function instead of duplicating the complex condition.

A minor issue with this patch is that the validation function now
sometimes returns -EINVAL where it would before return -EPERM in the
cases where both errors are applicable. This may not be desired.

Please ACK or NACK, I'm not sure which variant is better...
---
 kernel/time/timekeeping.c | 40 ++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 02097a7d225e..dad11205a86d 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -2220,20 +2220,25 @@ ktime_t ktime_get_update_offsets_now(unsigned int *cwsseq, ktime_t *offs_real,
 
 /**
  * timekeeping_validate_timex - Ensures the timex is ok for use in do_adjtimex
+ *
+ * Return value:
+ *   1 - OK, syscall is read-only
+ *   0 - OK, syscall modifies some value
+ *   negative - NOT OK (-EINVAL or -EPERM)
  */
 static int timekeeping_validate_timex(struct timex *txc)
 {
+	int readonly = 1;
+
 	if (txc->modes & ADJ_ADJTIME) {
 		/* singleshot must not be used with any other mode bits */
 		if (!(txc->modes & ADJ_OFFSET_SINGLESHOT))
 			return -EINVAL;
-		if (!(txc->modes & ADJ_OFFSET_READONLY) &&
-		    !capable(CAP_SYS_TIME))
-			return -EPERM;
+		if (!(txc->modes & ADJ_OFFSET_READONLY))
+			readonly = 0;
 	} else {
-		/* In order to modify anything, you gotta be super-user! */
-		if (txc->modes && !capable(CAP_SYS_TIME))
-			return -EPERM;
+		if (txc->modes)
+			readonly = 0;
 		/*
 		 * if the quartz is off by more than 10% then
 		 * something is VERY wrong!
@@ -2245,9 +2250,7 @@ static int timekeeping_validate_timex(struct timex *txc)
 	}
 
 	if (txc->modes & ADJ_SETOFFSET) {
-		/* In order to inject time, you gotta be super-user! */
-		if (!capable(CAP_SYS_TIME))
-			return -EPERM;
+		readonly = 0;
 
 		/*
 		 * Validate if a timespec/timeval used to inject a time
@@ -2269,6 +2272,10 @@ static int timekeeping_validate_timex(struct timex *txc)
 		}
 	}
 
+	/* In order to modify anything, you gotta be super-user! */
+	if (!readonly && !capable(CAP_SYS_TIME))
+		return -EPERM;
+
 	/*
 	 * Check for potential multiplication overflows that can
 	 * only happen on 64-bit systems:
@@ -2280,7 +2287,7 @@ static int timekeeping_validate_timex(struct timex *txc)
 			return -EINVAL;
 	}
 
-	return 0;
+	return readonly;
 }
 
 
@@ -2293,13 +2300,15 @@ int do_adjtimex(struct timex *txc)
 	unsigned long flags;
 	struct timespec64 ts;
 	s32 orig_tai, tai;
-	int ret;
+	int ret, readonly;
 
 	/* Validate the data before disabling interrupts */
 	ret = timekeeping_validate_timex(txc);
-	if (ret)
+	if (ret < 0)
 		return ret;
 
+	readonly = ret;
+
 	if (txc->modes & ADJ_SETOFFSET) {
 		struct timespec64 delta;
 		delta.tv_sec  = txc->time.tv_sec;
@@ -2316,14 +2325,11 @@ int do_adjtimex(struct timex *txc)
 	raw_spin_lock_irqsave(&timekeeper_lock, flags);
 	write_seqcount_begin(&tk_core.seq);
 
-	/* Only log audit event if the clock was changed/attempted to be changed.
-	 * Based on the logic inside timekeeping_validate_timex().
+	/* Only log audit event if the clock is being changed.
 	 * NOTE: We need to log the event before any of the fields get
 	 * overwritten by the output values (__do_adjtimex() does not fail, so
 	 * it is OK to do it here). */
-	if (   ( (txc->modes & ADJ_ADJTIME) && !(txc->modes & ADJ_OFFSET_READONLY))
-	    || (!(txc->modes & ADJ_ADJTIME) &&   txc->modes)
-	    ||   (txc->modes & ADJ_SETOFFSET))
+	if (!readonly)
 		audit_adjtime(txc);
 
 	orig_tai = tai = tk->tai_offset;
-- 
2.17.1