On 10/24/23, Paul Moore <[email protected]> wrote: > On Tue, Oct 24, 2023 at 12:47 PM John Johansen > <[email protected]> wrote: >> On 10/24/23 09:14, Paul Moore 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. We >> > resolve this problem with two changes: ignoring those cases where the >> > task being audited is not the current task, and changing our approach >> > to obtaining the executable file struct to not require task_lock(). >> > >> > With the intent of the audit exe filter being to filter on audit events >> > generated by processes started by the specified executable, it makes >> > sense that we would only want to use the exe filter on audit records >> > associated with the currently executing process, e.g. @current. If >> > we are asked to filter records using a non-@current task_struct we can >> > safely ignore the exe filter without negatively impacting the admin's >> > expectations for the exe filter. >> > >> > Knowing that we only have to worry about filtering the currently >> > executing task in audit_exe_compare() we can do away with the >> > task_lock() and call get_mm_exe_file() with @current->mm directly. >> > >> > 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]> >> >> looks good to me >> Reviewed-by: John Johansen <[email protected]> >> >> > --- >> > - v2 >> > * dropped mmget()/mmput() >> > >> > - v1 >> > * initial revision >> > --- >> > kernel/audit_watch.c | 7 ++++++- >> > 1 file changed, 6 insertions(+), 1 deletion(-) >> > >> > diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c >> > index 65075f1e4ac8..99da4ee8e597 100644 >> > --- a/kernel/audit_watch.c >> > +++ b/kernel/audit_watch.c >> > @@ -527,11 +527,16 @@ int audit_exe_compare(struct task_struct *tsk, >> > struct audit_fsnotify_mark *mark) >> > unsigned long ino; >> > dev_t dev; >> > >> > - exe_file = get_task_exe_file(tsk); >> > + /* only do exe filtering if we are recording @current >> > events/records */ >> > + if (tsk != current) >> > + return 0; >> > + >> > + exe_file = get_mm_exe_file(current->mm); > > Hmmm. I don't know why I didn't think of this earlier, but we should > probably protect against @current->mm being NULL, right? >
For the thread to start executing ->mm has to be set. Although I do find it plausible there maybe a corner case during kernel bootstrap and it may be that code can land here with that state, but I can't be arsed to check. Given that stock code has an unintentional property of handling empty mm and this is a bugfix, I am definitely not going to protest adding a check. But I would WARN_ONCE it though. >> > if (!exe_file) >> > return 0; >> > ino = file_inode(exe_file)->i_ino; >> > dev = file_inode(exe_file)->i_sb->s_dev; >> > fput(exe_file); >> > + >> > return audit_mark_compare(mark, ino, dev); >> > } > > -- > paul-moore.com > -- Mateusz Guzik <mjguzik gmail.com>
