apparmor: fix aa_xattrs_match() may sleep while holding a RCU lock

aa_xattrs_match() is unfortunately calling vfs_getxattr_alloc() from a
context protected by an rcu_read_lock. This can not be done as
vfs_getxattr_alloc() may sleep regardles of the gfp_t value being
passed to it.

Fix this by breaking the rcu_read_lock on the policy search when the
xattr match feature is requested and restarting the search if a policy
changes occur.

Fixes: 8e51f9087f40 ("apparmor: Add support for attaching profiles via xattr, presence and value")
Reported-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Reported-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 09996f2552ee45b1503d3fae321032a90e03cc14..47aff8700547d2434e2b4beeffa89dc59f8b1e96 100644 (file)
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -623,7 +623,7 @@ static __poll_t ns_revision_poll(struct file *file, poll_table *pt)
 
 void __aa_bump_ns_revision(struct aa_ns *ns)
 {
-       ns->revision++;
+       WRITE_ONCE(ns->revision, ns->revision + 1);
        wake_up_interruptible(&ns->wait);
 }
 

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 9be7ccb8379edff7b2261ed87c0ec90a4ebe2670..6ceb74e0f7895548c5faaa6f95b7eb8c11367e87 100644 (file)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -317,6 +317,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm,
 
        if (!bprm || !profile->xattr_count)
                return 0;
+       might_sleep();
 
        /* transition from exec match to xattr set */
        state = aa_dfa_null_transition(profile->xmatch, state);
@@ -361,10 +362,11 @@ out:
 }
 
 /**
- * __attach_match_ - find an attachment match
+ * find_attach - do attachment search for unconfined processes
  * @bprm - binprm structure of transitioning task
- * @name - to match against  (NOT NULL)
+ * @ns: the current namespace  (NOT NULL)
  * @head - profile list to walk  (NOT NULL)
+ * @name - to match against  (NOT NULL)
  * @info - info message if there was an error (NOT NULL)
  *
  * Do a linear search on the profiles in the list.  There is a matching
@@ -374,12 +376,11 @@ out:
  *
  * Requires: @head not be shared or have appropriate locks held
  *
- * Returns: profile or NULL if no match found
+ * Returns: label or NULL if no match found
  */
-static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
-                                        const char *name,
-                                        struct list_head *head,
-                                        const char **info)
+static struct aa_label *find_attach(const struct linux_binprm *bprm,
+                                   struct aa_ns *ns, struct list_head *head,
+                                   const char *name, const char **info)
 {
        int candidate_len = 0, candidate_xattrs = 0;
        bool conflict = false;
@@ -388,6 +389,8 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
        AA_BUG(!name);
        AA_BUG(!head);
 
+       rcu_read_lock();
+restart:
        list_for_each_entry_rcu(profile, head, base.list) {
                if (profile->label.flags & FLAG_NULL &&
                    &profile->label == ns_unconfined(profile->ns))
@@ -413,16 +416,32 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
                        perm = dfa_user_allow(profile->xmatch, state);
                        /* any accepting state means a valid match. */
                        if (perm & MAY_EXEC) {
-                               int ret;
+                               int ret = 0;
 
                                if (count < candidate_len)
                                        continue;
 
-                               ret = aa_xattrs_match(bprm, profile, state);
-                               /* Fail matching if the xattrs don't match */
-                               if (ret < 0)
-                                       continue;
-
+                               if (bprm && profile->xattr_count) {
+                                       long rev = READ_ONCE(ns->revision);
+
+                                       if (!aa_get_profile_not0(profile))
+                                               goto restart;
+                                       rcu_read_unlock();
+                                       ret = aa_xattrs_match(bprm, profile,
+                                                             state);
+                                       rcu_read_lock();
+                                       aa_put_profile(profile);
+                                       if (rev !=
+                                           READ_ONCE(ns->revision))
+                                               /* policy changed */
+                                               goto restart;
+                                       /*
+                                        * Fail matching if the xattrs don't
+                                        * match
+                                        */
+                                       if (ret < 0)
+                                               continue;
+                               }
                                /*
                                 * TODO: allow for more flexible best match
                                 *
@@ -445,43 +464,28 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm,
                                candidate_xattrs = ret;
                                conflict = false;
                        }
-               } else if (!strcmp(profile->base.name, name))
+               } else if (!strcmp(profile->base.name, name)) {
                        /*
                         * old exact non-re match, without conditionals such
                         * as xattrs. no more searching required
                         */
-                       return profile;
+                       candidate = profile;
+                       goto out;
+               }
        }
 
-       if (conflict) {
-               *info = "conflicting profile attachments";
+       if (!candidate || conflict) {
+               if (conflict)
+                       *info = "conflicting profile attachments";
+               rcu_read_unlock();
                return NULL;
        }
 
-       return candidate;
-}
-
-/**
- * find_attach - do attachment search for unconfined processes
- * @bprm - binprm structure of transitioning task
- * @ns: the current namespace  (NOT NULL)
- * @list: list to search  (NOT NULL)
- * @name: the executable name to match against  (NOT NULL)
- * @info: info message if there was an error
- *
- * Returns: label or NULL if no match found
- */
-static struct aa_label *find_attach(const struct linux_binprm *bprm,
-                                   struct aa_ns *ns, struct list_head *list,
-                                   const char *name, const char **info)
-{
-       struct aa_profile *profile;
-
-       rcu_read_lock();
-       profile = aa_get_profile(__attach_match(bprm, name, list, info));
+out:
+       candidate = aa_get_newest_profile(candidate);
        rcu_read_unlock();
 
-       return profile ? &profile->label : NULL;
+       return &candidate->label;
 }
 
 static const char *next_name(int xtype, const char *name)

diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index 03104830c9132a38574407a32a2c87536dbea8f6..269f2f53c0b115405eda0046249151b605d1d116 100644 (file)
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -1125,8 +1125,8 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj,
        if (!name) {
                /* remove namespace - can only happen if fqname[0] == ':' */
                mutex_lock_nested(&ns->parent->lock, ns->level);
-               __aa_remove_ns(ns);
                __aa_bump_ns_revision(ns);
+               __aa_remove_ns(ns);
                mutex_unlock(&ns->parent->lock);
        } else {
                /* remove profile */
@@ -1138,9 +1138,9 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj,
                        goto fail_ns_lock;
                }
                name = profile->base.hname;
+               __aa_bump_ns_revision(ns);
                __remove_profile(profile);
                __aa_labelset_update_subtree(ns);
-               __aa_bump_ns_revision(ns);
                mutex_unlock(&ns->lock);
        }