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

From: Paul Moore <paul@paul-moore.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Eric Paris <eparis@parisplace.org>,
	Linux-Audit Mailing List <linux-audit@redhat.com>
Subject: Re: [PATCH v5] audit: log AUDIT_TIME_* records only from rules
Date: Thu, 17 Feb 2022 17:55:38 -0500	[thread overview]
Message-ID: <CAHC9VhQV4dUqjNcwAsZ9-pO6w+AFMhQsM=N79AdS1M3uq2Y3WA@mail.gmail.com> (raw)
In-Reply-To: <d97744aaeee1cce34f793dabf9ce035354d73250.1645043410.git.rgb@redhat.com>

On Wed, Feb 16, 2022 at 3:33 PM 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.
>
> Note: This eats the audit_buffer, unlike any others in show_special().
>
> 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
> v3
> - move tk into union
> - rename audit_log_ntp() to audit_log_time() and add tk
> - key off both AUDIT_TIME_* but favour NTP
> v4
> - drop tk goto in favour of ntp if clause
> - add comments to clarify calling function buffer expectations
> v5
> - hold my nose and swallow the audit_buffer in audit_log_time()
>
>  kernel/audit.h   |  4 +++
>  kernel/auditsc.c | 85 ++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 69 insertions(+), 20 deletions(-)

...

> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index fce5d43a933f..53d684966350 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1340,6 +1340,51 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>                          from_kuid(&init_user_ns, name->fcap.rootid));
>  }
>
> +void audit_log_time(struct audit_context *context, struct audit_buffer **ab)

It seems like audit_log_time() could be declared as a static function,
no?  We're only ever going to use it from show_special(), right?

> +{
> +       const struct audit_ntp_data *ntp = &context->time.ntp_data;
> +       const struct timespec64 *tk = &context->time.tk_injoffset;
> +       const char *ntp_name[] = {
> +               "offset",
> +               "freq",
> +               "status",
> +               "tai",
> +               "tick",
> +               "adjust",
> +       };

Similarly, should the ntp_name array be declared as a static const
array?  FWIW, a modern checkpatch would have flagged this.

> +       int type;
> +
> +       if (context->type == AUDIT_TIME_ADJNTPVAL) {
> +               for (type = 0; type < AUDIT_NTP_NVALS; type++) {
> +                       if (ntp->vals[type].newval != ntp->vals[type].oldval) {
> +                               if (!*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], ntp->vals[type].oldval,
> +                                                ntp->vals[type].newval);
> +                               audit_log_end(*ab);
> +                               *ab = NULL;
> +                       }
> +               }
> +       }
> +       if (tk->tv_sec != 0 || tk->tv_nsec != 0) {
> +               if (!*ab) {
> +                       *ab = audit_log_start(context, GFP_KERNEL,
> +                                             AUDIT_TIME_INJOFFSET);
> +                       if (!*ab)
> +                               return;
> +               }
> +               audit_log_format(*ab, "sec=%lli nsec=%li",
> +                                (long long)tk->tv_sec, tk->tv_nsec);
> +               audit_log_end(*ab);
> +               *ab = NULL;
> +       }
> +}

-- 
paul-moore.com

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