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

From: Paul Moore
Date: Tue Jan 31 2017 - 11:07:36 EST

Next message: Oleg Nesterov: "Re: [PATCHv3 01/12] uprobes: split THPs before trying replace them"
Previous message: Noralf TrÃnnes: "[PATCH v3 7/7] drm/tinydrm: Add support for Multi-Inno MI0283QT display"
In reply to: Richard Guy Briggs: "Re: [RFC] [PATCH] audit: log module name on init_module"
Next in thread: Richard Guy Briggs: "Re: [RFC] [PATCH] audit: log module name on init_module"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jan 31, 2017 at 7:36 AM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> On 2017-01-31 06:59, Paul Moore wrote:
>> On Thu, Jan 26, 2017 at 4:21 PM, Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>
>> > ---
>> > 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@xxxxxxxxxx>
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635

--
paul moore
www.paul-moore.com

Next message: Oleg Nesterov: "Re: [PATCHv3 01/12] uprobes: split THPs before trying replace them"
Previous message: Noralf TrÃnnes: "[PATCH v3 7/7] drm/tinydrm: Add support for Multi-Inno MI0283QT display"
In reply to: Richard Guy Briggs: "Re: [RFC] [PATCH] audit: log module name on init_module"
Next in thread: Richard Guy Briggs: "Re: [RFC] [PATCH] audit: log module name on init_module"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]