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

[Richard Guy Briggs <rgb@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 <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