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

[Richard Guy Briggs <rgb@redhat.com>]

On 15/07/16, Paul Moore wrote:
> 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.

Ah, fair enough.  Yeah, I'll do that.

> 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
> 

- 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