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

[Richard Guy Briggs]

On 2017-02-04 08:27, Steve Grubb wrote:
> On Friday, February 3, 2017 7:18:58 PM EST Paul Moore wrote:
> > On Tue, Jan 31, 2017 at 3:02 PM, Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:
> > > On 2017-01-31 11:07, Paul Moore wrote:
> > >> 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-rec
> > >> >> > ord-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).
> > > 
> > > Do you see a need to include module initialization arguments?  It sounds
> > > potentially useful to me, but also potentially bandwidth-consuming.  I
> > > have a prototype patch to add the args as one encoded field.  Along with
> > > the addition of this field is the concern about message lengths and
> > > buffer allocations since it is an encoded field that would need twice
> > > the length of the argment text to store in the message.
> > 
> > Argument filtering is surely going to be a mess, just look at the
> > related execve() stuff.  Unless there is a hard requirement I say skip
> > the argument logging for now, we can always add it later.
> 
> Its not a requirement at this point. Just cleanup the current patch and it 
> should be what we needed.

Good.  I've changed the record type from AUDIT_MODULE_INIT to
AUDIT_KERN_MODULE to be generally useful but to avoid confusion with
other types of modules.

> -Steve

- 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