by Paul Moore

[permalink] [raw]

Subject: Re: [PATCH V6 1/4] audit: implement audit by executable

On Friday, July 17, 2015 04:46:18 PM Richard Guy Briggs wrote:
> On 15/07/17, Paul Moore wrote:
> > You could do a "based on" or similar tag if you want. I'm honestly not
> > sure what the official tags are beyond signed-off, acked, and reviewed.
> > Those are the only ones I really care about anyway ;)
>
> From Documentation/SubmittingPatches:
> 11) Signed-off-by:
> 12) Acked-by: and Cc:
> 13) Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:

... you would think after referring people to this file I would have a better
idea of what currently lives in there ;)

Thanks for the info.

--
paul moore
security @ redhat

2015-08-01 20:03:23

by Richard Guy Briggs

[permalink] [raw]

Subject: Re: [PATCH V6 2/4] audit: clean simple fsnotify implementation

On 15/07/16, Paul Moore wrote:
> On Tuesday, July 14, 2015 11:50:24 AM Richard Guy Briggs wrote:
> > This is to be used to audit by executable rules, but audit watches
> > should be able to share this code eventually.
> >
> > At the moment the audit watch code is a lot more complex, that code only
> > creates one fsnotify watch per parent directory. That 'audit_parent' in
> > turn has a list of 'audit_watches' which contain the name, ino, dev of
> > the specific object we care about. This just creates one fsnotify watch
> > per object we care about. So if you watch 100 inodes in /etc this code
> > will create 100 fsnotify watches on /etc. The audit_watch code will
> > instead create 1 fsnotify watch on /etc (the audit_parent) and then 100
> > individual watches chained from that fsnotify mark.
> >
> > We should be able to convert the audit_watch code to do one fsnotify
> > mark per watch and simplify things/remove a whole lot of code. After
> > that conversion we should be able to convert the audit_fsnotify code to
> > support that hierarchy if the optimization is necessary.
> >
> > Signed-off-by: Eric Paris <[email protected]>
> >
> > RGB: Move the access to the entry for audit_match_signal() to the beginning
> > of the function in case the entry found is the same one passed in. This
> > will enable it to be used by audit_autoremove_mark_rule().
> > RGB: Rename several "watch" references to "mark".
> > RGB: Rename audit_remove_rule() to audit_autoremove_mark_rule().
> > RGB: Put audit_alloc_mark() arguments in same order as watch, tree and
> > inode. RGB: Remove space from audit log value text in
> > audit_autoremove_mark_rule(). RGB: Explicitly declare prototypes as extern.
> > RGB: Rename audit_remove_mark_rule() called from audit_mark_handle_event()
> > to audit_autoremove_mark_rule() to avoid confusion with
> > audit_remove_{watch,tree}_rule() usage.
> > RGB: Add audit_remove_mark_rule() to provide similar interface as
> > audit_remove_{watch,tree}_rule().
> > RGB: Simplify stubs to defines.
> > RGB: Rename audit_free_fsnotify_mark() to audit_fsnotify_free_mark() in
> > keeping with the naming convention of inotify_free_mark(),
> > dnotify_free_mark(), fanotify_free_mark(), audit_watch_free_mark().
> > RGB: Return -ENOMEM rather than null in case of memory allocation failure
> > for audit_mark. RGB: Rename audit_free_mark() to audit_mark_free() to avoid
> > association with {i,d,fa}notify_free_mark() and audit_watch_free_mark().
>
> Definitely enough changes here to call this your own; credit Eric in the
> description and just stick with your sign off.

Done.

> > Based-on-code-by: Eric Paris <[email protected]>
> > Signed-off-by: Richard Guy Briggs <[email protected]>
>
> Based on the patch description, should this be patch 1/4 instead of 2/4?

Or 1/2 as you have suggested.

> > diff --git a/kernel/Makefile b/kernel/Makefile
> > index a7ea330..397109e 100644
> > --- a/kernel/Makefile
> > +++ b/kernel/Makefile
> > @@ -64,7 +64,7 @@ obj-$(CONFIG_SMP) += stop_machine.o
> > obj-$(CONFIG_KPROBES_SANITY_TEST) += test_kprobes.o
> > obj-$(CONFIG_AUDIT) += audit.o auditfilter.o
> > obj-$(CONFIG_AUDITSYSCALL) += auditsc.o
> > -obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o
> > +obj-$(CONFIG_AUDIT_WATCH) += audit_watch.o audit_exe.o audit_fsnotify.o
> > obj-$(CONFIG_AUDIT_TREE) += audit_tree.o
> > obj-$(CONFIG_GCOV_KERNEL) += gcov/
> > obj-$(CONFIG_KPROBES) += kprobes.o
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 3aca24f..491bd4b 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -286,6 +294,20 @@ extern int audit_exe_compare(struct task_struct *tsk,
> > struct audit_exe *exe); #define audit_watch_path(w) ""
> > #define audit_watch_compare(w, i, d) 0
> >
> > +#define audit_alloc_mark(k, p, l) (ERR_PTR(-EINVAL))
> > +static inline char *audit_mark_path(struct audit_fsnotify_mark *mark)
> > +{
> > + BUG();
> > + return "";
> > +}
> > +#define audit_remove_mark(m) BUG()
> > +#define audit_remove_mark_rule(k) BUG()
> > +static inline int audit_mark_compare(struct audit_fsnotify_mark *mark,
> > unsigned long ino, dev_t dev) +{
> > + BUG();
> > + return 0;
> > +}
>
> No BUG(), we've got enough of those things already.

Cleaned them out...

> > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c
> > new file mode 100644
> > index 0000000..a4e7b16
> > --- /dev/null
> > +++ b/kernel/audit_fsnotify.c
> > @@ -0,0 +1,253 @@
> > +/* audit_fsnotify.c -- tracking inodes
> > + *
> > + * Copyright 2003-2009,2014-2015 Red Hat, Inc.
> > + * Copyright 2005 Hewlett-Packard Development Company, L.P.
> > + * Copyright 2005 IBM Corporation
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
>
> ...
>
> > +static void audit_fsnotify_free_mark(struct fsnotify_mark *mark)
> > +{
> > + struct audit_fsnotify_mark *audit_mark;
> > +
> > + audit_mark = container_of(mark, struct audit_fsnotify_mark, mark);
> > + audit_mark_free(audit_mark);
> > +}
>
> It seems like audit_fsnotify_mark_free() might be more consistent with the
> rest of the naming, yes?

Uh, yes, ok.

> > +#if 0 /* not sure if we need these... */
> > +static void audit_get_mark(struct audit_fsnotify_mark *audit_mark)
> > +{
> > + if (likely(audit_mark))
> > + fsnotify_get_mark(&audit_mark->mark);
> > +}
> > +
> > +static void audit_put_mark(struct audit_fsnotify_mark *audit_mark)
> > +{
> > + if (likely(audit_mark))
> > + fsnotify_put_mark(&audit_mark->mark);
> > +}
> > +#endif
>
> If this code is '#if 0' let's dump it. We need it or we don't, keeping it
> around as dead weight is dangerous.

Agreed. Missed that.

> > +char *audit_mark_path(struct audit_fsnotify_mark *mark)
> > +{
> > + return mark->path;
> > +}
> > +
> > +int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long ino,
> > dev_t dev) +{
> > + if (mark->ino == (unsigned long)-1)
> > + return 0;
> > + return (mark->ino == ino) && (mark->dev == dev);
> > +}
>
> It seems like there should be a #define for -1 inodes, did you check? I
> generally hate magic numbers like this because I'm pretty stupid and tend to
> forget their meaning over time ....

I found nothing obvious, but it does surprise me a bit too...

> Also, at some point we should make (or find?) some generic inode comparison
> function/macro. I'm amazed at home many times I see (i_foo == ino && i_dev ==
> dev).

It would make sense if there were two structs that both included ino and
dev members to call a funciton/macro with pointers to the two structs,
or even one struct and an ino dev tuple, but when the four are discreet,
it starts to lose its utility...

> > +struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule,
> > char *pathname, int len) +{
> > + struct audit_fsnotify_mark *audit_mark;
> > + struct path path;
> > + struct dentry *dentry;
> > + struct inode *inode;
> > + unsigned long ino;
> > + char *local_pathname;
> > + dev_t dev;
> > + int ret;
> > +
> > + if (pathname[0] != '/' || pathname[len-1] == '/')
> > + return ERR_PTR(-EINVAL);
> > +
> > + dentry = kern_path_locked(pathname, &path);
> > + if (IS_ERR(dentry))
> > + return (void *)dentry; /* returning an error */
> > + inode = path.dentry->d_inode;
> > + mutex_unlock(&inode->i_mutex);
> > +
> > + if (!dentry->d_inode) {
> > + ino = (unsigned long)-1;
> > + dev = (unsigned)-1;
> > + } else {
> > + dev = dentry->d_inode->i_sb->s_dev;
> > + ino = dentry->d_inode->i_ino;
> > + }
>
> My comments on the ino and dev variables from the other patch apply here.
>
> > + audit_mark = ERR_PTR(-ENOMEM);
> > + local_pathname = kstrdup(pathname, GFP_KERNEL);
> > + if (!local_pathname)
> > + goto out;
>
> Why not just kstrdup() into audit_mark->path directly? I don't get the
> fascination with local variables. Also, why bother with the strdup if the
> audit_mark malloc is going to fail?

Yeah, I didn't like it either, that's why 4/4 exists...

> > + audit_mark = kzalloc(sizeof(*audit_mark), GFP_KERNEL);
> > + if (unlikely(!audit_mark)) {
> > + kfree(local_pathname);
> > + audit_mark = ERR_PTR(-ENOMEM);
> > + goto out;
> > + }
> > +
> > + fsnotify_init_mark(&audit_mark->mark, audit_fsnotify_free_mark);
> > + audit_mark->mark.mask = AUDIT_FS_EVENTS;
> > + audit_mark->path = local_pathname;
> > + audit_mark->ino = ino;
> > + audit_mark->dev = dev;
> > + audit_mark->rule = krule;
> > +
> > + ret = fsnotify_add_mark(&audit_mark->mark, audit_fsnotify_group, inode,
> > NULL, true); + if (ret < 0) {
> > + audit_mark_free(audit_mark);
> > + audit_mark = ERR_PTR(ret);
> > + }
> > +out:
> > + dput(dentry);
> > + path_put(&path);
> > + return audit_mark;
> > +}
>
> ...
>
> > +static void audit_mark_log_rule_change(struct audit_fsnotify_mark
> > *audit_mark, char *op)
> > +{
>
> That is a lot of letters ... who about audit_mark_log_change()?

I used audit_watch_log_rule_change() and audit_tree_log_remove_rule() as
references... I think it is fine.

> > +/* Update mark data in audit rules based on fsnotify events. */
> > +static int audit_mark_handle_event(struct fsnotify_group *group,
> > + struct inode *to_tell,
> > + struct fsnotify_mark *inode_mark,
> > + struct fsnotify_mark *vfsmount_mark,
> > + u32 mask, void *data, int data_type,
> > + const unsigned char *dname, u32 cookie)
> > +{
> > + struct audit_fsnotify_mark *audit_mark;
> > + struct inode *inode = NULL;
> > +
> > + audit_mark = container_of(inode_mark, struct audit_fsnotify_mark, mark);
> > +
> > + BUG_ON(group != audit_fsnotify_group);
>
> What happens when BUG_ON() is compiled out? Let's back this up with some
> normal error checking if you think this is a real concern. If it isn't a real
> concern, why do we have a BUG_ON()? This doesn't strike me as something that
> is going to be a real problem.

It should not be, if the code is correct. I have no reason to believe
otherwise since we are the only callers and no case should trigger it.

> > + switch (data_type) {
> > + case (FSNOTIFY_EVENT_PATH):
> > + inode = ((struct path *)data)->dentry->d_inode;
> > + break;
> > + case (FSNOTIFY_EVENT_INODE):
> > + inode = (struct inode *)data;
> > + break;
> > + default:
> > + BUG();
> > + return 0;
> > + };
>
> We can do better than BUG() in the default catch-all above. Maybe a
> prink(KERN_ERR ...)?

Oh dang, missed this one in the patchsets I just sent out.

This too was a development debug aid. I don't expect this condition to
be hit, but pr_err(...) would work fine here.

> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index 09041b2..bbb39ec 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -977,7 +977,7 @@ error:
> > }
> >
> > /* Remove an existing rule from filterlist. */
> > -static inline int audit_del_rule(struct audit_entry *entry)
> > +int audit_del_rule(struct audit_entry *entry)
> > {
> > struct audit_entry *e;
> > struct audit_tree *tree = entry->rule.tree;
> > @@ -985,6 +985,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) int ret = 0;
> > #ifdef CONFIG_AUDITSYSCALL
> > int dont_count = 0;
> > + int match = audit_match_signal(entry);
> >
> > /* If either of these, don't count towards total */
> > if (entry->rule.listnr == AUDIT_FILTER_USER ||
> > @@ -1017,7 +1018,7 @@ static inline int audit_del_rule(struct audit_entry
> > *entry) if (!dont_count)
> > audit_n_rules--;
> >
> > - if (!audit_match_signal(entry))
> > + if (!match)
> > audit_signals--;
> > #endif
> > mutex_unlock(&audit_filter_mutex);
>
> Is the bit above worthy of it's own bugfix patch independent of this fsnotify
> implementation, or is it only an issue with this new fsnotify code?

I've split it out and added a note to the cover letter. Er, I should
have included the removal of "static inline" in that split out patch...

> paul moore

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545