On Wed, Oct 18, 2023 at 8:22 PM Mateusz Guzik <[email protected]> wrote: > 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).
A link to the original problem report is below, but the quick summary is a capable() check in the do_prlimit() code path. https://lore.kernel.org/audit/[email protected] > >> 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. If there was any other caller I would agree with you, but audit_exe_compare() is the only place where we need this logic and considering the size of the function it seems silly to break out the mm/exe_file logic into a separate function. -- paul-moore.com
