[PATCH v2] audit: log AUDIT_TIME_* records only from rules

[PATCH v2] audit: log AUDIT_TIME_* records only from rules

[Richard Guy Briggs]

AUDIT_TIME_* events are generated when there are syscall rules present that are
not related to time keeping.  This will produce noisy log entries that could
flood the logs and hide events we really care about.

Rather than immediately produce the AUDIT_TIME_* records, store the data in the
context and log it at syscall exit time respecting the filter rules.

Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919

Fixes: 7e8eda734d30 ("ntp: Audit NTP parameters adjustment")
Fixes: 2d87a0674bd6 ("timekeeping: Audit clock adjustments")
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Changelog:
v2:
- rename __audit_ntp_log_ to audit_log_ntp
- pre-check ntp before storing
- move tk out of the context union and move ntp logging to the bottom of audit_show_special()
- restructure logging of ntp to use ab and allocate more only if more
- add Fixes lines

 kernel/audit.h   |  2 ++
 kernel/auditsc.c | 77 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 59 insertions(+), 20 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index c4498090a5bd..11789249d838 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -201,8 +201,10 @@ struct audit_context {
 		struct {
 			char			*name;
 		} module;
+		struct audit_ntp_data		ntp_data;
 	};
 	int fds[2];
+	struct timespec64			tk_injoffset;
 	struct audit_proctitle proctitle;
 };
 
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index b517947bfa48..1838a2b3ab10 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1331,6 +1331,38 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
 			 from_kuid(&init_user_ns, name->fcap.rootid));
 }
 
+void audit_log_ntp(struct audit_context *context, struct audit_buffer **ab,
+		   const struct audit_ntp_data *ad)
+{
+	const char *ntp_name[] = {
+		"offset",
+		"freq",
+		"status",
+		"tai",
+		"tick",
+		"adjust",
+	};
+	int type, first = 1;
+
+	/* use up allocated ab from show_special before new one */
+	for (type = 0; type < AUDIT_NTP_NVALS; type++) {
+		if (ad->vals[type].newval != ad->vals[type].oldval) {
+			if (first) {
+				first = 0;
+			} else {
+				audit_log_end(*ab);
+				*ab = audit_log_start(context, GFP_KERNEL,
+						      AUDIT_TIME_ADJNTPVAL);
+				if (!*ab)
+					return;
+			}
+			audit_log_format(*ab, "op=%s old=%lli new=%lli",
+					 ntp_name[type], ad->vals[type].oldval,
+					 ad->vals[type].newval);
+		}
+	}
+}
+
 static void show_special(struct audit_context *context, int *call_panic)
 {
 	struct audit_buffer *ab;
@@ -1445,6 +1477,9 @@ static void show_special(struct audit_context *context, int *call_panic)
 			audit_log_format(ab, "(null)");
 
 		break;
+	case AUDIT_TIME_ADJNTPVAL:
+		audit_log_ntp(context, &ab, &context->ntp_data);
+		break;
 	}
 	audit_log_end(ab);
 }
@@ -1767,6 +1802,17 @@ static void audit_log_exit(void)
 		audit_log_name(context, n, NULL, i++, &call_panic);
 	}
 
+	if (context->tk_injoffset.tv_sec != 0 ||
+	    context->tk_injoffset.tv_nsec != 0) {
+		ab = audit_log_start(context, GFP_KERNEL, AUDIT_TIME_INJOFFSET);
+		if (ab) {
+			audit_log_format(ab, "sec=%lli nsec=%li",
+					 (long long)context->tk_injoffset.tv_sec,
+					 context->tk_injoffset.tv_nsec);
+			audit_log_end(ab);
+		}
+	}
+
 	if (context->context == AUDIT_CTX_SYSCALL)
 		audit_log_proctitle();
 
@@ -2840,31 +2886,22 @@ void __audit_fanotify(unsigned int 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_ntp_val(const struct audit_ntp_data *ad,
-			      const char *op, enum audit_ntp_type type)
-{
-	const struct audit_ntp_val *val = &ad->vals[type];
-
-	if (val->newval == val->oldval)
-		return;
+	struct audit_context *context = audit_context();
 
-	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
-		  "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
+	memcpy(&context->tk_injoffset, &offset, sizeof(offset));
 }
 
 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);
+	struct audit_context *context = audit_context();
+	int type;
+
+	for (type = 0; type < AUDIT_NTP_NVALS; type++)
+		if (ad->vals[type].newval != ad->vals[type].oldval) {
+			context->type = AUDIT_TIME_ADJNTPVAL;
+			memcpy(&context->ntp_data, ad, sizeof(*ad));
+			break;
+		}
 }
 
 void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
-- 
2.27.0

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

Re: [PATCH v2] audit: log AUDIT_TIME_* records only from rules

[Paul Moore]

On Fri, Jan 21, 2022 at 11:17 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> AUDIT_TIME_* events are generated when there are syscall rules present that are
> not related to time keeping.  This will produce noisy log entries that could
> flood the logs and hide events we really care about.
>
> Rather than immediately produce the AUDIT_TIME_* records, store the data in the
> context and log it at syscall exit time respecting the filter rules.
>
> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919

I haven't made sense of the rest of the patch yet, but *please* do not
include non-public sources of information in patch descriptions.

(... and please don't respin this patch just for that, just keep it in
mind for future work.)

> Fixes: 7e8eda734d30 ("ntp: Audit NTP parameters adjustment")
> Fixes: 2d87a0674bd6 ("timekeeping: Audit clock adjustments")
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Changelog:
> v2:
> - rename __audit_ntp_log_ to audit_log_ntp
> - pre-check ntp before storing
> - move tk out of the context union and move ntp logging to the bottom of audit_show_special()
> - restructure logging of ntp to use ab and allocate more only if more
> - add Fixes lines
>
>  kernel/audit.h   |  2 ++
>  kernel/auditsc.c | 77 +++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 59 insertions(+), 20 deletions(-)

-- 
paul moore
paul-moore.com

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

Re: [PATCH v2] audit: log AUDIT_TIME_* records only from rules

[Paul Moore]

On Fri, Jan 21, 2022 at 11:17 AM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> AUDIT_TIME_* events are generated when there are syscall rules present that are
> not related to time keeping.  This will produce noisy log entries that could
> flood the logs and hide events we really care about.
>
> Rather than immediately produce the AUDIT_TIME_* records, store the data in the
> context and log it at syscall exit time respecting the filter rules.
>
> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
>
> Fixes: 7e8eda734d30 ("ntp: Audit NTP parameters adjustment")
> Fixes: 2d87a0674bd6 ("timekeeping: Audit clock adjustments")
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Changelog:
> v2:
> - rename __audit_ntp_log_ to audit_log_ntp
> - pre-check ntp before storing
> - move tk out of the context union and move ntp logging to the bottom of audit_show_special()
> - restructure logging of ntp to use ab and allocate more only if more
> - add Fixes lines
>
>  kernel/audit.h   |  2 ++
>  kernel/auditsc.c | 77 +++++++++++++++++++++++++++++++++++-------------
>  2 files changed, 59 insertions(+), 20 deletions(-)
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index c4498090a5bd..11789249d838 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -201,8 +201,10 @@ struct audit_context {
>                 struct {
>                         char                    *name;
>                 } module;
> +               struct audit_ntp_data           ntp_data;
>         };
>         int fds[2];
> +       struct timespec64                       tk_injoffset;
>         struct audit_proctitle proctitle;
>  };

Why do we need to keep tk_injoffset outside of the audit_context
union?  I think we could do something like this, which would be an
improvement IMO:

  struct audit_context {
    /* ... */
    union {
      /* ... */
      struct {
        struct audit_ntp_data ntp_data;
        struct timespec64 tk_injoffset;
      } time;
    };
    /* ... */
  }

  void __audit_tk_injoffset(offset)
  {
    struct audit_context *ctx = audit_context();
    memcpy(&ctx->time->tk_injoffset, offset, sizeof(offset));
  }

  void audit_log_time(ctx)
  {
    /* ... */

    offset = ctx->time->tk_injoffset;
    if (offset->tv_sec != 0 || offset->tv_nsec != 0) {
      ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_TIME_INJOFFSET);
      /* ... */
      audit_log_end(ab);
    }

    ntp = ctx->time->ntp_data;
    for (i = 0; i < AUDIT_NTP_NVALS; i++) {
      if (ntp->vals[i].newval != ntp->vals[i].oldval ) {
        /* ... log the ntp/time param changes ... */
      }
    }

    /* ... */
  }

  void show_special(...)
  {
    /* ... */
    case AUDIT_TIME_INJOFFSET:
    case AUDIT_TIME_ADJNTPVAL:
      audit_log_time(context);
      break;
   /* ... */
  }

-- 
paul moore
paul-moore.com

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

Re: [PATCH v2] audit: log AUDIT_TIME_* records only from rules

[Richard Guy Briggs]

On 2022-01-24 22:11, Paul Moore wrote:
> On Fri, Jan 21, 2022 at 11:17 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > not related to time keeping.  This will produce noisy log entries that could
> > flood the logs and hide events we really care about.
> >
> > Rather than immediately produce the AUDIT_TIME_* records, store the data in the
> > context and log it at syscall exit time respecting the filter rules.
> >
> > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> >
> > Fixes: 7e8eda734d30 ("ntp: Audit NTP parameters adjustment")
> > Fixes: 2d87a0674bd6 ("timekeeping: Audit clock adjustments")
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Changelog:
> > v2:
> > - rename __audit_ntp_log_ to audit_log_ntp
> > - pre-check ntp before storing
> > - move tk out of the context union and move ntp logging to the bottom of audit_show_special()
> > - restructure logging of ntp to use ab and allocate more only if more
> > - add Fixes lines
> >
> >  kernel/audit.h   |  2 ++
> >  kernel/auditsc.c | 77 +++++++++++++++++++++++++++++++++++-------------
> >  2 files changed, 59 insertions(+), 20 deletions(-)
> >
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index c4498090a5bd..11789249d838 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -201,8 +201,10 @@ struct audit_context {
> >                 struct {
> >                         char                    *name;
> >                 } module;
> > +               struct audit_ntp_data           ntp_data;
> >         };
> >         int fds[2];
> > +       struct timespec64                       tk_injoffset;
> >         struct audit_proctitle proctitle;
> >  };
> 
> Why do we need to keep tk_injoffset outside of the audit_context
> union?  I think we could do something like this, which would be an
> improvement IMO:

I did three implementations.  The first put ntp_data outside which was
the quickest and simplest patch, but I really didn't like the wastage in
audit_context.  The second did as you suggest below in the union, but
having the type overwritten by the other AUDIT_TIME_* didn't sit well
with me, but since we have to check all the values anyways, we don't
really lose anything.  This means restructuring the loop a bit but that
should work.  Both types will need to be written to the context->type.

I'll code that up...

>   struct audit_context {
>     /* ... */
>     union {
>       /* ... */
>       struct {
>         struct audit_ntp_data ntp_data;
>         struct timespec64 tk_injoffset;
>       } time;
>     };
>     /* ... */
>   }
> 
>   void __audit_tk_injoffset(offset)
>   {
>     struct audit_context *ctx = audit_context();
>     memcpy(&ctx->time->tk_injoffset, offset, sizeof(offset));
>   }
> 
>   void audit_log_time(ctx)
>   {
>     /* ... */
> 
>     offset = ctx->time->tk_injoffset;
>     if (offset->tv_sec != 0 || offset->tv_nsec != 0) {
>       ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_TIME_INJOFFSET);
>       /* ... */
>       audit_log_end(ab);
>     }
> 
>     ntp = ctx->time->ntp_data;
>     for (i = 0; i < AUDIT_NTP_NVALS; i++) {
>       if (ntp->vals[i].newval != ntp->vals[i].oldval ) {
>         /* ... log the ntp/time param changes ... */
>       }
>     }
> 
>     /* ... */
>   }
> 
>   void show_special(...)
>   {
>     /* ... */
>     case AUDIT_TIME_INJOFFSET:
>     case AUDIT_TIME_ADJNTPVAL:
>       audit_log_time(context);
>       break;
>    /* ... */
>   }
> 
> -- 
> paul moore
> paul-moore.com
> 

- 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

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