[apparmor] AppArmor kernel audit locks up system

[Paul Moore]

On Wed, Oct 18, 2023 at 4:34 PM John Johansen
<john.johansen at canonical.com> wrote:
> 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.

It shouldn't, the only time we ever really operate on something other
than @current is when a fork/clone happens and we are filtering on the
new child process.  At least that used to be the case, I can't imagine
someone would audit something other than @current (not sure you could
with respect to this stuff?), but I guess it couldn't hurt to double
check on the current code base.

> otherwise it looks good

It just passed through the audit and SELinux test suites without
problem, so I'm hopeful this might be a viable solution.

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

-- 
paul-moore.com