Re: [RFC] [PATCH] audit: log module name on init_module

On Tue, Jan 31, 2017 at 7:36 AM, Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > On 2017-01-31 06:59, Paul Moore wrote: >> On Thu, Jan 26, 2017 at 4:21 PM, Richard Guy Briggs >> <rgb(a)redhat.com&gt; 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-f... >> >> Consistency nit: capitalize the first letter in the wiki page words >> (see the existing RFE pages) >> >> > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; >> > --- >> > 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 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.

>> > 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(a)redhat.com&gt; > Kernel Security Engineering, Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635