From mboxrd@z Thu Jan 1 00:00:00 1970 From: Richard Guy Briggs Subject: Re: [PATCH 13/14] audit: continue fleshing out audit by exe Date: Wed, 25 Jun 2014 16:19:08 -0400 Message-ID: <20140625201908.GA8594@madcap2.tricolour.ca> References: <06994108be2e63f82b290aa2985463db793a4058.1403060033.git.rgb@redhat.com> <20140618100819.2bcb7840@flatline.rdu.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20140618100819.2bcb7840@flatline.rdu.redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-audit-bounces@redhat.com Errors-To: linux-audit-bounces@redhat.com To: Eric Paris Cc: linux-audit@redhat.com List-Id: linux-audit@redhat.com On 14/06/18, Eric Paris wrote: > Whew, lot going on in here.... Yeah, overloaded that commit... I've split it... (more below...) > On Tue, 17 Jun 2014 23:09:48 -0400 > Richard Guy Briggs wrote: > > --- 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... Yeah, I suspected I had misunderstood and was going down the wrong path... > > --- a/kernel/auditfilter.c > > +++ b/kernel/auditfilter.c > > @@ -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() In fact, it looks to me like this should be audit_remove_mark(erule->exe), and get rid of the audit_remove_mark() from audit_del_rule(). This way, rules that get purged due to a watch disappearing will have the mark properly cleaned up rather than orphaned. > > @@ -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?) Actually, it was already 2 because of the call to fsnotify_add_mark() which is bumped up for g_list and i_list (freed by the fsnotify_mark_destroy thread), but yeah, this is unnecessary, I agree. > > @@ -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... Agreed. > > @@ -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 Agreed. > > @@ -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... Understood now. > > @@ -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 */ > > 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... I am starting to suspect that myself. Fixing that is a seperate patch, but understanding that will help get this one right. > > @@ -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... Agreed. So it should free the rule it is seeking in audit_del_rule() and the rule passed to it should be freed after it returns. > 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) Yes, this was renamed to audit_remove_mark_rule(). In there, the entry should be sent to audit_free_rule() after returning from audit_del_rule(). Along the way, I was thinking that fsnotify_destroy_mark_locked() should call fsnotify_put_mark() as well, but I think I've convinced myself that is a bad idea because either there is no other holder of that mark reference, or the reference is still needed for other cleanup. > Yeah, refcounting on marks/trees seems really off to me... Suspecting that helps to understand this... I'll look at the watch and tree count structures somewhat seperately. - 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