[apparmor] AppArmor kernel audit locks up system

[Paul Moore]

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.
>
>
> 1. lift the capable() and security_task_setrlimit() calls out of the lock.
>
> this might be possible, it should be fine for capable() but does open
> a potential race window for security_task_setrlimit() if the LSM hooks
> mediation looks at the tasks current resource value.

Ignoring the race for a moment, I worry that even if we solve it for
this particular case it could easily come back to bite us somewhere
else.

> 2. rework get_task_exe_file() to not need the task lock. That looks
> like a major under taking, and I don't currently see it as viable.

Agreed.

> 3. get the task lock switch to a recursive spin_lock. Another large
> piece of work that I don't currently see as viable.

Agreed.

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

WARNING: this is a cut-n-paste so it's probably mangled.

diff --git a/kernel/audit.h b/kernel/audit.h
index a60d2840559e..eac470aaca4f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -146,6 +146,8 @@ struct audit_context {
       u32                 target_sid;
       char                target_comm[TASK_COMM_LEN];

+       struct mm_struct    *mm;
+
       struct audit_tree_refs *trees, *first_trees;
       struct list_head killed_trees;
       int tree_count;
diff --git a/kernel/audit_watch.c b/kernel/audit_watch.c
index 65075f1e4ac8..a619394530bd 100644
--- a/kernel/audit_watch.c
+++ b/kernel/audit_watch.c
@@ -526,8 +526,12 @@ 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);
+       mm = tsk->audit_context->mm;
+       if (!mm)
+               return 0;
+       exe_file = get_mm_exe_file(mm);
       if (!exe_file)
               return 0;
       ino = file_inode(exe_file)->i_ino;
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 21d2fa815e78..edeff28a4bab 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1019,6 +1019,10 @@ static void audit_reset_context(struct audit_context *ctx
)
       ctx->target_sessionid = 0;
       ctx->target_sid = 0;
       ctx->target_comm[0] = '\0';
+       if (ctx->mm) {
+               mmput(ctx->mm);
+               ctx->mm = NULL;
+       }
       unroll_tree_refs(ctx, NULL, 0);
       WARN_ON(!list_empty(&ctx->killed_trees));
       audit_free_module(ctx);
@@ -2035,6 +2039,9 @@ void __audit_syscall_entry(int major, unsigned long a1, un
signed long a2,
                       return;
       }

+       /* get mm as it requires task_lock() which may not be safe later */
+       context->mm = get_task_mm(current);
+
       context->arch       = syscall_get_arch(current);
       context->major      = major;
       context->argv[0]    = a1;

-- 
paul-moore.com