On 2022-01-31 17:02, Paul Moore wrote: > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <r...@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 <r...@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 <r...@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