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? > > 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
