[RFC,ghak90,(was,ghak32),V3,04/10] audit: add support for non-syscall auxiliary records

Submitted by Richard Guy Briggs on June 6, 2018, 4:58 p.m.

Details

Message ID dcd3d6e27c4beb8aec11805faeb4de7013132bd7.1528304204.git.rgb@redhat.com
State New
Series "audit: implement container identifier"
Headers show

Commit Message

Richard Guy Briggs June 6, 2018, 4:58 p.m.
Standalone audit records have the timestamp and serial number generated
on the fly and as such are unique, making them standalone.  This new
function audit_alloc_local() generates a local audit context that will
be used only for a standalone record and its auxiliary record(s).  The
context is discarded immediately after the local associated records are
produced.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/linux/audit.h |  8 ++++++++
 kernel/auditsc.c      | 25 +++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/include/linux/audit.h b/include/linux/audit.h
index ab50985..f549121 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -232,7 +232,9 @@  struct audit_task_info {
 extern struct audit_task_info init_struct_audit;
 extern void __init audit_task_init(void);
 extern int  audit_alloc(struct task_struct *task);
+extern struct audit_context *audit_alloc_local(void);
 extern void audit_free(struct task_struct *task);
+extern void audit_free_context(struct audit_context *context);
 extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
 				  unsigned long a2, unsigned long a3);
 extern void __audit_syscall_exit(int ret_success, long ret_value);
@@ -493,6 +495,12 @@  static inline int audit_alloc(struct task_struct *task)
 {
 	return 0;
 }
+static inline struct audit_context *audit_alloc_local(void)
+{
+	return NULL;
+}
+static inline void audit_free_context(struct audit_context *context)
+{ }
 static inline void audit_free(struct task_struct *task)
 { }
 static inline void audit_syscall_entry(int major, unsigned long a0,
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index cface9d..81c9765 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -916,8 +916,11 @@  static inline void audit_free_aux(struct audit_context *context)
 static inline struct audit_context *audit_alloc_context(enum audit_state state)
 {
 	struct audit_context *context;
+	gfp_t gfpflags;
 
-	context = kzalloc(sizeof(*context), GFP_KERNEL);
+	/* We can be called in atomic context via audit_tg() */
+	gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
+	context = kzalloc(sizeof(*context), gfpflags);
 	if (!context)
 		return NULL;
 	context->state = state;
@@ -993,8 +996,26 @@  struct audit_task_info init_struct_audit = {
 	.ctx = NULL,
 };
 
-static inline void audit_free_context(struct audit_context *context)
+struct audit_context *audit_alloc_local(void)
 {
+	struct audit_context *context;
+
+	if (!audit_ever_enabled)
+		return NULL; /* Return if not auditing. */
+
+	context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
+	if (!context)
+		return NULL;
+	context->serial = audit_serial();
+	context->ctime = current_kernel_time64();
+	context->in_syscall = 1;
+	return context;
+}
+
+void audit_free_context(struct audit_context *context)
+{
+	if (!context)
+		return;
 	audit_free_names(context);
 	unroll_tree_refs(context, NULL, 0);
 	free_tree_refs(context);

Comments

Paul Moore July 20, 2018, 10:14 p.m.
On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> Standalone audit records have the timestamp and serial number generated
> on the fly and as such are unique, making them standalone.  This new
> function audit_alloc_local() generates a local audit context that will
> be used only for a standalone record and its auxiliary record(s).  The
> context is discarded immediately after the local associated records are
> produced.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  include/linux/audit.h |  8 ++++++++
>  kernel/auditsc.c      | 25 +++++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 2 deletions(-)

...

> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -916,8 +916,11 @@ static inline void audit_free_aux(struct audit_context *context)
>  static inline struct audit_context *audit_alloc_context(enum audit_state state)
>  {
>         struct audit_context *context;
> +       gfp_t gfpflags;
>
> -       context = kzalloc(sizeof(*context), GFP_KERNEL);
> +       /* We can be called in atomic context via audit_tg() */
> +       gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;

Instead of trying to guess the proper gfp flags, and likely getting it
wrong at some point (the in_atomic() comment in preempt.h don't give
me the warm fuzzies), why not pass the gfp flags as an argument?

Right now it looks like we would only have two callers: audit_alloc()
and audit_audit_local().  The audit_alloc() invocation would simply
pass GFP_KERNEL and we could allow the audit_alloc_local() callers to
specify the gfp flags when calling audit_alloc_local() (although I
suspect that will always be GFP_ATOMIC since we should only be calling
audit_alloc_local() from interrupt-like context, in all other cases we
should use the audit_context from the current task_struct.

> +       context = kzalloc(sizeof(*context), gfpflags);
>         if (!context)
>                 return NULL;
>         context->state = state;
> @@ -993,8 +996,26 @@ struct audit_task_info init_struct_audit = {
>         .ctx = NULL,
>  };
>
> -static inline void audit_free_context(struct audit_context *context)
> +struct audit_context *audit_alloc_local(void)
>  {

Let's see where this goes, but we may want to rename this slightly to
indicate that this should only be called from interrupt context when
we can't rely on current's audit_context.  Bonus points if we can find
a way to enforce this with a WARN() assertion so we can better catch
abuse.

> +       struct audit_context *context;
> +
> +       if (!audit_ever_enabled)
> +               return NULL; /* Return if not auditing. */
> +
> +       context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> +       if (!context)
> +               return NULL;
> +       context->serial = audit_serial();
> +       context->ctime = current_kernel_time64();
> +       context->in_syscall = 1;

Setting in_syscall is both interesting and a bit troubling, if for no
other reason than I expect most (all?) callers to be in an interrupt
context when audit_alloc_local() is called.  Setting in_syscall would
appear to be conceptually in this case.  Can you help explain why this
is the right thing to do, or necessary to ensure things are handled
correctly?

Looking quickly at the audit code, it seems to only be used on record
and/or syscall termination to end things properly as well as in some
of the PATH record code paths to limit filename collection to actual
syscalls.  However, this was just a quick look so I could be missing
some important points.

> +       return context;
> +}
> +
> +void audit_free_context(struct audit_context *context)
> +{
> +       if (!context)
> +               return;
>         audit_free_names(context);
>         unroll_tree_refs(context, NULL, 0);
>         free_tree_refs(context);
> --
> 1.8.3.1
>
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

--
paul moore
www.paul-moore.com
Richard Guy Briggs July 24, 2018, 7:37 p.m.
On 2018-07-20 18:14, Paul Moore wrote:
> On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > Standalone audit records have the timestamp and serial number generated
> > on the fly and as such are unique, making them standalone.  This new
> > function audit_alloc_local() generates a local audit context that will
> > be used only for a standalone record and its auxiliary record(s).  The
> > context is discarded immediately after the local associated records are
> > produced.
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/audit.h |  8 ++++++++
> >  kernel/auditsc.c      | 25 +++++++++++++++++++++++--
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> ...
> 
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -916,8 +916,11 @@ static inline void audit_free_aux(struct audit_context *context)
> >  static inline struct audit_context *audit_alloc_context(enum audit_state state)
> >  {
> >         struct audit_context *context;
> > +       gfp_t gfpflags;
> >
> > -       context = kzalloc(sizeof(*context), GFP_KERNEL);
> > +       /* We can be called in atomic context via audit_tg() */
> > +       gfpflags = (in_atomic() || irqs_disabled()) ? GFP_ATOMIC : GFP_KERNEL;
> 
> Instead of trying to guess the proper gfp flags, and likely getting it
> wrong at some point (the in_atomic() comment in preempt.h don't give
> me the warm fuzzies), why not pass the gfp flags as an argument?
> 
> Right now it looks like we would only have two callers: audit_alloc()
> and audit_audit_local().  The audit_alloc() invocation would simply
> pass GFP_KERNEL and we could allow the audit_alloc_local() callers to
> specify the gfp flags when calling audit_alloc_local() (although I
> suspect that will always be GFP_ATOMIC since we should only be calling
> audit_alloc_local() from interrupt-like context, in all other cases we
> should use the audit_context from the current task_struct.

Ok, I'll explicitly pass it in.

> > +       context = kzalloc(sizeof(*context), gfpflags);
> >         if (!context)
> >                 return NULL;
> >         context->state = state;
> > @@ -993,8 +996,26 @@ struct audit_task_info init_struct_audit = {
> >         .ctx = NULL,
> >  };
> >
> > -static inline void audit_free_context(struct audit_context *context)
> > +struct audit_context *audit_alloc_local(void)
> >  {
> 
> Let's see where this goes, but we may want to rename this slightly to
> indicate that this should only be called from interrupt context when
> we can't rely on current's audit_context.  Bonus points if we can find
> a way to enforce this with a WARN() assertion so we can better catch
> abuse.

I'll see what I can come up with.

> > +       struct audit_context *context;
> > +
> > +       if (!audit_ever_enabled)
> > +               return NULL; /* Return if not auditing. */
> > +
> > +       context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> > +       if (!context)
> > +               return NULL;
> > +       context->serial = audit_serial();
> > +       context->ctime = current_kernel_time64();
> > +       context->in_syscall = 1;
> 
> Setting in_syscall is both interesting and a bit troubling, if for no
> other reason than I expect most (all?) callers to be in an interrupt
> context when audit_alloc_local() is called.  Setting in_syscall would
> appear to be conceptually in this case.  Can you help explain why this
> is the right thing to do, or necessary to ensure things are handled
> correctly?

I'll admit this is cheating a bit, but seemed harmless.  It is needed so
that auditsc_get_stamp() from audit_get_stamp() from audit_log_start()
doesn't bail on me without giving me its already assigned time and
serial values rather than generating a new one.  I did look to see if
there were any other undesireable side effects and found none, so I'm
tmepted to rename the ->in_syscall to something a bit more helpful.  I
could add a new audit_context structure member to make
auditsc_get_stamp() co-operative, but this seems wasteful and
unnecessary.

> Looking quickly at the audit code, it seems to only be used on record
> and/or syscall termination to end things properly as well as in some
> of the PATH record code paths to limit filename collection to actual
> syscalls.  However, this was just a quick look so I could be missing
> some important points.
> 
> > +       return context;
> > +}
> > +
> > +void audit_free_context(struct audit_context *context)
> > +{
> > +       if (!context)
> > +               return;
> >         audit_free_names(context);
> >         unroll_tree_refs(context, NULL, 0);
> >         free_tree_refs(context);
> > --
> > 1.8.3.1
> >
> > --
> > Linux-audit mailing list
> > Linux-audit@redhat.com
> > https://www.redhat.com/mailman/listinfo/linux-audit
> 
> --
> paul moore
> www.paul-moore.com

- 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
Paul Moore July 24, 2018, 9:57 p.m.
On Tue, Jul 24, 2018 at 3:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-07-20 18:14, Paul Moore wrote:
> > On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > Standalone audit records have the timestamp and serial number generated
> > > on the fly and as such are unique, making them standalone.  This new
> > > function audit_alloc_local() generates a local audit context that will
> > > be used only for a standalone record and its auxiliary record(s).  The
> > > context is discarded immediately after the local associated records are
> > > produced.
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > >  include/linux/audit.h |  8 ++++++++
> > >  kernel/auditsc.c      | 25 +++++++++++++++++++++++--
> > >  2 files changed, 31 insertions(+), 2 deletions(-)

...

> > > +       struct audit_context *context;
> > > +
> > > +       if (!audit_ever_enabled)
> > > +               return NULL; /* Return if not auditing. */
> > > +
> > > +       context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> > > +       if (!context)
> > > +               return NULL;
> > > +       context->serial = audit_serial();
> > > +       context->ctime = current_kernel_time64();
> > > +       context->in_syscall = 1;
> >
> > Setting in_syscall is both interesting and a bit troubling, if for no
> > other reason than I expect most (all?) callers to be in an interrupt
> > context when audit_alloc_local() is called.  Setting in_syscall would
> > appear to be conceptually in this case.  Can you help explain why this
> > is the right thing to do, or necessary to ensure things are handled
> > correctly?
>
> I'll admit this is cheating a bit, but seemed harmless.  It is needed so
> that auditsc_get_stamp() from audit_get_stamp() from audit_log_start()
> doesn't bail on me without giving me its already assigned time and
> serial values rather than generating a new one.  I did look to see if
> there were any other undesireable side effects and found none, so I'm
> tmepted to rename the ->in_syscall to something a bit more helpful.  I
> could add a new audit_context structure member to make
> auditsc_get_stamp() co-operative, but this seems wasteful and
> unnecessary.

That's what I suspected.

Let's look into renaming the "in_syscall" field, it borderline
confusing now, and hijacking it for something which is very obviously
not "in syscall" is A Very Bad Thing.
Richard Guy Briggs July 26, 2018, 2:30 p.m.
On 2018-07-24 17:57, Paul Moore wrote:
> On Tue, Jul 24, 2018 at 3:40 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-20 18:14, Paul Moore wrote:
> > > On Wed, Jun 6, 2018 at 1:01 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Standalone audit records have the timestamp and serial number generated
> > > > on the fly and as such are unique, making them standalone.  This new
> > > > function audit_alloc_local() generates a local audit context that will
> > > > be used only for a standalone record and its auxiliary record(s).  The
> > > > context is discarded immediately after the local associated records are
> > > > produced.
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > >  include/linux/audit.h |  8 ++++++++
> > > >  kernel/auditsc.c      | 25 +++++++++++++++++++++++--
> > > >  2 files changed, 31 insertions(+), 2 deletions(-)
> 
> ...
> 
> > > > +       struct audit_context *context;
> > > > +
> > > > +       if (!audit_ever_enabled)
> > > > +               return NULL; /* Return if not auditing. */
> > > > +
> > > > +       context = audit_alloc_context(AUDIT_RECORD_CONTEXT);
> > > > +       if (!context)
> > > > +               return NULL;
> > > > +       context->serial = audit_serial();
> > > > +       context->ctime = current_kernel_time64();
> > > > +       context->in_syscall = 1;
> > >
> > > Setting in_syscall is both interesting and a bit troubling, if for no
> > > other reason than I expect most (all?) callers to be in an interrupt
> > > context when audit_alloc_local() is called.  Setting in_syscall would
> > > appear to be conceptually in this case.  Can you help explain why this
> > > is the right thing to do, or necessary to ensure things are handled
> > > correctly?
> >
> > I'll admit this is cheating a bit, but seemed harmless.  It is needed so
> > that auditsc_get_stamp() from audit_get_stamp() from audit_log_start()
> > doesn't bail on me without giving me its already assigned time and
> > serial values rather than generating a new one.  I did look to see if
> > there were any other undesireable side effects and found none, so I'm
> > tmepted to rename the ->in_syscall to something a bit more helpful.  I
> > could add a new audit_context structure member to make
> > auditsc_get_stamp() co-operative, but this seems wasteful and
> > unnecessary.
> 
> That's what I suspected.
> 
> Let's look into renaming the "in_syscall" field, it borderline
> confusing now, and hijacking it for something which is very obviously
> not "in syscall" is A Very Bad Thing.

Ok, looking more carefully, I'm not going to touch in_syscall, since it
does more than I remember discovering when investigating why the
existing stamp wasn't being used.  I don't want to change the existing
behaviour.  I'll somewhat reluctantly grow the context struct and add a
"local" boolean to it so that auditsc_get_stamp knows to use the
existing stamp in both the in_syscall and local cases.

> 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