Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On Fri, Feb 11, 2022 at 9:15 AM Richard Guy Briggs wrote:
>
> On 2022-02-10 21:08, Paul Moore wrote:
> > On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs wrote:
> > > On 2022-01-31 20:29, Paul Moore wrote:
> > > > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs
> > > > wrote:
> > > > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
> > > > > > 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 = >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
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On 2022-02-10 21:08, Paul Moore wrote:
> On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs wrote:
> > On 2022-01-31 20:29, Paul Moore wrote:
> > > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs
> > > wrote:
> > > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
> > > > > 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 = >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.
> paul-moore.com
- RGB
--
Richard Guy Briggs
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
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On Thu, Feb 10, 2022 at 6:46 PM Richard Guy Briggs wrote:
> On 2022-01-31 20:29, Paul Moore wrote:
> > On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs wrote:
> > > On 2022-01-31 17:02, Paul Moore wrote:
> > > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
> > > > 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 = >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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On 2022-01-31 20:29, Paul Moore wrote:
> On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs wrote:
> > On 2022-01-31 17:02, Paul Moore wrote:
> > > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
> > > 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
> > > > > ---
> > > > > 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(_user_ns, name->fcap.rootid));
> > > > > }
> > > > >
> > > > > +void audit_log_time(struct audit_context *context, struct
> > > > > audit_buffer **ab)
> > > > > +{
> > > > > + const struct audit_ntp_data *ntp = >time.ntp_data;
> > > > > + const struct timespec64 *tk = >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 =
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On Mon, Jan 31, 2022 at 6:29 PM Richard Guy Briggs wrote:
> On 2022-01-31 17:02, Paul Moore wrote:
> > On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs
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
> > > > ---
> > > > 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(_user_ns, name->fcap.rootid));
> > > > }
> > > >
> > > > +void audit_log_time(struct audit_context *context, struct
audit_buffer **ab)
> > > > +{
> > > > + const struct audit_ntp_data *ntp = >time.ntp_data;
> > > > + const struct timespec64 *tk = >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
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On 2022-01-31 17:02, Paul Moore wrote:
> On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs 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
> > > ---
> > > 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(_user_ns, name->fcap.rootid));
> > > }
> > >
> > > +void audit_log_time(struct audit_context *context, struct audit_buffer
> > > **ab)
> > > +{
> > > + const struct audit_ntp_data *ntp = >time.ntp_data;
> > > + const struct timespec64 *tk = >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);
> }
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
On Wed, Jan 26, 2022 at 8:52 AM Richard Guy Briggs 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
> > ---
> > 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(_user_ns, name->fcap.rootid));
> > }
> >
> > +void audit_log_time(struct audit_context *context, struct audit_buffer
> > **ab)
> > +{
> > + const struct audit_ntp_data *ntp = >time.ntp_data;
> > + const struct timespec64 *tk = >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 = >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 :
Re: [PATCH v3] audit: log AUDIT_TIME_* records only from rules
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
> ---
> 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(_user_ns, name->fcap.rootid));
> }
>
> +void audit_log_time(struct audit_context *context, struct audit_buffer **ab)
> +{
> + const struct audit_ntp_data *ntp = >time.ntp_data;
> + const struct timespec64 *tk = >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, );
> + 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);
> -}
> -
>
