On 10/19/23, Paul Moore <[email protected]> wrote: >> The get_task_exe_file() function locks the given task with task_lock() >> which when used inside audit_exe_compare() can cause deadlocks on >> systems that generate audit records when the task_lock() is held. As >> we should be able to safely access current->mm we can drop the call to >> get_task_exe_file() and get a reference to current->mm directly which >> we can then use in a call to get_mm_exe_file() without having to take >> task_lock(). >>
I don't follow what's the deadlock. You mean they hold task lock for current and then call into audit_exe_compare which takes it again? I would argue any calls into the routine with *any* task already locked are fishy at best (I'm assuming there is an established task lock ordering which may be accidentally violated). >> While the audit_exe_compare() function takes an arbitrary task_struct >> parameter, all of the callers outside of fork/clone, call >> audit_exe_compare() to operate on the current task_struct, making it >> the common case. In the fork/clone case, when audit_exe_compare() is >> called on the child task_struct, it should be safe to use the >> get_task_mm() function as no other task should be holding task_lock() >> at this time. >> >> Cc: <[email protected]> >> Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare") >> Reported-by: Andreas Steinmetz <[email protected]> >> Signed-off-by: Paul Moore <[email protected]> >> --- >> kernel/audit_watch.c | 14 +++++++++++++- >> 1 file changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c >> index 65075f1e4ac8..fa3e6ea0e58c 100644 >> --- a/kernel/audit_watch.c >> +++ b/kernel/audit_watch.c >> @@ -526,8 +526,20 @@ int audit_exe_compare(struct task_struct *tsk, struct >> audit_fsnotify_mark *mark) >> struct file *exe_file; >> unsigned long ino; >> dev_t dev; >> + struct mm_struct *mm; >> >> - exe_file = get_task_exe_file(tsk); >> + /* almost always operate on current, exceptions for fork/clone */ >> + if (likely(tsk == current)) { >> + mmget(current->mm); >> + mm = current->mm; >> + } else { >> + /* this can deadlock outside the fork/clone usage (see >> above) */ >> + mm = get_task_mm(tsk); >> + if (!mm) >> + return 0; >> + } >> + exe_file = get_mm_exe_file(mm); >> + mmput(mm); >> if (!exe_file) >> return 0; >> ino = file_inode(exe_file)->i_ino; Assuming concerns expressed above are moot, I would hide the logic in audit_get_task_exe_file or similar. -- Mateusz Guzik <mjguzik gmail.com>
