Re: [PATCH 13/14] audit: continue fleshing out audit by exe

Richard Guy Briggs
2014-06-25T20:19:08Z

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 <rgb@redhat.com> 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 <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