Messages in this thread | | | Date | Wed, 1 Feb 2017 09:30:33 +0100 | From | Steve Grubb <> | Subject | Re: [RFC] [PATCH] audit: log module name on init_module |
| |
On Tue, 31 Jan 2017 11:07:24 -0500 Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jan 31, 2017 at 7:36 AM, Richard Guy Briggs <rgb@redhat.com> > wrote: > > On 2017-01-31 06:59, Paul Moore wrote: > >> On Thu, Jan 26, 2017 at 4:21 PM, Richard Guy Briggs > >> <rgb@redhat.com> wrote: > >> > This adds a new auxiliary record MODULE_INIT to the SYSCALL > >> > event. > >> > > >> > We get finit_module for free since it made most sense to hook > >> > this in to load_module(). > >> > > >> > https://github.com/linux-audit/audit-kernel/issues/7 > >> > https://github.com/linux-audit/audit-kernel/wiki/RFE-Module-load-record-format > >> > >> Consistency nit: capitalize the first letter in the wiki page words > >> (see the existing RFE pages) > >> > >> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > >> > --- > >> > include/linux/audit.h | 12 ++++++++++++ > >> > include/uapi/linux/audit.h | 1 + > >> > kernel/audit.h | 3 +++ > >> > kernel/auditsc.c | 20 ++++++++++++++++++++ > >> > kernel/module.c | 5 ++++- > >> > 5 files changed, 40 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/include/linux/audit.h b/include/linux/audit.h > >> > index 2be99b2..7bb23d5 100644 > >> > --- a/include/linux/audit.h > >> > +++ b/include/linux/audit.h > >> > @@ -360,6 +360,7 @@ extern int __audit_log_bprm_fcaps(struct > >> > linux_binprm *bprm, const struct cred *old); > >> > extern void __audit_log_capset(const struct cred *new, const > >> > struct cred *old); extern void __audit_mmap_fd(int fd, int > >> > flags); +extern void __audit_module_init(char *name); > >> > > >> > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > >> > { > >> > @@ -450,6 +451,12 @@ static inline void audit_mmap_fd(int fd, > >> > int flags) __audit_mmap_fd(fd, flags); > >> > } > >> > > >> > +static inline void audit_module_init(char *name) > >> > +{ > >> > + if (!audit_dummy_context()) > >> > + __audit_module_init(name); > >> > +} > >> > >> More on this below, but I was expecting the function above to named > >> audit_log_kern_module(), or something similar. > > > > Ok fair enough, I had mis-understood your previous point. > > I probably could have been more specific too. > > > Any comment on the new record format? > > Not really, it's just the single field so it's kinda hard to have > anything meaningful to say. We obviously need to worry about the > field name, but I'll let Steve speak to that as that likely means more > to him than it does to me. From my perspective, "name" seems > perfectly reasonable, especially since it is in the context of a > module specific record (no real worries about it being ambiguous).
I'm fine with "name".
> I guess that does bring up the question of the record name, right now > it is AUDIT_MODULE_INIT ... do we want it to be specific to init/load > or do we want it more generic with an "op=" field (you mentioned some > thoughts both ways in the GH issue). I'm not sure I care too much > either way, but once again I imagine Steve may have a preference for > one over the other.
If this does not hook delete_module, then I see no reason for an op= field. But even if it were to, the operation is implicit in the syscall name. So, I don't think anything else is needed.
-Steve
> >> > extern int audit_n_rules; > >> > extern int audit_signals; > >> > #else /* CONFIG_AUDITSYSCALL */ > >> > @@ -561,6 +568,11 @@ static inline void audit_log_capset(const > >> > struct cred *new, { } > >> > static inline void audit_mmap_fd(int fd, int flags) > >> > { } > >> > + > >> > +static inline void audit_module_init(char *name) > >> > +{ > >> > +} > >> > + > >> > static inline void audit_ptrace(struct task_struct *t) > >> > { } > >> > #define audit_n_rules 0 > >> > diff --git a/include/uapi/linux/audit.h > >> > b/include/uapi/linux/audit.h index 3f24110..4a328b4 100644 > >> > --- a/include/uapi/linux/audit.h > >> > +++ b/include/uapi/linux/audit.h > >> > @@ -111,6 +111,7 @@ > >> > #define AUDIT_PROCTITLE 1327 /* Proctitle > >> > emit event */ #define AUDIT_FEATURE_CHANGE 1328 /* audit > >> > log listing feature changes */ #define AUDIT_REPLACE > >> > 1329 /* Replace auditd if this packet unanswerd */ +#define > >> > AUDIT_MODULE_INIT 1330 /* Module init event */ > >> > > >> > #define AUDIT_AVC 1400 /* SE Linux avc denial > >> > or grant */ #define AUDIT_SELINUX_ERR 1401 /* Internal > >> > SE Linux Errors */ diff --git a/kernel/audit.h b/kernel/audit.h > >> > index 431444c..144b7eb 100644 > >> > --- a/kernel/audit.h > >> > +++ b/kernel/audit.h > >> > @@ -199,6 +199,9 @@ struct audit_context { > >> > struct { > >> > int argc; > >> > } execve; > >> > + struct { > >> > + char *name; > >> > + } module; > >> > }; > >> > int fds[2]; > >> > struct audit_proctitle proctitle; > >> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > >> > index bb5f504..3e12678 100644 > >> > --- a/kernel/auditsc.c > >> > +++ b/kernel/auditsc.c > >> > @@ -1172,6 +1172,14 @@ out: > >> > kfree(buf_head); > >> > } > >> > > >> > +static void audit_log_kern_module(struct audit_context *context, > >> > + struct audit_buffer **ab) > >> > +{ > >> > + audit_log_format(*ab, " name="); > >> > + audit_log_untrustedstring(*ab, context->module.name); > >> > + kfree(context->module.name); > >> > +} > >> > + > >> > static void show_special(struct audit_context *context, int > >> > *call_panic) { > >> > struct audit_buffer *ab; > >> > @@ -1268,6 +1276,9 @@ static void show_special(struct > >> > audit_context *context, int *call_panic) case AUDIT_EXECVE: { > >> > audit_log_execve_info(context, &ab); > >> > break; } > >> > + case AUDIT_MODULE_INIT: > >> > + audit_log_kern_module(context, &ab); > >> > + break; > >> > >> With the exception of AUDIT_EXECVE everything else in > >> show_special() is simply open coded inside the show_special() > >> function; considering that audit_log_kern_module() is trivial it > >> seems like a separate function isn't really needed here. > >> > >> > } > >> > audit_log_end(ab); > >> > } > >> > @@ -2368,6 +2379,15 @@ void __audit_mmap_fd(int fd, int flags) > >> > context->type = AUDIT_MMAP; > >> > } > >> > > >> > +void __audit_module_init(char *name) > >> > +{ > >> > + struct audit_context *context = current->audit_context; > >> > + > >> > + context->module.name = kmalloc(strlen(name) + 1, > >> > GFP_KERNEL); > >> > + strcpy(context->module.name, name); > >> > + context->type = AUDIT_MODULE_INIT; > >> > +} > >> > + > >> > static void audit_log_task(struct audit_buffer *ab) > >> > { > >> > kuid_t auid, uid; > >> > diff --git a/kernel/module.c b/kernel/module.c > >> > index 529efae..678407e 100644 > >> > --- a/kernel/module.c > >> > +++ b/kernel/module.c > >> > @@ -61,6 +61,7 @@ > >> > #include <linux/pfn.h> > >> > #include <linux/bsearch.h> > >> > #include <linux/dynamic_debug.h> > >> > +#include <linux/audit.h> > >> > #include <uapi/linux/module.h> > >> > #include "module-internal.h" > >> > > >> > @@ -3593,6 +3594,8 @@ static int load_module(struct load_info > >> > *info, const char __user *uargs, goto free_copy; > >> > } > >> > > >> > + audit_module_init(mod->name); > >> > + > >> > /* Reserve our place in the list. */ > >> > err = add_unformed_module(mod); > >> > if (err) > >> > @@ -3681,7 +3684,7 @@ static int load_module(struct load_info > >> > *info, const char __user *uargs, mod->name, after_dashes); > >> > } > >> > > >> > - /* Link in to syfs. */ > >> > + /* Link in to sysfs. */ > >> > err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp); > >> > if (err < 0) > >> > goto coming_cleanup; > >> > >> paul moore > > > > - RGB > > > > -- > > Richard Guy Briggs <rgb@redhat.com> > > Kernel Security Engineering, Base Operating Systems, Red Hat > > Remote, Ottawa, Canada > > Voice: +1.647.777.2635, Internal: (81) 32635 > > >
| |