On Tue, Sep 22, 2026 at 10:47 AM Jérémy Jean
<[email protected]> wrote:
>
> On 2026-09-22 13:26, Ricardo Robaina wrote:
> > On Mon, Sep 21, 2026 at 5:07 PM Jérémy Jean
> > <[email protected]> wrote:
> >>
> >> kill_rules() removes mixed AUDIT_DIR and AUDIT_EXE rules when an audit
> >> tree is pruned. It drops entry->rule.exe before removing the rule from
> >> the RCU-visible filter lists.
> >>
> >> After a rule has been installed with AUDIT_ADD_RULE, which requires
> >> CAP_AUDIT_CONTROL, removing or moving the watched directory can race
> >> with another task that is still evaluating the rule. In that case,
> >> fsnotify can free the executable mark before the reader reaches
> >> audit_mark_compare(), causing a use-after-free.
> >>
> >> KASAN reports:
> >>
> >> BUG: KASAN: slab-use-after-free in audit_mark_compare+0x8d/0xa0
> >>
> >> Remove the rule from the RCU-visible filter lists first, wait for a
> >> grace
> >> period, and only then drop the executable mark. audit_del_rule()
> >> already
> >> uses this ordering.
> >>
> >> Fixes: 34d99af52ad4 ("audit: implement audit by executable")
> >> Assisted-by: Codex:gpt-5
> >> Signed-off-by: Jérémy Jean <[email protected]>
> >> ---
> >> kernel/audit_tree.c | 5 +++--
> >> 1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> >> index 1ed19b7..864a58b 100644
> >> --- a/kernel/audit_tree.c
> >> +++ b/kernel/audit_tree.c
> >> @@ -553,11 +553,12 @@ static void kill_rules(struct audit_context
> >> *context, struct audit_tree *tree)
> >> if (rule->tree) {
> >> /* not a half-baked one */
> >> audit_tree_log_remove_rule(context, rule);
> >> - if (entry->rule.exe)
> >> - audit_remove_mark(entry->rule.exe);
> >> rule->tree = NULL;
> >> list_del_rcu(&entry->list);
> >> list_del(&entry->rule.list);
> >> + synchronize_rcu();
> >> + if (entry->rule.exe)
> >> + audit_remove_mark(entry->rule.exe);
> >> call_rcu(&entry->rcu, audit_free_rule_rcu);
> >> }
> >> }
> >> --
> >> 2.47.3
> >>
> >>
> >
> > Hi Jérémy,
> >
> > Thanks for troubleshooting and fixing this bug.
> >
> > sashiko's review makes sense, though: synchronize_rcu() inside the
> > loop waits a full grace period per rule, and kill_rules() runs with
> > audit_filter_mutex held, so a tree with many rules stalls other audit
> > config operations. Below is an untested suggestion to use a single
> > grace period instead.
> >
> > diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> > index 1ed19b775912..5a3d7c7ba5ce 100644
> > --- a/kernel/audit_tree.c
> > +++ b/kernel/audit_tree.c
> > @@ -549,18 +549,30 @@ static void kill_rules(struct audit_context
> > *context, struct audit_tree *tree)
> > list_for_each_entry_safe(rule, next, &tree->rules, rlist) {
> > entry = container_of(rule, struct audit_entry, rule);
> >
> > - list_del_init(&rule->rlist);
> > if (rule->tree) {
> > /* not a half-baked one */
> > audit_tree_log_remove_rule(context, rule);
> > - if (entry->rule.exe)
> > - audit_remove_mark(entry->rule.exe);
> > rule->tree = NULL;
> > list_del_rcu(&entry->list);
> > list_del(&entry->rule.list);
> > - call_rcu(&entry->rcu, audit_free_rule_rcu);
> > + } else {
> > + list_del_init(&rule->rlist);
> > }
> > }
> > +
> > + if (list_empty(&tree->rules))
> > + return;
> > +
> > + synchronize_rcu();
> > +
> > + list_for_each_entry_safe(rule, next, &tree->rules, rlist) {
> > + entry = container_of(rule, struct audit_entry, rule);
> > +
> > + list_del_init(&rule->rlist);
> > + if (entry->rule.exe)
> > + audit_remove_mark(entry->rule.exe);
> > + call_rcu(&entry->rcu, audit_free_rule_rcu);
> > + }
> > }
> >
> > /*
>
> Hello Ricardo,
>
> Many thanks for your nice and constructive answer.
>
> Indeed, sashiko's review makes sense and your suggested v2 is a good
> fix.
> I have replayed my reproducer to ensure that it indeeds fix the reported
> bug.
>
> However, I wonder whether the wait should still happen in the case where
> all removed rules have rule->exe == NULL. Doesn't call_rcu() already
> free (kernel/auditfilter.c:96)? If yes, could we skip synchronize_rcu()
> in
> that case? If this question is valid, how about this fix based on yours,
> which
> simply adds a bool to detect whether synchronize_rcu() should be called.
>
> If you think this addition is good, I can send a v2 with that.
It looks like a nice optimization, good catch! synchronize_rcu() is
usually expensive, so calling it only when necessary is an improvement
for sure.
One minor detail, I'd use 'entry->rule.exe' instead of 'rule->exe' in
the need_sync check just for consistency.
>
> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 1ed19b775912..e2cd27b17590 100644
> --- a/kernel/audit_tree.c
> +++ b/kernel/audit_tree.c
> @@ -545,22 +545,38 @@ static void kill_rules(struct audit_context
> *context, struct audit_tree *tree)
> {
> struct audit_krule *rule, *next;
> struct audit_entry *entry;
> + bool need_sync = false;
>
> list_for_each_entry_safe(rule, next, &tree->rules, rlist) {
> entry = container_of(rule, struct audit_entry, rule);
>
> - list_del_init(&rule->rlist);
> if (rule->tree) {
> /* not a half-baked one */
> audit_tree_log_remove_rule(context, rule);
> - if (entry->rule.exe)
> - audit_remove_mark(entry->rule.exe);
> rule->tree = NULL;
> list_del_rcu(&entry->list);
> list_del(&entry->rule.list);
> - call_rcu(&entry->rcu, audit_free_rule_rcu);
> + if (rule->exe)
> + need_sync = true;
> + } else {
> + list_del_init(&rule->rlist);
> }
> }
> +
> + if (list_empty(&tree->rules))
> + return;
> +
> + if (need_sync)
> + synchronize_rcu();
> +
> + list_for_each_entry_safe(rule, next, &tree->rules, rlist) {
> + entry = container_of(rule, struct audit_entry, rule);
> +
> + list_del_init(&rule->rlist);
> + if (entry->rule.exe)
> + audit_remove_mark(entry->rule.exe);
> + call_rcu(&entry->rcu, audit_free_rule_rcu);
> + }
> }
>
> /*
>
> Regards,
> Jérémy
>
--Ricardo
