Date: Sat, 1 Aug 2015 16:03:10 -0400
From: Richard Guy Briggs <rgb@redhat.com>
To: Paul Moore <pmoore@redhat.com>
Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com,
        eparis@redhat.com
Subject: Re: [PATCH V6 2/4] audit: clean simple fsnotify implementation
Message-ID: <20150801200310.GA24289@madcap2.tricolour.ca>
References: <cover.1436823321.git.rgb@redhat.com>
 <267c9708b85459c06f7f5252c51760267a18fa8b.1436823321.git.rgb@redhat.com>
 <2244755.2LgxqKMseS@sifl>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <2244755.2LgxqKMseS@sifl>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 12851
Lines: 348

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 <eparis@redhat.com>
> > 
> > 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 <eparis@redhat.com>
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> 
> 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 <rbriggs@redhat.com>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/