[PATCH v3] audit: log AUDIT_TIME_* records only from rules
Richard Guy Briggs
rgb at redhat.com
Mon Jan 31 23:29:40 UTC 2022
On 2022-01-31 17:02, Paul Moore wrote:
> On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb at redhat.com> wrote:
> > On 2022-01-25 22:24, Richard Guy Briggs 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
> > > v3
> > > - move tk into union
> > > - rename audit_log_ntp() to audit_log_time() and add tk
> > > - key off both AUDIT_TIME_* but favour NTP
> > >
> > > kernel/audit.h | 4 +++
> > > kernel/auditsc.c | 86 +++++++++++++++++++++++++++++++++++++-----------
> > > 2 files changed, 70 insertions(+), 20 deletions(-)
>
> ...
>
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index b517947bfa48..9c6c55a81fdb 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1331,6 +1331,55 @@ 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)
> > > +{
> > > + 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",
> > > + };
> > > + int type, first = 1;
> > > +
> > > + if (context->type == AUDIT_TIME_INJOFFSET)
> > > + goto tk;
> > > +
> > > + /* use up allocated ab from show_special before new one */
> > > + for (type = 0; type < AUDIT_NTP_NVALS; type++) {
> > > + if (ntp->vals[type].newval != ntp->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], ntp->vals[type].oldval,
> > > + ntp->vals[type].newval);
> > > + }
> > > + }
> > > +
> > > +tk:
> > > + if (tk->tv_sec != 0 || tk->tv_nsec != 0) {
> > > + if (!first) {
> > > + audit_log_end(*ab);
> > > + *ab = audit_log_start(context, GFP_KERNEL,
> > > + AUDIT_TIME_INJOFFSET);
> > > + if (!*ab)
> > > + return;
> > > + }
> >
> > It occurs to me that a slight improvement could be made to move the
> > "tk:" label here. And better yet, change the tk condition above to:
> >
> > if (!tk->tv_sec && !tk->tv_nsec)
> > return;
>
> Honestly, I've looked at this a few times over different days trying
> to make sure I'm not missing something, but there is a lot of
> complexity in audit_log_time() that I don't believe is necessary.
> What about something like below?
>
> [WARNING: not compiled, tested, yadda yadda]
>
> void audit_log_time(struct audit_context ctx, struct audit_buffer **abp)
> {
> int i;
> int type = ctx->type;
> struct audit_buffer *ab = *abp;
> struct audit_ntp_val *ntp;
> const struct timespec64 *tk;
> const char *ntp_name[] = {
> "offset",
> "freq",
> "status",
> "tai",
> "tick",
> "adjust",
> };
>
> do {
> if (type == AUDIT_TIME_ADJNTPVAL) {
> ntp = ctx->time.ntp_data.val;
> for (i = 0; i < AUDIT_NTP_NVALS; i++) {
> if (ntp[i].newval != ntp[i].oldval) {
> audit_log_format(ab,
> "op=%s old=%lli new=%lli",
> ntp_name[type],
> ntp[i].oldval, ntp[i].newval);
> }
> }
> } else {
> tk = &ctx->time.tk_injoffset;
> audit_log_format(ab, "sec=%lli nsec=%li",
> (long long)tk->tv_sec, tk->tv_nsec);
> }
> audit_log_end(ab);
There is an audit_log_end() in the calling function, show_special(), so
it should only be called here if there is another buffer allocated in
this function after it. audit_log_end() is protected should it be
called with no valid buffer so this wouldn't create a bug.
> if (*abp) {
> *abp = NULL;
> type = (type == AUDIT_TIME_ADJNTPVAL ?
> AUDIT_TIME_INJOFFSET : AUDIT_TIME_ADJNTPVAL);
This cannot be allocated if there are no more needed above, for either
ntp or tk, and at this point we don't know for ntp and would need to add
a check for tk. This is all part of the logic in the patch.
The main challenge is that one buffer is allocated by show_special and
one is wrapped up, expecting the content of the switch statement that
matches the record type to consume it. If more are needed, the previous
needs to be wrapped up and a new one of the correct type allocated.
In this case, since the NTP type is the dominant, we don't know if a new
one is needed based solely on the context->type, so we can't use that
when allocating the new buffer.
> ab = audit_log_start(ctx, GFP_KERNEL, type);
> } else
> ab = NULL;
> } while (ab);
> }
>
> > > + audit_log_format(*ab, "sec=%lli nsec=%li",
> > > + (long long)tk->tv_sec, tk->tv_nsec);
> > > + }
> > > +}
>
> ...
>
> > > 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->time.ntp_data, ad, sizeof(*ad));
> > > + break;
>
> Not something I would normally worry about, but since you're probably
> going to respin this anyway you might as well change the 'break;' to
> 'return;'.
Sure. I can also make the mods I'd suggested in my own previous reply
to this patch.
> > > + }
>
> 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
More information about the Linux-audit
mailing list