[apparmor] AppArmor kernel audit locks up system

[John Johansen]

thanks for the poke, I have been meaning to get back to this

On 10/18/23 13:03, Paul Moore wrote:
> On Mon, Oct 9, 2023 at 1:51 PM Paul Moore <paul at paul-moore.com> wrote:
>> On Mon, Oct 9, 2023 at 1:41 PM John Johansen
>> <john.johansen at canonical.com> wrote:
>>> On 10/9/23 10:06, Paul Moore wrote:
>>>> I don't think anyone is objecting to resolving this, it's more a
>>>> matter of *how* we can resolve it.
>>>
>>> currently I am see four crazy/stupid paths forward, each with their own
>>> pain points.
> 
> ...
> 
>>> 4. caching a reference in the audit_context as paul has suggested.
>>
>> I don't like this idea, but I'm struggling to come up with something
>> less awful.  Below is a quick, untested patch to describe the concept
>> with code.  It is worth noting that we don't take a mm_struct
>> reference in the io_uring entry point because I'm not sure filtering
>> on the executable file makes much sense there given the async nature
>> of io_uring, however I'm open to comments here (as well as pretty much
>> everything else in this pseudo-patch).
> 
> Looking at this a bit more, I'm now wondering if there is a fifth
> option: call mmget() directly and skip the task_lock().
> 
> Take a look at the move_pages(2) code path:
> 
>   SYSCALL_DEFINE6(move_pages, ...)
>     -> kernel_move_pages(...)
>       -> find_mm_struct(...)
>         -> mmget(...)
> 
> In find_mm_struct(), if the task being manipulated is *not* the
> current task then get_task_mm() is called, which takes task_lock().
> However, if the task being manipulated *is* the current task then the
> task_lock() can be avoided and a direct call to get_mm() is used;
> get_mm() does a simple atomic_inc() without any additional locking.
> 
> What do you think of this approach (untested, copy-n-pasted patch):
> 
hrmmm, I like the idea but the task != current path still suffers from
the issue. If we can verify this case never happens great, otherwise
we either bail on that case or still need to come up with an
alternative.

otherwise it looks good

> diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
> index 65075f1e4ac8..ffd17ad97324 100644
> --- a/kernel/audit_watch.c
> +++ b/kernel/audit_watch.c
> @@ -526,8 +526,19 @@ 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 (always?) comparing @current, but handle both cases */
> +       if (likely(tsk == current)) {
> +               mmget(current->mm);
> +               mm = current->mm;
> +       } else {
> +               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;
> 
> --
> paul-moore.com