apparmor: change how profile replacement update is done

remove the use of replaced by chaining and move to profile invalidation
and lookup to handle task replacement.

Replacement chaining can result in large chains of profiles being pinned
in memory when one profile in the chain is use. With implicit labeling
this will be even more of a problem, so move to a direct lookup method.

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

6 files changed, 125 insertions, 87 deletions

diff --git a/security/apparmor/context.c b/security/apparmor/context.c
index d5af1d15f26d..3064c6ced87c 100644
--- a/security/apparmor/context.c
+++ b/security/apparmor/context.c
@@ -112,9 +112,9 @@ int aa_replace_current_profile(struct aa_profile *profile)
                 aa_clear_task_cxt_trans(cxt);
 
         /* be careful switching cxt->profile, when racing replacement it
-         * is possible that cxt->profile->replacedby is the reference keeping
-         * @profile valid, so make sure to get its reference before dropping
-         * the reference on cxt->profile */
+         * is possible that cxt->profile->replacedby->profile is the reference
+         * keeping @profile valid, so make sure to get its reference before
+         * dropping the reference on cxt->profile */
         aa_get_profile(profile);
         aa_put_profile(cxt->profile);
         cxt->profile = profile;
@@ -175,7 +175,7 @@ int aa_set_current_hat(struct aa_profile *profile, u64 token)
                 abort_creds(new);
                 return -EACCES;
         }
-        cxt->profile = aa_get_profile(aa_newest_version(profile));
+        cxt->profile = aa_get_newest_profile(profile);
         /* clear exec on switching context */
         aa_put_profile(cxt->onexec);
         cxt->onexec = NULL;
@@ -212,14 +212,8 @@ int aa_restore_previous_profile(u64 token)
         }
 
         aa_put_profile(cxt->profile);
-        cxt->profile = aa_newest_version(cxt->previous);
+        cxt->profile = aa_get_newest_profile(cxt->previous);
         BUG_ON(!cxt->profile);
-        if (unlikely(cxt->profile != cxt->previous)) {
-                aa_get_profile(cxt->profile);
-                aa_put_profile(cxt->previous);
-        }
-        /* ref has been transfered so avoid putting ref in clear_task_cxt */
-        cxt->previous = NULL;
         /* clear exec && prev information when restoring to previous context */
         aa_clear_task_cxt_trans(cxt);
 
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 454bcd7f3452..5488d095af6f 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -359,7 +359,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
         cxt = cred_cxt(bprm->cred);
         BUG_ON(!cxt);
 
-        profile = aa_get_profile(aa_newest_version(cxt->profile));
+        profile = aa_get_newest_profile(cxt->profile);
         /*
          * get the namespace from the replacement profile as replacement
          * can change the namespace
@@ -417,7 +417,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 
                 if (!(cp.allow & AA_MAY_ONEXEC))
                         goto audit;
-                new_profile = aa_get_profile(aa_newest_version(cxt->onexec));
+                new_profile = aa_get_newest_profile(cxt->onexec);
                 goto apply;
         }
 
diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
index d44ba5802e3d..6bf65798e5d1 100644
--- a/security/apparmor/include/context.h
+++ b/security/apparmor/include/context.h
@@ -98,7 +98,7 @@ static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
 {
         struct aa_task_cxt *cxt = cred_cxt(cred);
         BUG_ON(!cxt || !cxt->profile);
-        return aa_newest_version(cxt->profile);
+        return cxt->profile;
 }
 
 /**
@@ -152,15 +152,14 @@ static inline struct aa_profile *aa_current_profile(void)
         struct aa_profile *profile;
         BUG_ON(!cxt || !cxt->profile);
 
-        profile = aa_newest_version(cxt->profile);
-        /*
-         * Whether or not replacement succeeds, use newest profile so
-         * there is no need to update it after replacement.
-         */
-        if (unlikely((cxt->profile != profile)))
+        if (PROFILE_INVALID(cxt->profile)) {
+                profile = aa_get_newest_profile(cxt->profile);
                 aa_replace_current_profile(profile);
+                aa_put_profile(profile);
+                cxt = current_cxt();
+        }
 
-        return profile;
+        return cxt->profile;
 }
 
 /**
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index 82487a853353..e9f2baf4467e 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -42,6 +42,8 @@ extern const char *const profile_mode_names[];
 
 #define PROFILE_IS_HAT(_profile) ((_profile)->flags & PFLAG_HAT)
 
+#define PROFILE_INVALID(_profile) ((_profile)->flags & PFLAG_INVALID)
+
 #define on_list_rcu(X) (!list_empty(X) && (X)->prev != LIST_POISON2)
 
 /*
@@ -65,6 +67,7 @@ enum profile_flags {
         PFLAG_USER_DEFINED = 0x20,      /* user based profile - lower privs */
         PFLAG_NO_LIST_REF = 0x40,       /* list doesn't keep profile ref */
         PFLAG_OLD_NULL_TRANS = 0x100,   /* use // as the null transition */
+        PFLAG_INVALID = 0x200,          /* profile replaced/removed */
 
         /* These flags must correspond with PATH_flags */
         PFLAG_MEDIATE_DELETED = 0x10000, /* mediate instead delegate deleted */
@@ -146,6 +149,12 @@ struct aa_policydb {
 
 };
 
+struct aa_replacedby {
+        struct kref count;
+        struct aa_profile __rcu *profile;
+};
+
+
 /* struct aa_profile - basic confinement data
  * @base - base components of the profile (name, refcount, lists, lock ...)
  * @parent: parent of profile
@@ -169,8 +178,7 @@ struct aa_policydb {
  * used to determine profile attachment against unconfined tasks.  All other
  * attachments are determined by profile X transition rules.
  *
- * The @replacedby field is write protected by the profile lock.  Reads
- * are assumed to be atomic.
+ * The @replacedby struct is write protected by the profile lock.
  *
  * Profiles have a hierarchy where hats and children profiles keep
  * a reference to their parent.
@@ -184,14 +192,14 @@ struct aa_profile {
         struct aa_profile __rcu *parent;
 
         struct aa_namespace *ns;
-        struct aa_profile *replacedby;
+        struct aa_replacedby *replacedby;
         const char *rename;
 
         struct aa_dfa *xmatch;
         int xmatch_len;
         enum audit_mode audit;