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