Re: [PATCH] audit: fix exe mark UAF in kill_rules()

[Bradley Morgan]

On 22 September 2026 14:46:55 BST, "Jérémy Jean"
<jeremy.jean@xxxxxxxxxxxxxxxxx> wrote:
>On 2026-09-22 13:26, Ricardo Robaina wrote:
>> On Mon, Sep 21, 2026 at 5:07 PM Jérémy Jean
>> <Jeremy.Jean@xxxxxxxxxxxxxxxxx> 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 <Jeremy.Jean@xxxxxxxxxxxxxxxxx>
>>> ---
>>>  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.
>
>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);
>+       }
> }
>
> /*

LGTM!

Reviewed-by: Bradley Morgan <brads@xxxxxxxxxxxxxx>


What do you reckon Ricardo?


>
>Regards,
>Jérémy

--- Thanks!
"I'm not a very positive person" - Linus torvalds