apparmor: fix race condition in label replacement

label replacement can result in the need for locking on two separate
trees. Currently this is done by locking the tree to remove and then
the tree to add to.

For compound labels the race can result in the old label proxy
pointing to the the new label that lost the race and that was not
inserted in to the new tree. This does not break mediation, but it does
result in a task that will not update its profile correctly on future
mediation, and that will leak its refcount due to a circular reference
in its proxy, resulting in a memory leak.

Signed-off-by: John Johansen <john.johansen@canonical.com>

1 files changed, 48 insertions, 12 deletions

diff --git a/security/apparmor/label.c b/security/apparmor/label.c
index a165cadf8249..32efef2f617a 100644
--- a/security/apparmor/label.c
+++ b/security/apparmor/label.c
@@ -796,6 +796,44 @@ bool aa_label_remove(struct aa_label *label)
 	return res;
 }
 
+enum ls_lock_class {
+	AA_LS_LOCK_FIRST,
+	AA_LS_LOCK_SECOND,
+};
+
+#define write_lock_irqsave_nested(L, F, SC) write_lock_irqsave(L, F)
+
+static void ns_ls_double_lock(struct aa_ns *ns1, struct aa_ns *ns2,
+	unsigned long *flags)
+{
+	if (likely(ns1 == ns2)) {
+		write_lock_irqsave(&ns1->labels.lock, *flags);
+		return;
+	}
+
+	/* ordered by namespace hierarchy (walked in nesting order in
+	 * labels_update. If at the same level by address order
+	 */
+	if ((ns1->level > ns2->level) ||
+	    (ns1->level == ns2->level && ns1 > ns2))
+		swap(ns1, ns2);
+
+	write_lock_irqsave_nested(&ns1->labels.lock, *flags, AA_LS_LOCK_FIRST);
+	write_lock_nested(&ns2->labels.lock, AA_LS_LOCK_SECOND);
+}
+
+static void ns_ls_double_unlock(struct aa_ns *ns1, struct aa_ns *ns2,
+			     unsigned long flags)
+{
+	if (likely(ns1 == ns2)) {
+		write_unlock_irqrestore(&ns1->labels.lock, flags);
+		return;
+	}
+	/* order doesn't matter on unlock, except flags restore must be last */
+	write_unlock(&ns2->labels.lock);
+	write_unlock_irqrestore(&ns1->labels.lock, flags);
+}
+
 /**
  * aa_label_replace - replace a label @old with a new version @new
  * @old: label to replace
@@ -803,36 +841,34 @@ bool aa_label_remove(struct aa_label *label)
  *
  * Returns: true if @old was in tree and replaced
  *     else @old was not in tree, and @new was not inserted
+ *
+ * replacement can involve two different labelsets so has to be
+ * handled very careful, as a double lock may be required.
  */
 bool aa_label_replace(struct aa_label *old, struct aa_label *new)
 {
+	struct aa_ns *ons = labels_ns(old);
+	struct aa_ns *nns = labels_ns(new);
 	unsigned long flags;
 	bool res;
 
-	if (name_is_shared(old, new) && labels_ns(old) == labels_ns(new)) {
-		write_lock_irqsave(&labels_set(old)->lock, flags);
+	ns_ls_double_lock(ons, nns, &flags);
+	if (ons == nns && name_is_shared(old, new)) {
 		if (old->proxy != new->proxy)
 			__proxy_share(old, new);
 		else
 			__aa_proxy_redirect(old, new);
 		res = __label_replace(old, new);
-		write_unlock_irqrestore(&labels_set(old)->lock, flags);
 	} else {
 		struct aa_label *l;
-		struct aa_labelset *ls = labels_set(old);
 
-		write_lock_irqsave(&ls->lock, flags);
+		/* will redirect old proxy to new */
 		res = __label_remove(old, new);
-		if (labels_ns(old) != labels_ns(new)) {
-			write_unlock_irqrestore(&ls->lock, flags);
-			ls = labels_set(new);
-			write_lock_irqsave(&ls->lock, flags);
-		}
-		l = __label_insert(ls, new, true);
+		l = __label_insert(&nns->labels, new, true);
 		res = (l == new);
-		write_unlock_irqrestore(&ls->lock, flags);
 		aa_put_label(l);
 	}
+	ns_ls_double_unlock(ons, nns, flags);
 
 	return res;
 }