On 22 September 2026 14:46:55 BST, "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. > >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 <[email protected]> What do you reckon Ricardo? > >Regards, >Jérémy --- Thanks! "I'm not a very positive person" - Linus torvalds
