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

On 2021-12-29 17:51, Paul Moore wrote: > On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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 and >> log it at syscall exit time respecting the filter rules. >> >> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919 >> >> Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; >> --- >> Note: This is a quick and dirty proof-of-concept. If this approach of >> storing the values in the audit_context for later filtering is >> acceptable I'll clean up the patch (re-name functions) and re-submit. >> >> kernel/audit.h | 6 ++++++ >> kernel/auditsc.c | 29 +++++++++++++++++++++++++---- >> 2 files changed, 31 insertions(+), 4 deletions(-) > Reviewing this now with a more critical eye since it is longer just a > quick-n-dirty proof of concept ... > >> diff --git a/kernel/audit.h b/kernel/audit.h >> index 3b64a97f6091..25d63731b0e0 100644 >> --- a/kernel/audit.h >> +++ b/kernel/audit.h >> @@ -196,6 +196,12 @@ struct audit_context { >> struct { >> char *name; >> } module; >> + struct { >> + struct audit_ntp_data data; >> + } ntp; >> + struct { >> + struct timespec64 injoffset; >> + } tk; > With the ntp and tk structs being separate parts of a union, are we > going to have a problem when ADJ_SETOFFSET is set in a call to > do_adjtimex()? Yes, so since both record types can exist in one event, either both pieces of data will need to be in one struct within the union, or one piece of data will need to go outside of the union (preferably the smaller one to avoid bloating struct audit_context more than necessary). The tk data is smaller and is easier to check if it is set, so that might be better to store outside the union. Since show_special is keyed off record type, it makes more sense to store one of those datum outside the union and process it outside show_special to avoid record type tricks in show_special.

>> }; >> int fds[2]; >> struct audit_proctitle proctitle; >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c >> index 6efb0bb909d0..8983790ad86a 100644 >> --- a/kernel/auditsc.c >> +++ b/kernel/auditsc.c >> @@ -1210,11 +1210,18 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name) >> from_kuid(&init_user_ns, name->fcap.rootid)); >> } >> >> +void __audit_ntp_log_(const struct audit_ntp_data *ad); >> + >> static void show_special(struct audit_context *context, int *call_panic) >> { >> struct audit_buffer *ab; >> int i; >> >> + if (context->type == AUDIT_TIME_ADJNTPVAL) { >> + __audit_ntp_log_(&context->ntp.data); >> + return; >> + } > Can we find a way to move this down into the main switch statement in > show_special() like you did with AUDIT_TIME_INJOFFSET? This looks > *really* hacky to me. Why should AUDIT_TIME_ADJNTPVAL be different > from the other "special" bits? Agreed, it *looked* hacky to me too. As previously noted, this was the most expedient way to do this due to the unknown number of records being generated, minimum of zero (max 6). show_special allocates an audit_buffer before processing the specific record type, assuming it exists. In the case of audit_ntp_data, it may all be equal and not need to be logged. Ideally, the old vs new data should be compared at the time of the call from do_adjtimex() to find out if we even need to store that data when all old and new values are equal. So, a bit more pre-processing during the call to avoid storing the information if it isn't needed will avoid it at syscall exit. Are you ok with that? >> ab = audit_log_start(context, GFP_KERNEL, context->type); >> if (!ab) >> return; >> @@ -1324,6 +1331,11 @@ static void show_special(struct audit_context *context, int *call_panic) >> audit_log_format(ab, "(null)"); >> >> break; >> + case AUDIT_TIME_INJOFFSET: >> + audit_log_format(ab, "sec=%lli nsec=%li", >> + (long long)context->tk.injoffset.tv_sec, >> + context->tk.injoffset.tv_nsec); >> + break; >> } >> audit_log_end(ab); >> } >> @@ -2571,9 +2583,18 @@ 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); >> + struct audit_context *context = audit_context(); >> + >> + context->type = AUDIT_TIME_INJOFFSET; >> + memcpy(&context->tk.injoffset, &offset, sizeof(offset)); >> +} >> + >> +void __audit_ntp_log(const struct audit_ntp_data *ad) >> +{ >> + struct audit_context *context = audit_context(); >> + >> + context->type = AUDIT_TIME_ADJNTPVAL; >> + memcpy(&context->ntp.data, ad, sizeof(*ad)); >> } >> >> static void audit_log_ntp_val(const struct audit_ntp_data *ad, >> @@ -2588,7 +2609,7 @@ static void audit_log_ntp_val(const struct audit_ntp_data *ad, >> "op=%s old=%lli new=%lli", op, val->oldval, val->newval); >> } >> >> -void __audit_ntp_log(const struct audit_ntp_data *ad) >> +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); > Ooof, *please* don't end a function, or any symbol for that matter, > with an underscore. Ok, renamed to be consistent with others called from show_special(). > paul moore - RGB -- Richard Guy Briggs <rgb(a)redhat.com&gt; 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(a)redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit