On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <[email protected]> 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 <[email protected]>
> > ---
> > 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);
if (*abp) {
*abp = NULL;
type = (type == AUDIT_TIME_ADJNTPVAL ?
AUDIT_TIME_INJOFFSET : AUDIT_TIME_ADJNTPVAL);
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;'.
> > + }
--
paul-moore.com
--
Linux-audit mailing list
[email protected]
https://listman.redhat.com/mailman/listinfo/linux-audit
