apparmor: grab ns lock and refresh when looking up changehat child profiles

There was a race condition involving change_hat and profile replacement in
which replacement of the parent profile during a changehat operation could
result in the list of children becoming empty and the changehat operation
failing. To prevent this:
 - grab the namespace lock until we've built the hat transition, and
 - use aa_get_newest_profile to avoid using stale profile objects.

Link: https://bugs.launchpad.net/bugs/2139664
Fixes: 89dbf1962aa63 ("apparmor: move change_hat mediation to using labels")
Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 6748ac74b060bd58924f54e7aef159b040b0364f..c2652165a5886d5f2aa85b0023c2a5c49db42226 100644 (file)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -12,6 +12,7 @@
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/mount.h>
+#include <linux/mutex.h>
 #include <linux/syscalls.h>
 #include <linux/personality.h>
 #include <linux/xattr.h>
@@ -1109,6 +1110,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
                                   int count, int flags)
 {
        struct aa_profile *profile, *root, *hat = NULL;
+       struct aa_ns *ns, *new_ns;
        struct aa_label *new;
        struct label_it it;
        bool sibling = false;
@@ -1119,6 +1121,32 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
        AA_BUG(!hats);
        AA_BUG(count < 1);
 
+       /*
+        * Acquire the newest label and then hold the lock until we choose a
+        * hat, so that profile replacement doesn't atomically truncate the
+        * list of potential hats. Because we are getting the namespaces from
+        * the profiles and label, we can rely on the namespaces being live
+        * and avoid incrementing their refcounts while grabbing the lock.
+        */
+       label = aa_get_label(label);
+       ns = labels_ns(label);
+
+retry:
+       mutex_lock_nested(&ns->lock, ns->level);
+       if (label_is_stale(label)) {
+               new = aa_get_newest_label(label);
+               new_ns = labels_ns(new);
+               if (new_ns != ns) {
+                       aa_put_label(new);
+                       mutex_unlock(&ns->lock);
+                       ns = new_ns;
+                       label = new;
+                       goto retry;
+               }
+               aa_put_label(label);
+               label = new;
+       }
+
        if (PROFILE_IS_HAT(labels_profile(label)))
                sibling = true;
 
@@ -1127,7 +1155,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred,
                name = hats[i];
                label_for_each_in_scope(it, labels_ns(label), label, profile) {
                        if (sibling && PROFILE_IS_HAT(profile)) {
-                               root = aa_get_profile_rcu(&profile->parent);
+                               root = aa_get_profile(profile->parent);
                        } else if (!sibling && !PROFILE_IS_HAT(profile)) {
                                root = aa_get_profile(profile);
                        } else {        /* conflicting change type */
@@ -1187,6 +1215,7 @@ fail:
                                      GLOBAL_ROOT_UID, info, error);
                }
        }
+       mutex_unlock(&ns->lock);
        return ERR_PTR(error);
 
 build:
@@ -1199,7 +1228,7 @@ build:
                error = -ENOMEM;
                goto fail;
        } /* else if (IS_ERR) build_change_hat has logged error so return new */
-
+       mutex_unlock(&ns->lock);
        return new;
 }