On Tue, Oct 24, 2023 at 2:48 PM Mateusz Guzik <[email protected]> wrote: > > This does fix the bug, and with the caveat I can't vouch for ignoring > non-current in the routine:
Understood, I'm vouching for that bit. > Reviewed-by: Mateusz Guzik <[email protected]> > > Thanks for fixing the bug. Pretty weird it took this long for it to surface. Yeah, I thought the same thing. Regardless, thanks for taking the time to review the fix. > On 10/24/23, Paul Moore <[email protected]> wrote: > > On Tue, Oct 24, 2023 at 2:39 PM 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. 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]> > >> Reviewed-by: John Johansen <[email protected]> > > > > John, I took the liberty of preserving your 'Reviewed-by' tag since I > > felt the current->mm check was a minor addition, but if you feel > > otherwise let me know and I'll remove it. > > > >> Signed-off-by: Paul Moore <[email protected]> > >> --- > >> - v3 > >> * added a !current->mm check > >> - v2 > >> * dropped mmget()/mmput() > >> - v1 > >> * initial revision > >> --- > >> kernel/audit_watch.c | 9 ++++++++- > >> 1 file changed, 8 insertions(+), 1 deletion(-) > > > > -- > > paul-moore.com > > -- > Mateusz Guzik <mjguzik gmail.com> -- paul-moore.com
