| Message ID | dcd3d6e27c4beb8aec11805faeb4de7013132bd7.1528304204.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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
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);
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(-)