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

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

[Jérémy Jean]

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@oss.cyber.gouv.fr>
---
 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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] audit: O(N) synchronize_rcu() inside kill_rules() loop
--

commit b7d5542dc56408775e37d42010c82f61694ec4f9
Author: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>

audit: fix exe mark UAF in kill_rules()

This commit addresses a use-after-free when an audit tree is pruned by
reordering the removal of the executable mark after the rule is removed
from the rcu-visible list. It introduces a synchronize_rcu() call before
dropping the executable mark.

> diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c
> index 1ed19b7759129..864a58b6e2caf 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();

[Severity: High]
Does this code introduce a regression by calling synchronize_rcu() inside
the list_for_each_entry_safe() loop in kill_rules()?

Executing synchronize_rcu() inside a list iteration loop causes sequential
grace period waits for every rule being removed. Each wait typically takes
10 to 30 milliseconds. If an audit tree has many rules, this loop could
block the executing thread for seconds or even minutes.

Since kill_rules() is called with the global audit_filter_mutex held (and
often audit_cmd_mutex via audit_ctl_lock()), this prolonged lock-holding
blocks any other process attempting to acquire audit mutexes.

Because unprivileged users can trigger the deletion of watched directories,
could this result in a local denial of service and hung task panics?

Would it be safer to batch this operation by removing all rules from the
rcu list into a local list first, executing a single synchronize_rcu()
outside the loop, and then iterating the local list to remove the marks
and schedule freeing?

> +			if (entry->rule.exe)
> +				audit_remove_mark(entry->rule.exe);
>  			call_rcu(&entry->rcu, audit_free_rule_rcu);
>  		}
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921195921.4174830-2-Jeremy.Jean@oss.cyber.gouv.fr?part=1

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

[Bradley Morgan]

On 21 September 2026 20:59:22 BST, "Jérémy Jean"
<Jeremy.Jean@oss.cyber.gouv.fr> 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

NAK, read sashiko and generate a V2,  and then CC me.

>Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
>---
> 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);
> 		}
> 	}
>

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

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

[Ricardo Robaina]

On Mon, Sep 21, 2026 at 5:07 PM Jérémy Jean
<Jeremy.Jean@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr>
> ---
>  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);
+       }
 }

 /*

--Ricardo

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

[Jérémy Jean]

On 2026-09-22 13:26, Ricardo Robaina wrote:
> On Mon, Sep 21, 2026 at 5:07 PM Jérémy Jean
> <Jeremy.Jean@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr>
>> ---
>>  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);
+       }
  }

  /*

Regards,
Jérémy

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@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr>
>>> ---
>>>  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@mainlining.org>


What do you reckon Ricardo?


>
>Regards,
>Jérémy

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

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

[Ricardo Robaina]

On Tue, Sep 22, 2026 at 10:47 AM Jérémy Jean
<jeremy.jean@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr> 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@oss.cyber.gouv.fr>
> >> ---
> >>  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