Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752771AbbGQCDD (ORCPT ); Thu, 16 Jul 2015 22:03:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51582 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750895AbbGQCDA (ORCPT ); Thu, 16 Jul 2015 22:03:00 -0400 Date: Thu, 16 Jul 2015 22:02:56 -0400 From: Richard Guy Briggs To: Paul Moore Cc: linux-audit@redhat.com, linux-kernel@vger.kernel.org, sgrubb@redhat.com, eparis@redhat.com Subject: Re: [PATCH V6 3/4] audit: convert audit_exe to audit_fsnotify Message-ID: <20150717020256.GG32473@madcap2.tricolour.ca> References: <2b37c4f1602bd672ee5b241c0fda57559d4a576a.1436823321.git.rgb@redhat.com> <3828355.kbLCDqrcbq@sifl> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <3828355.kbLCDqrcbq@sifl> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12501 Lines: 375 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 > > > > 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 > > Signed-off-by: Richard Guy Briggs > > 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 > > #include > > -#include > > #include > > #include > > #include > > #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 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/