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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Richard Guy Briggs <[email protected]>
>
> 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 <[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
--
Linux-audit mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/linux-audit
