Re: [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify

[Paul Moore <pmoore@redhat.com>]

On Tuesday, July 14, 2015 11:50:25 AM Richard Guy Briggs wrote:
> Instead of just hard coding the ino and dev of the executable we care
> about at the moment the rule is inserted into the kernel, use the new
> audit_fsnotify infrastructure.  This means that if the inode in question
> is unlinked and creat'd (aka updated) the rule will just continue to
> work.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>
> 
> RGB: Clean up exe with similar interface as watch and tree.
> RGB: Clean up audit exe mark just before audit_free_rule() rather than in it
> to avoid mutex in software interrupt context.
> 
> Based-on-code-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Ah, good, the fix that patch 1/4 was hoping for is finally happening :)

Since we're not getting paid by the number of patches in a patch set, I think 
I would prefer to see the fsnotify implementation as the first patch and the 
original crappy exe filter patch merged with this fixup patch.

No in depth comments on this particular patch, I skimmed it but frankly it 
makes much more sense to review this patch once you've merged the two.

> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 95463a2..aee456f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -59,7 +59,7 @@ struct audit_krule {
>  	struct audit_field	*inode_f; /* quick access to an inode field */
>  	struct audit_watch	*watch;	/* associated watch */
>  	struct audit_tree	*tree;	/* associated watched tree */
> -	struct audit_exe	*exe;
> +	struct audit_fsnotify_mark	*exe;
>  	struct list_head	rlist;	/* entry in audit_{watch,tree}.rules list */
>  	struct list_head	list;	/* for AUDIT_LIST* purposes only */
>  	u64			prio;
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 491bd4b..eeaf054 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -51,7 +51,6 @@ enum audit_state {
>  /* Rule lists */
>  struct audit_watch;
>  struct audit_fsnotify_mark;
> -struct audit_exe;
>  struct audit_tree;
>  struct audit_chunk;
> 
> @@ -279,11 +278,8 @@ extern void audit_remove_mark(struct
> audit_fsnotify_mark *audit_mark); extern void audit_remove_mark_rule(struct
> audit_krule *krule);
>  extern int audit_mark_compare(struct audit_fsnotify_mark *mark, unsigned
> long ino, dev_t dev);
> 
> -extern int audit_make_exe_rule(struct audit_krule *krule, char *pathname,
> int len, u32 op); -extern void audit_remove_exe_rule(struct audit_krule
> *krule);
> -extern char *audit_exe_path(struct audit_exe *exe);
>  extern int audit_dupe_exe(struct audit_krule *new, struct audit_krule
> *old); -extern int audit_exe_compare(struct task_struct *tsk, struct
> audit_exe *exe); +extern int audit_exe_compare(struct task_struct *tsk,
> struct audit_fsnotify_mark *mark);
> 
>  #else
>  #define audit_put_watch(w) {}
> @@ -302,36 +298,19 @@ static inline char *audit_mark_path(struct
> audit_fsnotify_mark *mark) }
>  #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;
> -}
> 
> -static inline int audit_make_exe_rule(struct audit_krule *krule, char
> *pathname, int len, u32 op) -{
> -	return -EINVAL;
> -}
> -static inline void audit_remove_exe_rule(struct audit_krule *krule)
> -{
> -	BUG();
> -	return 0;
> -}
> -static inline char *audit_exe_path(struct audit_exe *exe)
> +static inline int audit_exe_compare(struct task_struct *tsk, struct
> audit_fsnotify_mark *mark) {
>  	BUG();
> -	return "";
> +	return -EINVAL;
>  }
> +
>  static inline int audit_dupe_exe(struct audit_krule *new, struct
> audit_krule *old) {
>  	BUG();
> -	return -EINVAL
> -}
> -static inline int audit_exe_compare(struct task_struct *tsk, struct
> audit_exe *exe) -{
> -	BUG();
> -	return 0;
> +	return -EINVAL;
>  }
> +
>  #endif /* CONFIG_AUDIT_WATCH */
> 
>  #ifdef CONFIG_AUDIT_TREE
> diff --git a/kernel/audit_exe.c b/kernel/audit_exe.c
> index d4cc8b5..75ad4f2 100644
> --- a/kernel/audit_exe.c
> +++ b/kernel/audit_exe.c
> @@ -17,93 +17,30 @@
> 
>  #include <linux/kernel.h>
>  #include <linux/audit.h>
> -#include <linux/mutex.h>
>  #include <linux/fs.h>
>  #include <linux/namei.h>
>  #include <linux/slab.h>
>  #include "audit.h"
> 
> -struct audit_exe {
> -	char *pathname;
> -	unsigned long ino;
> -	dev_t dev;
> -};
> -
> -/* Translate a watch string to kernel respresentation. */
> -int audit_make_exe_rule(struct audit_krule *krule, char *pathname, int len,
> u32 op) -{
> -	struct audit_exe *exe;
> -	struct path path;
> -	struct dentry *dentry;
> -	unsigned long ino;
> -	dev_t dev;
> -
> -	if (pathname[0] != '/' || pathname[len-1] == '/')
> -		return -EINVAL;
> -
> -	dentry = kern_path_locked(pathname, &path);
> -	if (IS_ERR(dentry))
> -		return PTR_ERR(dentry);
> -	mutex_unlock(&path.dentry->d_inode->i_mutex);
> -
> -	if (!dentry->d_inode)
> -		return -ENOENT;
> -	dev = dentry->d_inode->i_sb->s_dev;
> -	ino = dentry->d_inode->i_ino;
> -	dput(dentry);
> -
> -	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
> -	if (!exe)
> -		return -ENOMEM;
> -	exe->ino = ino;
> -	exe->dev = dev;
> -	exe->pathname = pathname;
> -	krule->exe = exe;
> -
> -	return 0;
> -}
> -
> -void audit_remove_exe_rule(struct audit_krule *krule)
> -{
> -	struct audit_exe *exe;
> -
> -	exe = krule->exe;
> -	krule->exe = NULL;
> -	kfree(exe->pathname);
> -	kfree(exe);
> -}
> -
> -char *audit_exe_path(struct audit_exe *exe)
> -{
> -	return exe->pathname;
> -}
> -
>  int audit_dupe_exe(struct audit_krule *new, struct audit_krule *old)
>  {
> -	struct audit_exe *exe;
> -
> -	exe = kmalloc(sizeof(*exe), GFP_KERNEL);
> -	if (!exe)
> -		return -ENOMEM;
> +	struct audit_fsnotify_mark *audit_mark;
> +	char *pathname;
> 
> -	exe->pathname = kstrdup(old->exe->pathname, GFP_KERNEL);
> -	if (!exe->pathname) {
> -		kfree(exe);
> -		return -ENOMEM;
> -	}
> +	pathname = audit_mark_path(old->exe);
> 
> -	exe->ino = old->exe->ino;
> -	exe->dev = old->exe->dev;
> -	new->exe = exe;
> +	audit_mark = audit_alloc_mark(new, pathname, strlen(pathname));
> +	if (IS_ERR(audit_mark))
> +		return PTR_ERR(audit_mark);
> +	new->exe = audit_mark;
> 
>  	return 0;
>  }
> 
> -int audit_exe_compare(struct task_struct *tsk, struct audit_exe *exe)
> +int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark
> *mark) {
> -	if (tsk->mm->exe_file->f_inode->i_ino != exe->ino)
> -		return 0;
> -	if (tsk->mm->exe_file->f_inode->i_sb->s_dev != exe->dev)
> -		return 0;
> -	return 1;
> +	unsigned long ino = tsk->mm->exe_file->f_inode->i_ino;
> +	dev_t dev = tsk->mm->exe_file->f_inode->i_sb->s_dev;
> +
> +	return audit_mark_compare(mark, ino, dev);
>  }
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index b0f9877..94ecdab 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -479,6 +479,8 @@ static void kill_rules(struct audit_tree *tree)
>  		if (rule->tree) {
>  			/* not a half-baked one */
>  			audit_tree_log_remove_rule(rule);
> +			if (entry->rule.exe)
> +				audit_remove_mark(entry->rule.exe);
>  			rule->tree = NULL;
>  			list_del_rcu(&entry->list);
>  			list_del(&entry->rule.list);
> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 8f123d7..4aaa393 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -312,6 +312,8 @@ static void audit_update_watch(struct audit_parent
> *parent, list_replace(&oentry->rule.list,
>  					     &nentry->rule.list);
>  			}
> +			if (oentry->rule.exe)
> +				audit_remove_mark(oentry->rule.exe);
> 
>  			audit_watch_log_rule_change(r, owatch, "updated_rules");
> 
> @@ -342,6 +344,8 @@ static void audit_remove_parent_watches(struct
> audit_parent *parent) list_for_each_entry_safe(r, nextr, &w->rules, rlist)
> {
>  			e = container_of(r, struct audit_entry, rule);
>  			audit_watch_log_rule_change(r, w, "remove_rule");
> +			if (e->rule.exe)
> +				audit_remove_mark(e->rule.exe);
>  			list_del(&r->rlist);
>  			list_del(&r->list);
>  			list_del_rcu(&e->list);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index bbb39ec..f65c97f 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -426,6 +426,7 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data, size_t remain = datasz - sizeof(struct
> audit_rule_data);
>  	int i;
>  	char *str;
> +	struct audit_fsnotify_mark *audit_mark;
> 
>  	entry = audit_to_entry_common(data);
>  	if (IS_ERR(entry))
> @@ -557,11 +558,13 @@ static struct audit_entry *audit_data_to_entry(struct
> audit_rule_data *data, }
>  			entry->rule.buflen += f->val;
> 
> -			err = audit_make_exe_rule(&entry->rule, str, f->val, f->op);
> -			if (err) {
> -				kfree(str);
> +			audit_mark = audit_alloc_mark(&entry->rule, str, f->val);
> +			kfree(str);
> +			if (IS_ERR(audit_mark)) {
> +				err = PTR_ERR(audit_mark);
>  				goto exit_free;
>  			}
> +			entry->rule.exe = audit_mark;
>  			break;
>  		}
>  	}
> @@ -575,6 +578,8 @@ exit_nofree:
>  exit_free:
>  	if (entry->rule.tree)
>  		audit_put_tree(entry->rule.tree); /* that's the temporary one */
> +	if (entry->rule.exe)
> +		audit_remove_mark(entry->rule.exe); /* that's the template one */
>  	audit_free_rule(entry);
>  	return ERR_PTR(err);
>  }
> @@ -642,7 +647,7 @@ static struct audit_rule_data
> *audit_krule_to_data(struct audit_krule *krule) case AUDIT_EXE:
>  		case AUDIT_EXE_CHILDREN:
>  			data->buflen += data->values[i] =
> -				audit_pack_string(&bufp, audit_exe_path(krule->exe));
> +				audit_pack_string(&bufp, audit_mark_path(krule->exe));
>  			break;
>  		case AUDIT_LOGINUID_SET:
>  			if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> @@ -710,8 +715,8 @@ static int audit_compare_rule(struct audit_krule *a,
> struct audit_krule *b) case AUDIT_EXE:
>  		case AUDIT_EXE_CHILDREN:
>  			/* both paths exist based on above type compare */
> -			if (strcmp(audit_exe_path(a->exe),
> -				   audit_exe_path(b->exe)))
> +			if (strcmp(audit_mark_path(a->exe),
> +				   audit_mark_path(b->exe)))
>  				return 1;
>  			break;
>  		case AUDIT_UID:
> @@ -842,6 +847,8 @@ struct audit_entry *audit_dupe_rule(struct audit_krule
> *old) break;
>  		}
>  		if (err) {
> +			if (new->exe)
> +				audit_remove_mark(new->exe);
>  			audit_free_rule(entry);
>  			return ERR_PTR(err);
>  		}
> @@ -1008,7 +1015,7 @@ int audit_del_rule(struct audit_entry *entry)
>  		audit_remove_tree_rule(&e->rule);
> 
>  	if (e->rule.exe)
> -		audit_remove_exe_rule(&e->rule);
> +		audit_remove_mark_rule(&e->rule);
> 
>  	list_del_rcu(&e->list);
>  	list_del(&e->rule.list);
> @@ -1113,8 +1120,11 @@ int audit_rule_change(int type, __u32 portid, int
> seq, void *data, WARN_ON(1);
>  	}
> 
> -	if (err || type == AUDIT_DEL_RULE)
> +	if (err || type == AUDIT_DEL_RULE) {
> +		if (entry->rule.exe)
> +			audit_remove_mark(entry->rule.exe);
>  		audit_free_rule(entry);
> +	}
> 
>  	return err;
>  }
> @@ -1406,6 +1416,8 @@ static int update_lsm_rule(struct audit_krule *r)
>  		return 0;
> 
>  	nentry = audit_dupe_rule(r);
> +	if (entry->rule.exe)
> +		audit_remove_mark(entry->rule.exe);
>  	if (IS_ERR(nentry)) {
>  		/* save the first error encountered for the
>  		 * return value */

-- 
paul moore
security @ redhat