Re: [PATCH 13/14] audit: continue fleshing out audit by exe

[Eric Paris <eparis@redhat.com>]

Whew, lot going on in here....

On Tue, 17 Jun 2014 23:09:48 -0400
Richard Guy Briggs <rgb@redhat.com> wrote:

> ---
>  include/linux/audit.h   |    1 +
>  kernel/audit.h          |    1 +
>  kernel/audit_fsnotify.c |   15 +++++++++++++++
>  kernel/auditfilter.c    |   21 ++++++++++++++++++++-
>  4 files changed, 37 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index f2a8044..0bb9ea6 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -43,6 +43,7 @@ struct mq_attr;
>  struct mqstat;
>  struct audit_watch;
>  struct audit_tree;
> +struct audit_fsnotify_mark;
>  struct sk_buff;
>  
>  struct audit_krule {
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 7bf3138..2093c5e 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -285,6 +285,7 @@ extern int audit_watch_compare(struct audit_watch
> *watch, unsigned long ino, dev 
>  struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule
> *krule, char *pathname, int len); char *audit_mark_path(struct
> audit_fsnotify_mark *mark); +int audit_add_mark_rule(struct
> audit_krule *krule, struct list_head **list); void
> audit_remove_mark(struct audit_fsnotify_mark *audit_mark); int
> audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned long
> ino, dev_t dev); diff --git a/kernel/audit_fsnotify.c
> b/kernel/audit_fsnotify.c index efefa16..cc4175a 100644
> --- a/kernel/audit_fsnotify.c
> +++ b/kernel/audit_fsnotify.c
> @@ -161,6 +161,21 @@ static void audit_mark_log_rule_change(struct
> audit_fsnotify_mark *audit_mark, c audit_log_end(ab);
>  }
>  
> +int audit_add_mark_rule(struct audit_krule *krule, struct list_head
> **list) +{
> +	struct audit_fsnotify_mark *audit_mark;
> +	int h, ret = 0;
> +
> +	if (krule->exe)
> +		audit_mark = krule->exe;
> +	else
> +		return -EINVAL;  //XXX
> +
> +	h = audit_hash_ino((u32)audit_mark->ino);
> +	*list = &audit_inode_hash[h];
> +	return ret;
> +}

This would mean that audit_exe rules would trigger at the same times
audit_watch rules trigger.  Not sure if that is the semantics we are
after...


> +
>  static int audit_update_mark(struct audit_fsnotify_mark *audit_mark,
>  			     struct inode *inode)
>  {
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index f40c13b..7b6e892 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -34,6 +34,7 @@
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
>  #include "audit.h"
> +#include <linux/fsnotify_backend.h>
>  
>  /*
>   * Locking model:
> @@ -79,6 +80,8 @@ static inline void audit_free_rule(struct
> audit_entry *e) /* some rules don't have associated watches */
>  	if (erule->watch)
>  		audit_put_watch(erule->watch);
> +	if (erule->exe)
> +		fsnotify_put_mark(erule->exe->mark);

Now this might be a good idea...  This is how marks would get cleaned
up in some error cases, whereas I believe it would get cleaned up from
rule removal in audit_remove_mark()

>  	if (erule->fields)
>  		for (i = 0; i < erule->field_count; i++) {
>  			struct audit_field *f = &erule->fields[i];
> @@ -566,6 +569,7 @@ static struct audit_entry
> *audit_data_to_entry(struct audit_rule_data *data, err =
> PTR_ERR(audit_mark); goto exit_free;
>  			}
> +			fsnotify_get_mark(audit_mark->mark);

So this just took a reference on the mark, so now the refcnt is 2.
Even though there is only 1 user...  (what else references audit_mark
other than entry->rule.exe?)

>  			entry->rule.exe = audit_mark;
>  			break;
>  		}
> @@ -582,6 +586,8 @@ exit_free:
>  		audit_put_watch(entry->rule.watch); /* matches
> initial get */ if (entry->rule.tree)
>  		audit_put_tree(entry->rule.tree); /* that's the
> temporary one */
> +	if (entry->rule.exe)
> +		fsnotify_put_mark(entry->rule.exe->mark); /* matches
> initial get */ audit_free_rule(entry);

ok, so maybe this doesn't panic, since you took and extra reference
above you can put a reference here and then again inside
audit_free_rule().  But the code is still wrong...

>  	return ERR_PTR(err);
>  }
> @@ -866,7 +872,7 @@ static struct audit_entry *audit_find_rule(struct
> audit_entry *entry, if (entry->rule.inode_f) {
>  		h = audit_hash_ino(entry->rule.inode_f->val);
>  		*p = list = &audit_inode_hash[h];
> -	} else if (entry->rule.watch) {
> +	} else if (entry->rule.watch || entry->rule.exe) {

nope

>  		/* we don't know the inode number, so must walk
> entire hash */ for (h = 0; h < AUDIT_INODE_BUCKETS; h++) {
>  			list = &audit_inode_hash[h];
> @@ -900,6 +906,7 @@ static inline int audit_add_rule(struct
> audit_entry *entry) struct audit_entry *e;
>  	struct audit_watch *watch = entry->rule.watch;
>  	struct audit_tree *tree = entry->rule.tree;
> +	struct audit_fsnotify_mark *exe = entry->rule.exe;
>  	struct list_head *list;
>  	int err;
>  #ifdef CONFIG_AUDITSYSCALL
> @@ -943,6 +950,13 @@ static inline int audit_add_rule(struct
> audit_entry *entry) goto error;
>  		}
>  	}
> +	if (exe) {
> +		err = audit_add_mark_rule(&entry->rule, &list);
> +		if (err) {
> +			mutex_unlock(&audit_filter_mutex);
> +			goto error;
> +		}
> +	}

naah, we don't want to find exe rules when we are looking for watch
rules.  This is the list of things we are watching for FS operations
like open, chmod, chown, etc.  Not things we are exec'ing...

>  
>  	entry->rule.prio = ~0ULL;
>  	if (entry->rule.listnr == AUDIT_FILTER_EXIT) {
> @@ -976,6 +990,8 @@ static inline int audit_add_rule(struct
> audit_entry *entry) error:
>  	if (watch)
>  		audit_put_watch(watch); /* tmp watch, matches
> initial get */
> +	if (exe)
> +		fsnotify_put_mark(exe->mark); /* tmp mark, matches
> initial get */ return err;

I'm not so sure the 'watch' code is right here, since any failure is
going to call audit_free_rule(), which will free the watch (and which
you 'fixed' to put the exe...

>  }
>  
> @@ -985,6 +1001,7 @@ int audit_del_rule(struct audit_entry *entry)
>  	struct audit_entry  *e;
>  	struct audit_watch *watch = entry->rule.watch;
>  	struct audit_tree *tree = entry->rule.tree;
> +	struct audit_fsnotify_mark *exe = entry->rule.exe;
>  	struct list_head *list;
>  	int ret = 0;
>  #ifdef CONFIG_AUDITSYSCALL
> @@ -1031,6 +1048,8 @@ out:
>  		audit_put_watch(watch); /* match initial get */
>  	if (tree)
>  		audit_put_tree(tree);	/* that's the temporary
> one */
> +	if (exe)
> +		fsnotify_put_mark(exe->mark);	/* match
> initial get */ 

So audit_del_rule() frees both the rule that was in the kernel being
used on a filter list and the rule that is passed to it used to find
the rule on the filter list?  This is a crap interface and needs
rewritten...

I'm pretty sure that's a bug in my code.

audit_del_rule() has 2 callers.
1) audit_rule_change - which takes userspace input, creates a new rule,
then passes that new rule to audit_del_rule() which find the rule on
the filter list and frees both of them (kinda sorta...)

2) audit_remove_rule() (which badly needs renamed) which actually finds
the rule ON the filter list and passes that to audit_del_rule().
Which finds itself and then frees itself twice (kinda sorta)

Yeah, refcounting on marks/trees seems really off to me...