Re: [PATCH] audit: use mmget() instead of get_task_exe_file() when auditing @current

[Paul Moore]

Updating Mateusz's email.

On Wed, Oct 18, 2023 at 6:20 PM Paul Moore <p...@paul-moore.com> 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().
>
> 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: <sta...@vger.kernel.org>
> Fixes: 5efc244346f9 ("audit: fix exe_file access in audit_exe_compare")
> Reported-by: Andreas Steinmetz <anstei...@googlemail.com>
> Signed-off-by: Paul Moore <p...@paul-moore.com>
> ---
>  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;
> --
> 2.42.0

-- 
paul-moore.com