9:24 p.m.
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(a)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/audit.h b/kernel/audit.h
index c4498090a5bd..58b66543b4d5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -201,6 +201,10 @@ struct audit_context {
struct {
char *name;
} module;
+ struct {
+ struct audit_ntp_data ntp_data;
+ struct timespec64 tk_injoffset;
+ } time;
};
int fds[2];
struct audit_proctitle proctitle;
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;
+ }
+ audit_log_format(*ab, "sec=%lli nsec=%li",
+ (long long)tk->tv_sec, tk->tv_nsec);
+ }
+}
+
static void show_special(struct audit_context *context, int *call_panic)
{
struct audit_buffer *ab;
@@ -1445,6 +1494,10 @@ static void show_special(struct audit_context *context, int
*call_panic)
audit_log_format(ab, "(null)");
break;
+ case AUDIT_TIME_ADJNTPVAL:
+ case AUDIT_TIME_INJOFFSET:
+ audit_log_time(context, &ab);
+ break;
}
audit_log_end(ab);
}
@@ -2840,31 +2893,24 @@ 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);
-}
-
-static void audit_log_ntp_val(const struct audit_ntp_data *ad,
- const char *op, enum audit_ntp_type type)
-{
- const struct audit_ntp_val *val = &ad->vals[type];
-
- if (val->newval == val->oldval)
- return;
+ struct audit_context *context = audit_context();
- audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
- "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
+ if (!context->type)
+ context->type = AUDIT_TIME_INJOFFSET;
+ memcpy(&context->time.tk_injoffset, &offset, sizeof(offset));
}
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;
+ }
}
void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
--
2.27.0
7:51 a.m.
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(a)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/audit.h b/kernel/audit.h
index c4498090a5bd..58b66543b4d5 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -201,6 +201,10 @@ struct audit_context {
struct {
char *name;
} module;
+ struct {
+ struct audit_ntp_data ntp_data;
+ struct timespec64 tk_injoffset;
+ } time;
};
int fds[2];
struct audit_proctitle proctitle;
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;
+ audit_log_format(*ab, "sec=%lli nsec=%li",
+ (long long)tk->tv_sec, tk->tv_nsec);
+ }
+}
+
static void show_special(struct audit_context *context, int *call_panic)
{
struct audit_buffer *ab;
@@ -1445,6 +1494,10 @@ static void show_special(struct audit_context *context, int
*call_panic)
audit_log_format(ab, "(null)");
break;
+ case AUDIT_TIME_ADJNTPVAL:
+ case AUDIT_TIME_INJOFFSET:
+ audit_log_time(context, &ab);
+ break;
}
audit_log_end(ab);
}
@@ -2840,31 +2893,24 @@ 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);
-}
-
-static void audit_log_ntp_val(const struct audit_ntp_data *ad,
- const char *op, enum audit_ntp_type type)
-{
- const struct audit_ntp_val *val = &ad->vals[type];
-
- if (val->newval == val->oldval)
- return;
+ struct audit_context *context = audit_context();
- audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
- "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
+ if (!context->type)
+ context->type = AUDIT_TIME_INJOFFSET;
+ memcpy(&context->time.tk_injoffset, &offset, sizeof(offset));
}
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;
+ }
}
void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries,
--
2.27.0
- RGB
--
Richard Guy Briggs <rgb(a)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
4:02 p.m.
On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb(a)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(a)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);
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
5:29 p.m.
On 2022-01-31 17:02, Paul Moore wrote:
On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
<rgb(a)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(a)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(a)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
7:29 p.m.
On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2022-01-31 17:02, Paul Moore wrote:
> On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb(a)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(a)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.
As audit_log_end() can safely take a NULL audit_buffer I don't care if we
send it back a valid buffer or a NULL. IMO it happens to be easier (and
cleaner) to send back a NULL.
> 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 ...
My mistake, I was distracted a few times while typing up my reply and the
code within; while I had that detail in mind when I started I lost it
during one of the interruptions. As penance, I wrote up some slightly more
proper code and at least made sure it complied, although I've not tried
booting it ...
diff --git a/kernel/audit.c b/kernel/audit.c
index e4bbe2c70c26..f21024d8a402 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1737,7 +1737,11 @@ static int __init audit_backlog_limit_set(char *str)
}
__setup("audit_backlog_limit=", audit_backlog_limit_set);
-static void audit_buffer_free(struct audit_buffer *ab)
+/**
+ * audit_buffer_free - free an audit buffer
+ * @ab: the audit buffer to free
+ */
+void audit_buffer_free(struct audit_buffer *ab)
{
if (!ab)
return;
diff --git a/kernel/audit.h b/kernel/audit.h
index c4498090a5bd..ba4c12b0d876 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -201,6 +201,10 @@ struct audit_context {
struct {
char *name;
} module;
+ struct {
+ struct audit_ntp_data ntp_data;
+ struct timespec64 tk_injoffset;
+ } time;
};
int fds[2];
struct audit_proctitle proctitle;
@@ -208,6 +212,8 @@ struct audit_context {
extern bool audit_ever_enabled;
+extern void audit_buffer_free(struct audit_buffer *ab);
+
extern void audit_log_session_info(struct audit_buffer *ab);
extern int auditd_test_task(struct task_struct *task);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fce5d43a933f..81434441aa13 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1340,6 +1340,76 @@ static void audit_log_fcaps(struct audit_buffer *ab,
struct audit_names *name)
from_kuid(&init_user_ns, name->fcap.rootid));
}
+/**
+ * audit_log_time - log ntp/time related information
+ * @context: the audit context
+ * @ab: an audit buffer
+ *
+ * This is a helper function for show_special() and should not be called
from
+ * any other context. This function also fully consumes @ab such that the
+ * caller should not assume it to be valid on return.
+ */
+static void audit_log_time(struct audit_context *context,
+ struct audit_buffer *ab)
+{
+ int i;
+ int type = context->type;
+ const char *ntp_name[] = {
+ "offset",
+ "freq",
+ "status",
+ "tai",
+ "tick",
+ "adjust",
+ };
+
+ do {
+ if (type == AUDIT_TIME_ADJNTPVAL) {
+ struct audit_ntp_val *ntp =
context->time.ntp_data.vals;
+
+ for (i = 0; i < AUDIT_NTP_NVALS; i++) {
+ if (ntp[i].newval == ntp[i].oldval)
+ continue;
+
+ if (!ab)
+ ab = audit_log_start(context,
+ GFP_KERNEL,
type);
+ audit_log_format(ab,
+ "op=%s old=%lli new=%lli",
+ ntp_name[i],
+ ntp[i].oldval,
ntp[i].newval);
+ audit_log_end(ab);
+ ab = NULL;
+ }
+
+ type = (type == context->type ?
+ AUDIT_TIME_INJOFFSET : 0);
+ } else if (type == AUDIT_TIME_INJOFFSET) {
+ struct timespec64 *tk = &context->time.tk_injoffset;
+
+ if (tk->tv_sec || tk->tv_nsec) {
+ if (!ab)
+ ab = audit_log_start(context,
+ GFP_KERNEL,
type);
+ audit_log_format(ab, "sec=%lli nsec=%li",
+ (long long)tk->tv_sec,
+ tk->tv_nsec);
+ audit_log_end(ab);
+ ab = NULL;
+ }
+
+ type = (type == context->type ?
+ AUDIT_TIME_ADJNTPVAL : 0);
+ } else
+ WARN_ONCE(1,
+ "audit_log_time() called with invalid
context (%d)\n",
+ context->type);
+ } while (type);
+
+ /* ab should always be NULL here, but cleanup _just_in_case_ ... */
+ audit_buffer_free(ab);
+}
+
static void show_special(struct audit_context *context, int *call_panic)
{
struct audit_buffer *ab;
@@ -1454,6 +1524,11 @@ static void show_special(struct audit_context
*context, int *call_panic)
audit_log_format(ab, "(null)");
break;
+ case AUDIT_TIME_ADJNTPVAL:
+ case AUDIT_TIME_INJOFFSET:
+ audit_log_time(context, ab);
+ ab = NULL;
+ break;
}
audit_log_end(ab);
}
@@ -2849,21 +2924,25 @@ 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();
+
+ if (!context->type)
+ context->type = AUDIT_TIME_INJOFFSET;
+ memcpy(&context->time.tk_injoffset, &offset, sizeof(offset));
}
static void audit_log_ntp_val(const struct audit_ntp_data *ad,
const char *op, enum audit_ntp_type type)
{
- const struct audit_ntp_val *val = &ad->vals[type];
-
- if (val->newval == val->oldval)
- return;
+ int i;
+ struct audit_context *context = audit_context();
- audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
- "op=%s old=%lli new=%lli", op, val->oldval,
val->newval);
+ for (i = 0; i < AUDIT_NTP_NVALS; i++)
+ if (ad->vals[i].newval != ad->vals[i].oldval) {
+ context->type = AUDIT_TIME_ADJNTPVAL;
+ memcpy(&context->time.ntp_data, ad, sizeof(*ad));
+ break;
+ }
}
void __audit_ntp_log(const struct audit_ntp_data *ad)
--
paul-moore.com
5:46 p.m.
On 2022-01-31 20:29, Paul Moore wrote:
On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2022-01-31 17:02, Paul Moore wrote:
> > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb(a)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(a)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.
As audit_log_end() can safely take a NULL audit_buffer I don't care if we
send it back a valid buffer or a NULL. IMO it happens to be easier (and
cleaner) to send back a NULL.
None of the other callees in show_special() do that, so this would be
surprising behaviour that could cause a future bug. This was part of
the UMN CS&E strategy.
> > 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 ...
My mistake, I was distracted a few times while typing up my reply and the
code within; while I had that detail in mind when I started I lost it
during one of the interruptions. As penance, I wrote up some slightly more
proper code and at least made sure it complied, although I've not tried
booting it ...
Did you test the code I submitted? It compiles and works. I found this
code harder to follow. This was partly why I wanted to leave one of the
record types outside of show_special() but I did find a way to
accomodate both with a minimum of overhead.
Speaking of distracted... I'm within earshot of a trucker convoy
that has dug in with horns blaring (with at least 4 train horns)
attempting to overthrow our government so I'm a bit rattled and I've had
a bit of trouble focussing for the last two weeks...
diff --git a/kernel/audit.c b/kernel/audit.c
index e4bbe2c70c26..f21024d8a402 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1737,7 +1737,11 @@ static int __init audit_backlog_limit_set(char *str)
}
__setup("audit_backlog_limit=", audit_backlog_limit_set);
-static void audit_buffer_free(struct audit_buffer *ab)
+/**
+ * audit_buffer_free - free an audit buffer
+ * @ab: the audit buffer to free
+ */
+void audit_buffer_free(struct audit_buffer *ab)
{
if (!ab)
return;
diff --git a/kernel/audit.h b/kernel/audit.h
index c4498090a5bd..ba4c12b0d876 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -201,6 +201,10 @@ struct audit_context {
struct {
char *name;
} module;
+ struct {
+ struct audit_ntp_data ntp_data;
+ struct timespec64 tk_injoffset;
+ } time;
};
int fds[2];
struct audit_proctitle proctitle;
@@ -208,6 +212,8 @@ struct audit_context {
extern bool audit_ever_enabled;
+extern void audit_buffer_free(struct audit_buffer *ab);
+
extern void audit_log_session_info(struct audit_buffer *ab);
extern int auditd_test_task(struct task_struct *task);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index fce5d43a933f..81434441aa13 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1340,6 +1340,76 @@ static void audit_log_fcaps(struct audit_buffer *ab,
struct audit_names *name)
from_kuid(&init_user_ns, name->fcap.rootid));
}
+/**
+ * audit_log_time - log ntp/time related information
+ * @context: the audit context
+ * @ab: an audit buffer
+ *
+ * This is a helper function for show_special() and should not be called from
+ * any other context. This function also fully consumes @ab such that the
+ * caller should not assume it to be valid on return.
+ */
+static void audit_log_time(struct audit_context *context,
+ struct audit_buffer *ab)
+{
+ int i;
+ int type = context->type;
+ const char *ntp_name[] = {
+ "offset",
+ "freq",
+ "status",
+ "tai",
+ "tick",
+ "adjust",
+ };
+
+ do {
+ if (type == AUDIT_TIME_ADJNTPVAL) {
+ struct audit_ntp_val *ntp = context->time.ntp_data.vals;
+
+ for (i = 0; i < AUDIT_NTP_NVALS; i++) {
+ if (ntp[i].newval == ntp[i].oldval)
+ continue;
+
+ if (!ab)
+ ab = audit_log_start(context,
+ GFP_KERNEL, type);
+ audit_log_format(ab,
+ "op=%s old=%lli new=%lli",
+ ntp_name[i],
+ ntp[i].oldval, ntp[i].newval);
+ audit_log_end(ab);
+ ab = NULL;
+ }
+
+ type = (type == context->type ?
+ AUDIT_TIME_INJOFFSET : 0);
Why the conditional? If it made it this far, the next type, if it
passes the condition below will be TK.
+ } else if (type == AUDIT_TIME_INJOFFSET) {
+ struct timespec64 *tk = &context->time.tk_injoffset;
+
+ if (tk->tv_sec || tk->tv_nsec) {
+ if (!ab)
+ ab = audit_log_start(context,
+ GFP_KERNEL, type);
+ audit_log_format(ab, "sec=%lli nsec=%li",
+ (long long)tk->tv_sec,
+ tk->tv_nsec);
+ audit_log_end(ab);
+ ab = NULL;
+ }
+
+ type = (type == context->type ?
+ AUDIT_TIME_ADJNTPVAL : 0);
Why? This is unnecessary since if the type is TK, the type can't be
NTP.
+ } else
+ WARN_ONCE(1,
+ "audit_log_time() called with invalid context
(%d)\n",
+ context->type);
This code is unnecessary since it can never be executed.
+ } while (type);
+
+ /* ab should always be NULL here, but cleanup _just_in_case_ ... */
+ audit_buffer_free(ab);
We never need a buffer free since it is taken care of above if the logic
is correct and isn't needed if we let the calling function call
audit_log_end().
+}
+
static void show_special(struct audit_context *context, int *call_panic)
{
struct audit_buffer *ab;
@@ -1454,6 +1524,11 @@ static void show_special(struct audit_context
*context, int *call_panic)
audit_log_format(ab, "(null)");
break;
+ case AUDIT_TIME_ADJNTPVAL:
+ case AUDIT_TIME_INJOFFSET:
+ audit_log_time(context, ab);
+ ab = NULL;
+ break;
}
audit_log_end(ab);
}
@@ -2849,21 +2924,25 @@ 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();
+
+ if (!context->type)
+ context->type = AUDIT_TIME_INJOFFSET;
+ memcpy(&context->time.tk_injoffset, &offset, sizeof(offset));
}
static void audit_log_ntp_val(const struct audit_ntp_data *ad,
const char *op, enum audit_ntp_type type)
{
- const struct audit_ntp_val *val = &ad->vals[type];
-
- if (val->newval == val->oldval)
- return;
+ int i;
+ struct audit_context *context = audit_context();
- audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_ADJNTPVAL,
- "op=%s old=%lli new=%lli", op, val->oldval,
val->newval);
+ for (i = 0; i < AUDIT_NTP_NVALS; i++)
+ if (ad->vals[i].newval != ad->vals[i].oldval) {
+ context->type = AUDIT_TIME_ADJNTPVAL;
+ memcpy(&context->time.ntp_data, ad, sizeof(*ad));
+ break;
+ }
}
This code might compile, but it won't work since audit_log_ntp_val() is
never called and the values are never set.
void __audit_ntp_log(const struct audit_ntp_data *ad)
--
paul-moore.com
- RGB
--
Richard Guy Briggs <rgb(a)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
8:08 p.m.
On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2022-01-31 20:29, Paul Moore wrote:
> On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2022-01-31 17:02, Paul Moore wrote:
> > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > > On 2022-01-25 22:24, Richard Guy Briggs wrote:
This isn't as complete of a response as I would like, but I wanted to
get *something* back to you same-day since the delays are getting a
bit long ...
> > > [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.
>
> As audit_log_end() can safely take a NULL audit_buffer I don't care if we
> send it back a valid buffer or a NULL. IMO it happens to be easier (and
> cleaner) to send back a NULL.
None of the other callees in show_special() do that, so this would be
surprising behaviour that could cause a future bug ...
To be both honest and frank: I disagree with your assessment. If you
really want to be concerned about this, there are plenty of ways to
mitigate the "risk" depending on your comfort level; comments and
returning within the switch/case block are just some of the options.
> > > 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 ...
>
> My mistake, I was distracted a few times while typing up my reply and the
> code within; while I had that detail in mind when I started I lost it
> during one of the interruptions. As penance, I wrote up some slightly more
> proper code and at least made sure it complied, although I've not tried
> booting it ...
Did you test the code I submitted? It compiles and works. I found this
code harder to follow. This was partly why I wanted to leave one of the
record types outside of show_special() but I did find a way to
accomodate both with a minimum of overhead.
Once again, I disagree with your assessment of the code. I'm not sure
how to put this politely, but I personally found your audit_log_time()
implementation to be awkward; the AUDIT_TIME_INJOFFSET goto/jump to
"tk" at the top of the function was not something I'm going to merge.
You don't have to like my suggestion, but please send me a patch that
has a somewhat reasonable code flow. I know you often want, or at
least ask for explicit suggestions (hence my untested code example),
but since you didn't like that let me just say this: when in doubt,
limit your use of gotos/jumps to error handling unless there is
something significantly unusual about the function. In my opinion
there is nothing significantly unusual about the audit_log_time()
function to require a jump as you've currently done.
--
paul-moore.com
8:14 a.m.
On 2022-02-10 21:08, Paul Moore wrote:
On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> On 2022-01-31 20:29, Paul Moore wrote:
> > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > On 2022-01-25 22:24, Richard Guy Briggs wrote:
This isn't as complete of a response as I would like, but I wanted to
get *something* back to you same-day since the delays are getting a
bit long ...
> > > > [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.
> >
> > As audit_log_end() can safely take a NULL audit_buffer I don't care if we
> > send it back a valid buffer or a NULL. IMO it happens to be easier (and
> > cleaner) to send back a NULL.
>
> None of the other callees in show_special() do that, so this would be
> surprising behaviour that could cause a future bug ...
To be both honest and frank: I disagree with your assessment. If you
really want to be concerned about this, there are plenty of ways to
mitigate the "risk" depending on your comfort level; comments and
returning within the switch/case block are just some of the options.
> > > > 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 ...
> >
> > My mistake, I was distracted a few times while typing up my reply and the
> > code within; while I had that detail in mind when I started I lost it
> > during one of the interruptions. As penance, I wrote up some slightly more
> > proper code and at least made sure it complied, although I've not tried
> > booting it ...
>
> Did you test the code I submitted? It compiles and works. I found this
> code harder to follow. This was partly why I wanted to leave one of the
> record types outside of show_special() but I did find a way to
> accomodate both with a minimum of overhead.
Once again, I disagree with your assessment of the code. I'm not sure
how to put this politely, but I personally found your audit_log_time()
implementation to be awkward; the AUDIT_TIME_INJOFFSET goto/jump to
"tk" at the top of the function was not something I'm going to merge.
You don't have to like my suggestion, but please send me a patch that
has a somewhat reasonable code flow. I know you often want, or at
least ask for explicit suggestions (hence my untested code example),
but since you didn't like that let me just say this: when in doubt,
limit your use of gotos/jumps to error handling unless there is
something significantly unusual about the function. In my opinion
there is nothing significantly unusual about the audit_log_time()
function to require a jump as you've currently done.
I hate gotos. I first learned to program on Waterloo Structured BASIC
4.0 on a Commodore PET 2001 where the language structure provided the
tools to be able to avoid them. I've avoided them and reluctantly used
them at your urging, so now I'm a bit confused.
- RGB
--
Richard Guy Briggs <rgb(a)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
9:25 a.m.
On Fri, Feb 11, 2022 at 9:15 AM Richard Guy Briggs <rgb(a)redhat.com> wrote:
On 2022-02-10 21:08, Paul Moore wrote:
> On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs <rgb(a)redhat.com> wrote:
> > On 2022-01-31 20:29, Paul Moore wrote:
> > > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs <rgb(a)redhat.com>
wrote:
> > > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
<rgb(a)redhat.com> wrote:
> > > > > > On 2022-01-25 22:24, Richard Guy Briggs wrote:
>
> This isn't as complete of a response as I would like, but I wanted to
> get *something* back to you same-day since the delays are getting a
> bit long ...
>
> > > > > [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.
> > >
> > > As audit_log_end() can safely take a NULL audit_buffer I don't care if
we
> > > send it back a valid buffer or a NULL. IMO it happens to be easier (and
> > > cleaner) to send back a NULL.
> >
> > None of the other callees in show_special() do that, so this would be
> > surprising behaviour that could cause a future bug ...
>
> To be both honest and frank: I disagree with your assessment. If you
> really want to be concerned about this, there are plenty of ways to
> mitigate the "risk" depending on your comfort level; comments and
> returning within the switch/case block are just some of the options.
>
> > > > > 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 ...
> > >
> > > My mistake, I was distracted a few times while typing up my reply and the
> > > code within; while I had that detail in mind when I started I lost it
> > > during one of the interruptions. As penance, I wrote up some slightly
more
> > > proper code and at least made sure it complied, although I've not
tried
> > > booting it ...
> >
> > Did you test the code I submitted? It compiles and works. I found this
> > code harder to follow. This was partly why I wanted to leave one of the
> > record types outside of show_special() but I did find a way to
> > accomodate both with a minimum of overhead.
>
> Once again, I disagree with your assessment of the code. I'm not sure
> how to put this politely, but I personally found your audit_log_time()
> implementation to be awkward; the AUDIT_TIME_INJOFFSET goto/jump to
> "tk" at the top of the function was not something I'm going to merge.
> You don't have to like my suggestion, but please send me a patch that
> has a somewhat reasonable code flow. I know you often want, or at
> least ask for explicit suggestions (hence my untested code example),
> but since you didn't like that let me just say this: when in doubt,
> limit your use of gotos/jumps to error handling unless there is
> something significantly unusual about the function. In my opinion
> there is nothing significantly unusual about the audit_log_time()
> function to require a jump as you've currently done.
I hate gotos. I first learned to program on Waterloo Structured BASIC
4.0 on a Commodore PET 2001 where the language structure provided the
tools to be able to avoid them. I've avoided them and reluctantly used
them at your urging, so now I'm a bit confused.
If you're confused follow the recommendations at the end of the
paragraph directly above your reply. Or look at the sample code I
proposed. You've been contributing long enough that this shouldn't be
that hard.
--
paul-moore.com
1044
days inactive
1060
days old
linux-audit@lists.linux-audit.osci.io
8 comments
2 participants
participants (2)
-
Paul Moore -
Richard Guy Briggs