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

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

[Richard Guy Briggs]

AUDIT_TIME_* events are generated when there are syscall rules present that are
not related to time keeping.  This will produce noisy log entries that could
flood the logs and hide events we really care about.

Rather than immediately produce the AUDIT_TIME_* records, store the data and
log it at syscall exit time respecting the filter rules.

Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
Note: This is a quick and dirty proof-of-concept.  If this approach of
storing the values in the audit_context for later filtering is
acceptable I'll clean up the patch (re-name functions) and re-submit.

 kernel/audit.h   |  6 ++++++
 kernel/auditsc.c | 29 +++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/kernel/audit.h b/kernel/audit.h
index 3b64a97f6091..25d63731b0e0 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -196,6 +196,12 @@ struct audit_context {
 		struct {
 			char			*name;
 		} module;
+		struct {
+			struct audit_ntp_data	data;
+		} ntp;
+		struct {
+			struct timespec64	injoffset;
+		} tk;
 	};
 	int fds[2];
 	struct audit_proctitle proctitle;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 6efb0bb909d0..8983790ad86a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1210,11 +1210,18 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
 			 from_kuid(&init_user_ns, name->fcap.rootid));
 }
 
+void __audit_ntp_log_(const struct audit_ntp_data *ad);
+
 static void show_special(struct audit_context *context, int *call_panic)
 {
 	struct audit_buffer *ab;
 	int i;
 
+	if (context->type == AUDIT_TIME_ADJNTPVAL) {
+		__audit_ntp_log_(&context->ntp.data);
+		return;
+	}
+
 	ab = audit_log_start(context, GFP_KERNEL, context->type);
 	if (!ab)
 		return;
@@ -1324,6 +1331,11 @@ static void show_special(struct audit_context *context, int *call_panic)
 			audit_log_format(ab, "(null)");
 
 		break;
+	case AUDIT_TIME_INJOFFSET:
+		audit_log_format(ab, "sec=%lli nsec=%li",
+				 (long long)context->tk.injoffset.tv_sec,
+				 context->tk.injoffset.tv_nsec);
+		break;
 	}
 	audit_log_end(ab);
 }
@@ -2571,9 +2583,18 @@ void __audit_fanotify(unsigned int response)
 
 void __audit_tk_injoffset(struct timespec64 offset)
 {
-	audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_INJOFFSET,
-		  "sec=%lli nsec=%li",
-		  (long long)offset.tv_sec, offset.tv_nsec);
+	struct audit_context *context = audit_context();
+
+	context->type = AUDIT_TIME_INJOFFSET;
+	memcpy(&context->tk.injoffset, &offset, sizeof(offset));
+}
+
+void __audit_ntp_log(const struct audit_ntp_data *ad)
+{
+	struct audit_context *context = audit_context();
+
+	context->type = AUDIT_TIME_ADJNTPVAL;
+	memcpy(&context->ntp.data, ad, sizeof(*ad));
 }
 
 static void audit_log_ntp_val(const struct audit_ntp_data *ad,
@@ -2588,7 +2609,7 @@ static void audit_log_ntp_val(const struct audit_ntp_data *ad,
 		  "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
 }
 
-void __audit_ntp_log(const struct audit_ntp_data *ad)
+void __audit_ntp_log_(const struct audit_ntp_data *ad)
 {
 	audit_log_ntp_val(ad, "offset",	AUDIT_NTP_OFFSET);
 	audit_log_ntp_val(ad, "freq",	AUDIT_NTP_FREQ);
-- 
2.27.0

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> AUDIT_TIME_* events are generated when there are syscall rules present that are
> not related to time keeping.  This will produce noisy log entries that could
> flood the logs and hide events we really care about.
>
> Rather than immediately produce the AUDIT_TIME_* records, store the data and
> log it at syscall exit time respecting the filter rules.
>
> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919

Unfortunately that URL isn't publicly accessible.  It might be helpful
to simply add the relevant information to the commit description[1]
and omit the link entirely.  Since this is just an RFC, please don't
resend the patch just to include that information, you can simply
reply to this thread with the additional info.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

On 2021-11-04 17:29, Paul Moore wrote:
> On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > not related to time keeping.  This will produce noisy log entries that could
> > flood the logs and hide events we really care about.
> >
> > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > log it at syscall exit time respecting the filter rules.
> >
> > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> 
> Unfortunately that URL isn't publicly accessible.  It might be helpful
> to simply add the relevant information to the commit description[1]
> and omit the link entirely.  Since this is just an RFC, please don't
> resend the patch just to include that information, you can simply
> reply to this thread with the additional info.

Hmmm, sorry about that.  There isn't really anything in that bz that
shouldn't be public, but I'll check before openning it up...

Basically it was a report that:
TIME_ADJNTPVAL audit events are not generated if there are no syscalls
rules, but that these events are generated when at least one unrelated
syscall rule is added.

This behaviour was confirmed but the conclusion about what should be the
correct behaviour differed from that of the reporter.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Thu, Nov 4, 2021 at 5:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-11-04 17:29, Paul Moore wrote:
> > On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > >
> > > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > > not related to time keeping.  This will produce noisy log entries that could
> > > flood the logs and hide events we really care about.
> > >
> > > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > > log it at syscall exit time respecting the filter rules.
> > >
> > > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> >
> > Unfortunately that URL isn't publicly accessible.  It might be helpful
> > to simply add the relevant information to the commit description[1]
> > and omit the link entirely.  Since this is just an RFC, please don't
> > resend the patch just to include that information, you can simply
> > reply to this thread with the additional info.
>
> Hmmm, sorry about that.  There isn't really anything in that bz that
> shouldn't be public, but I'll check before openning it up...
>
> Basically it was a report that:
> TIME_ADJNTPVAL audit events are not generated if there are no syscalls
> rules, but that these events are generated when at least one unrelated
> syscall rule is added.
>
> This behaviour was confirmed but the conclusion about what should be the
> correct behaviour differed from that of the reporter.

I'm still wondering about the best way to handle this situation, and I
want to make sure I'm understanding the problem correctly.  So I'm
clear on the problem, is the issue that the AUDIT_TIME records are
being generated whenever at least one syscall filter is present,
regardless of if that syscall is time related?  With the expected
behavior being that AUDIT_TIME records are only generated when a time
related syscall is being audited?

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

On 2021-11-19 11:15, Paul Moore wrote:
> On Thu, Nov 4, 2021 at 5:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-11-04 17:29, Paul Moore wrote:
> > > On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > >
> > > > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > > > not related to time keeping.  This will produce noisy log entries that could
> > > > flood the logs and hide events we really care about.
> > > >
> > > > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > > > log it at syscall exit time respecting the filter rules.
> > > >
> > > > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> > >
> > > Unfortunately that URL isn't publicly accessible.  It might be helpful
> > > to simply add the relevant information to the commit description[1]
> > > and omit the link entirely.  Since this is just an RFC, please don't
> > > resend the patch just to include that information, you can simply
> > > reply to this thread with the additional info.
> >
> > Hmmm, sorry about that.  There isn't really anything in that bz that
> > shouldn't be public, but I'll check before openning it up...
> >
> > Basically it was a report that:
> > TIME_ADJNTPVAL audit events are not generated if there are no syscalls
> > rules, but that these events are generated when at least one unrelated
> > syscall rule is added.
> >
> > This behaviour was confirmed but the conclusion about what should be the
> > correct behaviour differed from that of the reporter.
> 
> I'm still wondering about the best way to handle this situation, and I
> want to make sure I'm understanding the problem correctly.  So I'm
> clear on the problem, is the issue that the AUDIT_TIME records are
> being generated whenever at least one syscall filter is present,
> regardless of if that syscall is time related?  With the expected
> behavior being that AUDIT_TIME records are only generated when a time
> related syscall is being audited?

Exactly.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Fri, Nov 19, 2021 at 1:02 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-11-19 11:15, Paul Moore wrote:
> > On Thu, Nov 4, 2021 at 5:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2021-11-04 17:29, Paul Moore wrote:
> > > > On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > >
> > > > > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > > > > not related to time keeping.  This will produce noisy log entries that could
> > > > > flood the logs and hide events we really care about.
> > > > >
> > > > > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > > > > log it at syscall exit time respecting the filter rules.
> > > > >
> > > > > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> > > >
> > > > Unfortunately that URL isn't publicly accessible.  It might be helpful
> > > > to simply add the relevant information to the commit description[1]
> > > > and omit the link entirely.  Since this is just an RFC, please don't
> > > > resend the patch just to include that information, you can simply
> > > > reply to this thread with the additional info.
> > >
> > > Hmmm, sorry about that.  There isn't really anything in that bz that
> > > shouldn't be public, but I'll check before openning it up...
> > >
> > > Basically it was a report that:
> > > TIME_ADJNTPVAL audit events are not generated if there are no syscalls
> > > rules, but that these events are generated when at least one unrelated
> > > syscall rule is added.
> > >
> > > This behaviour was confirmed but the conclusion about what should be the
> > > correct behaviour differed from that of the reporter.
> >
> > I'm still wondering about the best way to handle this situation, and I
> > want to make sure I'm understanding the problem correctly.  So I'm
> > clear on the problem, is the issue that the AUDIT_TIME records are
> > being generated whenever at least one syscall filter is present,
> > regardless of if that syscall is time related?  With the expected
> > behavior being that AUDIT_TIME records are only generated when a time
> > related syscall is being audited?
>
> Exactly.

In that case it seems pretty similar to the PATH record and I would
think that handling it in a similar manner would be The Right Thing to
do ... which looks like what you are doing.

You mentioned that you wanted to do some more work on this patch so
I'll hold off further comments until the updated patch is posted.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

On 2021-11-24 10:44, Paul Moore wrote:
> On Fri, Nov 19, 2021 at 1:02 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-11-19 11:15, Paul Moore wrote:
> > > On Thu, Nov 4, 2021 at 5:53 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2021-11-04 17:29, Paul Moore wrote:
> > > > > On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > > >
> > > > > > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > > > > > not related to time keeping.  This will produce noisy log entries that could
> > > > > > flood the logs and hide events we really care about.
> > > > > >
> > > > > > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > > > > > log it at syscall exit time respecting the filter rules.
> > > > > >
> > > > > > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> > > > >
> > > > > Unfortunately that URL isn't publicly accessible.  It might be helpful
> > > > > to simply add the relevant information to the commit description[1]
> > > > > and omit the link entirely.  Since this is just an RFC, please don't
> > > > > resend the patch just to include that information, you can simply
> > > > > reply to this thread with the additional info.
> > > >
> > > > Hmmm, sorry about that.  There isn't really anything in that bz that
> > > > shouldn't be public, but I'll check before openning it up...
> > > >
> > > > Basically it was a report that:
> > > > TIME_ADJNTPVAL audit events are not generated if there are no syscalls
> > > > rules, but that these events are generated when at least one unrelated
> > > > syscall rule is added.
> > > >
> > > > This behaviour was confirmed but the conclusion about what should be the
> > > > correct behaviour differed from that of the reporter.
> > >
> > > I'm still wondering about the best way to handle this situation, and I
> > > want to make sure I'm understanding the problem correctly.  So I'm
> > > clear on the problem, is the issue that the AUDIT_TIME records are
> > > being generated whenever at least one syscall filter is present,
> > > regardless of if that syscall is time related?  With the expected
> > > behavior being that AUDIT_TIME records are only generated when a time
> > > related syscall is being audited?
> >
> > Exactly.
> 
> In that case it seems pretty similar to the PATH record and I would
> think that handling it in a similar manner would be The Right Thing to
> do ... which looks like what you are doing.
> 
> You mentioned that you wanted to do some more work on this patch so
> I'll hold off further comments until the updated patch is posted.

I had a look at this patch and there is no further adjustment needed.
The only note is that the AUDIT_TIME_ADJNTPVAL record is printed at the
top of show_special() due to the need to potentially allocate multiple
records.  Do you think this requires a comment in the description or
in the code just above the call to __audit_ntp_log_()?

If not, please merge it at your convenience.  Sorry to have dropped this
ball.

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Tue, Dec 21, 2021 at 3:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-11-24 10:44, Paul Moore wrote:
>
> > You mentioned that you wanted to do some more work on this patch so
> > I'll hold off further comments until the updated patch is posted.
>
> I had a look at this patch and there is no further adjustment needed.
> The only note is that the AUDIT_TIME_ADJNTPVAL record is printed at the
> top of show_special() due to the need to potentially allocate multiple
> records.  Do you think this requires a comment in the description or
> in the code just above the call to __audit_ntp_log_()?
>
> If not, please merge it at your convenience.  Sorry to have dropped this
> ball.

No worries, I'll put the patch back on the to-review pile.  However,
since we are at -rc6 this week with the holidays in full swing it
seems like this is something that we should defer merging until after
the upcoming merge window closes.  Now that this is no longer a RFC,
I'll try to take a closer look and offer any additional review
feedback next week.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

On 2021-12-23 12:38, Paul Moore wrote:
> On Tue, Dec 21, 2021 at 3:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2021-11-24 10:44, Paul Moore wrote:
> > > You mentioned that you wanted to do some more work on this patch so
> > > I'll hold off further comments until the updated patch is posted.
> >
> > I had a look at this patch and there is no further adjustment needed.
> > The only note is that the AUDIT_TIME_ADJNTPVAL record is printed at the
> > top of show_special() due to the need to potentially allocate multiple
> > records.  Do you think this requires a comment in the description or
> > in the code just above the call to __audit_ntp_log_()?
> >
> > If not, please merge it at your convenience.  Sorry to have dropped this
> > ball.
> 
> No worries, I'll put the patch back on the to-review pile.  However,
> since we are at -rc6 this week with the holidays in full swing it
> seems like this is something that we should defer merging until after
> the upcoming merge window closes.  Now that this is no longer a RFC,
> I'll try to take a closer look and offer any additional review
> feedback next week.

Darn, missed another merge window.  My fault for dropping the ball.

If I might advocate, it is a bugfix rather than a feature...

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Fri, Dec 24, 2021 at 9:21 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2021-12-23 12:38, Paul Moore wrote:
> > On Tue, Dec 21, 2021 at 3:50 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2021-11-24 10:44, Paul Moore wrote:
> > > > You mentioned that you wanted to do some more work on this patch so
> > > > I'll hold off further comments until the updated patch is posted.
> > >
> > > I had a look at this patch and there is no further adjustment needed.
> > > The only note is that the AUDIT_TIME_ADJNTPVAL record is printed at the
> > > top of show_special() due to the need to potentially allocate multiple
> > > records.  Do you think this requires a comment in the description or
> > > in the code just above the call to __audit_ntp_log_()?
> > >
> > > If not, please merge it at your convenience.  Sorry to have dropped this
> > > ball.
> >
> > No worries, I'll put the patch back on the to-review pile.  However,
> > since we are at -rc6 this week with the holidays in full swing it
> > seems like this is something that we should defer merging until after
> > the upcoming merge window closes.  Now that this is no longer a RFC,
> > I'll try to take a closer look and offer any additional review
> > feedback next week.
>
> Darn, missed another merge window.  My fault for dropping the ball.

I know you are well aware of this policy Richard, but for others who
may be watching this, I don't like to take significant changes into
-next much past the -rc5/-rc6 boundary unless it is a critical bug
fix.  While this particular patch might not be very large, it is a
user visible change which makes it a "significant change" in my
opinion.

https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git/tree/README.md

> If I might advocate, it is a bugfix rather than a feature...

Maybe.  I can see an argument for viewing it that way, but I also see
it as a user visible change.  Taking into account where we are at in
the current -rcX cycle, the fact that large portions of the kernel
development community are in the midst of various end-of-year
holidays, and the nature of the patch, I feel it is better to give
this more than a week or two in -next.

I also feel that if this was truly that important, we would have seen
more urgency and comments associated with the patch.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> AUDIT_TIME_* events are generated when there are syscall rules present that are
> not related to time keeping.  This will produce noisy log entries that could
> flood the logs and hide events we really care about.
>
> Rather than immediately produce the AUDIT_TIME_* records, store the data and
> log it at syscall exit time respecting the filter rules.
>
> Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> Note: This is a quick and dirty proof-of-concept.  If this approach of
> storing the values in the audit_context for later filtering is
> acceptable I'll clean up the patch (re-name functions) and re-submit.
>
>  kernel/audit.h   |  6 ++++++
>  kernel/auditsc.c | 29 +++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)

Reviewing this now with a more critical eye since it is longer just a
quick-n-dirty proof of concept ...

> diff --git a/kernel/audit.h b/kernel/audit.h
> index 3b64a97f6091..25d63731b0e0 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -196,6 +196,12 @@ struct audit_context {
>                 struct {
>                         char                    *name;
>                 } module;
> +               struct {
> +                       struct audit_ntp_data   data;
> +               } ntp;
> +               struct {
> +                       struct timespec64       injoffset;
> +               } tk;

With the ntp and tk structs being separate parts of a union, are we
going to have a problem when ADJ_SETOFFSET is set in a call to
do_adjtimex()?

>         };
>         int fds[2];
>         struct audit_proctitle proctitle;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 6efb0bb909d0..8983790ad86a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1210,11 +1210,18 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
>                          from_kuid(&init_user_ns, name->fcap.rootid));
>  }
>
> +void __audit_ntp_log_(const struct audit_ntp_data *ad);
> +
>  static void show_special(struct audit_context *context, int *call_panic)
>  {
>         struct audit_buffer *ab;
>         int i;
>
> +       if (context->type == AUDIT_TIME_ADJNTPVAL) {
> +               __audit_ntp_log_(&context->ntp.data);
> +               return;
> +       }

Can we find a way to move this down into the main switch statement in
show_special() like you did with AUDIT_TIME_INJOFFSET?  This looks
*really* hacky to me.  Why should AUDIT_TIME_ADJNTPVAL be different
from the other "special" bits?

>         ab = audit_log_start(context, GFP_KERNEL, context->type);
>         if (!ab)
>                 return;
> @@ -1324,6 +1331,11 @@ static void show_special(struct audit_context *context, int *call_panic)
>                         audit_log_format(ab, "(null)");
>
>                 break;
> +       case AUDIT_TIME_INJOFFSET:
> +               audit_log_format(ab, "sec=%lli nsec=%li",
> +                                (long long)context->tk.injoffset.tv_sec,
> +                                context->tk.injoffset.tv_nsec);
> +               break;
>         }
>         audit_log_end(ab);
>  }
> @@ -2571,9 +2583,18 @@ void __audit_fanotify(unsigned int response)
>
>  void __audit_tk_injoffset(struct timespec64 offset)
>  {
> -       audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_INJOFFSET,
> -                 "sec=%lli nsec=%li",
> -                 (long long)offset.tv_sec, offset.tv_nsec);
> +       struct audit_context *context = audit_context();
> +
> +       context->type = AUDIT_TIME_INJOFFSET;
> +       memcpy(&context->tk.injoffset, &offset, sizeof(offset));
> +}
> +
> +void __audit_ntp_log(const struct audit_ntp_data *ad)
> +{
> +       struct audit_context *context = audit_context();
> +
> +       context->type = AUDIT_TIME_ADJNTPVAL;
> +       memcpy(&context->ntp.data, ad, sizeof(*ad));
>  }
>
>  static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> @@ -2588,7 +2609,7 @@ static void audit_log_ntp_val(const struct audit_ntp_data *ad,
>                   "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
>  }
>
> -void __audit_ntp_log(const struct audit_ntp_data *ad)
> +void __audit_ntp_log_(const struct audit_ntp_data *ad)
>  {
>         audit_log_ntp_val(ad, "offset", AUDIT_NTP_OFFSET);
>         audit_log_ntp_val(ad, "freq",   AUDIT_NTP_FREQ);

Ooof, *please* don't end a function, or any symbol for that matter,
with an underscore.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Richard Guy Briggs]

On 2021-12-29 17:51, Paul Moore wrote:
> On Thu, Nov 4, 2021 at 5:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > AUDIT_TIME_* events are generated when there are syscall rules present that are
> > not related to time keeping.  This will produce noisy log entries that could
> > flood the logs and hide events we really care about.
> >
> > Rather than immediately produce the AUDIT_TIME_* records, store the data and
> > log it at syscall exit time respecting the filter rules.
> >
> > Please see https://bugzilla.redhat.com/show_bug.cgi?id=1991919
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> > Note: This is a quick and dirty proof-of-concept.  If this approach of
> > storing the values in the audit_context for later filtering is
> > acceptable I'll clean up the patch (re-name functions) and re-submit.
> >
> >  kernel/audit.h   |  6 ++++++
> >  kernel/auditsc.c | 29 +++++++++++++++++++++++++----
> >  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> Reviewing this now with a more critical eye since it is longer just a
> quick-n-dirty proof of concept ...
> 
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 3b64a97f6091..25d63731b0e0 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -196,6 +196,12 @@ struct audit_context {
> >                 struct {
> >                         char                    *name;
> >                 } module;
> > +               struct {
> > +                       struct audit_ntp_data   data;
> > +               } ntp;
> > +               struct {
> > +                       struct timespec64       injoffset;
> > +               } tk;
> 
> With the ntp and tk structs being separate parts of a union, are we
> going to have a problem when ADJ_SETOFFSET is set in a call to
> do_adjtimex()?

Yes, so since both record types can exist in one event, either both
pieces of data will need to be in one struct within the union, or
one piece of data will need to go outside of the union (preferably the
smaller one to avoid bloating struct audit_context more than necessary).
The tk data is smaller and is easier to check if it is set, so that
might be better to store outside the union.

Since show_special is keyed off record type, it makes more sense to
store one of those datum outside the union and process it outside
show_special to avoid record type tricks in show_special.

> >         };
> >         int fds[2];
> >         struct audit_proctitle proctitle;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 6efb0bb909d0..8983790ad86a 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1210,11 +1210,18 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> >                          from_kuid(&init_user_ns, name->fcap.rootid));
> >  }
> >
> > +void __audit_ntp_log_(const struct audit_ntp_data *ad);
> > +
> >  static void show_special(struct audit_context *context, int *call_panic)
> >  {
> >         struct audit_buffer *ab;
> >         int i;
> >
> > +       if (context->type == AUDIT_TIME_ADJNTPVAL) {
> > +               __audit_ntp_log_(&context->ntp.data);
> > +               return;
> > +       }
> 
> Can we find a way to move this down into the main switch statement in
> show_special() like you did with AUDIT_TIME_INJOFFSET?  This looks
> *really* hacky to me.  Why should AUDIT_TIME_ADJNTPVAL be different
> from the other "special" bits?

Agreed, it *looked* hacky to me too.  As previously noted, this was the
most expedient way to do this due to the unknown number of records being
generated, minimum of zero (max 6).  show_special allocates an
audit_buffer before processing the specific record type, assuming it
exists.  In the case of audit_ntp_data, it may all be equal and not need
to be logged.

Ideally, the old vs new data should be compared at the time of the call
from do_adjtimex() to find out if we even need to store that data
when all old and new values are equal.

So, a bit more pre-processing during the call to avoid storing the
information if it isn't needed will avoid it at syscall exit.  Are you
ok with that?

> >         ab = audit_log_start(context, GFP_KERNEL, context->type);
> >         if (!ab)
> >                 return;
> > @@ -1324,6 +1331,11 @@ static void show_special(struct audit_context *context, int *call_panic)
> >                         audit_log_format(ab, "(null)");
> >
> >                 break;
> > +       case AUDIT_TIME_INJOFFSET:
> > +               audit_log_format(ab, "sec=%lli nsec=%li",
> > +                                (long long)context->tk.injoffset.tv_sec,
> > +                                context->tk.injoffset.tv_nsec);
> > +               break;
> >         }
> >         audit_log_end(ab);
> >  }
> > @@ -2571,9 +2583,18 @@ void __audit_fanotify(unsigned int response)
> >
> >  void __audit_tk_injoffset(struct timespec64 offset)
> >  {
> > -       audit_log(audit_context(), GFP_KERNEL, AUDIT_TIME_INJOFFSET,
> > -                 "sec=%lli nsec=%li",
> > -                 (long long)offset.tv_sec, offset.tv_nsec);
> > +       struct audit_context *context = audit_context();
> > +
> > +       context->type = AUDIT_TIME_INJOFFSET;
> > +       memcpy(&context->tk.injoffset, &offset, sizeof(offset));
> > +}
> > +
> > +void __audit_ntp_log(const struct audit_ntp_data *ad)
> > +{
> > +       struct audit_context *context = audit_context();
> > +
> > +       context->type = AUDIT_TIME_ADJNTPVAL;
> > +       memcpy(&context->ntp.data, ad, sizeof(*ad));
> >  }
> >
> >  static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> > @@ -2588,7 +2609,7 @@ static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> >                   "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
> >  }
> >
> > -void __audit_ntp_log(const struct audit_ntp_data *ad)
> > +void __audit_ntp_log_(const struct audit_ntp_data *ad)
> >  {
> >         audit_log_ntp_val(ad, "offset", AUDIT_NTP_OFFSET);
> >         audit_log_ntp_val(ad, "freq",   AUDIT_NTP_FREQ);
> 
> Ooof, *please* don't end a function, or any symbol for that matter,
> with an underscore.

Ok, renamed to be consistent with others called from show_special().

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Casey Schaufler]

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

In the LSM stacking patch set I've implemented a list of
auxiliary data elements for supplemental records. That would
address these issues and provide the general mechanism we're
going to need regardless.

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit

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

[Paul Moore]

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

It seems so much easier just to put both "data" and "injoffset" inside
a single struct within the union, and considering the size of
mq_getsetattr in that same union I doubt the size will increase
significantly (or at all, if my quick estimation stands up).

I believe our best approach is to stay consistent with how we handle
other things similar to this, I don't currently see any reason why the
NTP adjustments would require special handling.

As an aside, how did you test this and not run into this issue?

> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 6efb0bb909d0..8983790ad86a 100644
> > > --- a/kernel/auditsc.c
> > > +++ b/kernel/auditsc.c
> > > @@ -1210,11 +1210,18 @@ static void audit_log_fcaps(struct audit_buffer *ab, struct audit_names *name)
> > >                          from_kuid(&init_user_ns, name->fcap.rootid));
> > >  }
> > >
> > > +void __audit_ntp_log_(const struct audit_ntp_data *ad);
> > > +
> > >  static void show_special(struct audit_context *context, int *call_panic)
> > >  {
> > >         struct audit_buffer *ab;
> > >         int i;
> > >
> > > +       if (context->type == AUDIT_TIME_ADJNTPVAL) {
> > > +               __audit_ntp_log_(&context->ntp.data);
> > > +               return;
> > > +       }
> >
> > Can we find a way to move this down into the main switch statement in
> > show_special() like you did with AUDIT_TIME_INJOFFSET?  This looks
> > *really* hacky to me.  Why should AUDIT_TIME_ADJNTPVAL be different
> > from the other "special" bits?
>
> Agreed, it *looked* hacky to me too.  As previously noted, this was the
> most expedient way to do this due to the unknown number of records being
> generated, minimum of zero (max 6).  show_special allocates an
> audit_buffer before processing the specific record type, assuming it
> exists.  In the case of audit_ntp_data, it may all be equal and not need
> to be logged.
>
> Ideally, the old vs new data should be compared at the time of the call
> from do_adjtimex() to find out if we even need to store that data
> when all old and new values are equal.
>
> So, a bit more pre-processing during the call to avoid storing the
> information if it isn't needed will avoid it at syscall exit.  Are you
> ok with that?

The currently proposed approach is ugly enough that I think it is
worth looking at alternatives, let's see what you can come up with in
the next version and make a decision then.

> > > @@ -2588,7 +2609,7 @@ static void audit_log_ntp_val(const struct audit_ntp_data *ad,
> > >                   "op=%s old=%lli new=%lli", op, val->oldval, val->newval);
> > >  }
> > >
> > > -void __audit_ntp_log(const struct audit_ntp_data *ad)
> > > +void __audit_ntp_log_(const struct audit_ntp_data *ad)
> > >  {
> > >         audit_log_ntp_val(ad, "offset", AUDIT_NTP_OFFSET);
> > >         audit_log_ntp_val(ad, "freq",   AUDIT_NTP_FREQ);
> >
> > Ooof, *please* don't end a function, or any symbol for that matter,
> > with an underscore.
>
> Ok, renamed to be consistent with others called from show_special().

Thanks.

-- 
paul moore
paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit