3:19 p.m.
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(a)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(a)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