[apparmor] AppArmor kernel audit locks up system

[Paul Moore]

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):

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