Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit f3248d6d authored by John Johansen's avatar John Johansen Committed by Greg Kroah-Hartman
Browse files

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



commit 8c62ed27a12c00e3db1c9f04bc0f272bdbb06734 upstream.

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: 8e51f908 ("apparmor: Add support for attaching profiles via xattr, presence and value")
Reported-by: default avatarJia-Ju Bai <baijiaju1990@gmail.com>
Reported-by: default avatarAl Viro <viro@zeniv.linux.org.uk>
Signed-off-by: default avatarJohn Johansen <john.johansen@canonical.com>
Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
parent 1c662483

security/apparmor/apparmorfs.c

+1 −1
Original line number Original line Diff line number Diff line
@@ -593,7 +593,7 @@ static __poll_t ns_revision_poll(struct file *file, poll_table *pt) 




void __aa_bump_ns_revision(struct aa_ns *ns)

void __aa_bump_ns_revision(struct aa_ns *ns)

{

{

	ns->revision++;

	WRITE_ONCE(ns->revision, ns->revision + 1);

	wake_up_interruptible(&ns->wait);

	wake_up_interruptible(&ns->wait);

}

}

security/apparmor/domain.c

+43 −39
Original line number Original line Diff line number Diff line
@@ -321,6 +321,7 @@ static int aa_xattrs_match(const struct linux_binprm *bprm, 




	if (!bprm || !profile->xattr_count)

	if (!bprm || !profile->xattr_count)

		return 0;

		return 0;

	might_sleep();





	/* transition from exec match to xattr set */

	/* transition from exec match to xattr set */

	state = aa_dfa_null_transition(profile->xmatch, state);

	state = aa_dfa_null_transition(profile->xmatch, state);
@@ -365,10 +366,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm, 
}

}





/**

/**

 * __attach_match_ - find an attachment match

 * find_attach - do attachment search for unconfined processes

 * @bprm - binprm structure of transitioning task

 * @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)

 * @head - profile list to walk  (NOT NULL)

 * @name - to match against  (NOT NULL)

 * @info - info message if there was an error (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

 * Do a linear search on the profiles in the list.  There is a matching
@@ -378,12 +380,11 @@ static int aa_xattrs_match(const struct linux_binprm *bprm, 
 *

 *

 * Requires: @head not be shared or have appropriate locks held

 * 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,

static struct aa_label *find_attach(const struct linux_binprm *bprm,

					 const char *name,

				    struct aa_ns *ns, struct list_head *head,

					 struct list_head *head,

				    const char *name, const char **info)

					 const char **info)

{

{

	int candidate_len = 0, candidate_xattrs = 0;

	int candidate_len = 0, candidate_xattrs = 0;

	bool conflict = false;

	bool conflict = false;
@@ -392,6 +393,8 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm, 
	AA_BUG(!name);

	AA_BUG(!name);

	AA_BUG(!head);

	AA_BUG(!head);





	rcu_read_lock();

restart:

	list_for_each_entry_rcu(profile, head, base.list) {

	list_for_each_entry_rcu(profile, head, base.list) {

		if (profile->label.flags & FLAG_NULL &&

		if (profile->label.flags & FLAG_NULL &&

		    &profile->label == ns_unconfined(profile->ns))

		    &profile->label == ns_unconfined(profile->ns))
@@ -417,16 +420,32 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm, 
			perm = dfa_user_allow(profile->xmatch, state);

			perm = dfa_user_allow(profile->xmatch, state);

			/* any accepting state means a valid match. */

			/* any accepting state means a valid match. */

			if (perm & MAY_EXEC) {

			if (perm & MAY_EXEC) {

				int ret;

				int ret = 0;





				if (count < candidate_len)

				if (count < candidate_len)

					continue;

					continue;





				ret = aa_xattrs_match(bprm, profile, state);

				if (bprm && profile->xattr_count) {

				/* Fail matching if the xattrs don't match */

					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)

					if (ret < 0)

						continue;

						continue;



				}

				/*

				/*

				 * TODO: allow for more flexible best match

				 * TODO: allow for more flexible best match

				 *

				 *
@@ -449,43 +468,28 @@ static struct aa_profile *__attach_match(const struct linux_binprm *bprm, 
				candidate_xattrs = ret;

				candidate_xattrs = ret;

				conflict = false;

				conflict = false;

			}

			}

		} else if (!strcmp(profile->base.name, name))

		} else if (!strcmp(profile->base.name, name)) {

			/*

			/*

			 * old exact non-re match, without conditionals such

			 * old exact non-re match, without conditionals such

			 * as xattrs. no more searching required

			 * as xattrs. no more searching required

			 */

			 */

			return profile;

			candidate = profile;

			goto out;

		}

	}

	}





	if (conflict) {

	if (!candidate || conflict) {

		if (conflict)

			*info = "conflicting profile attachments";

			*info = "conflicting profile attachments";

		rcu_read_unlock();

		return NULL;

		return NULL;

	}

	}





	return candidate;

out:

}

	candidate = aa_get_newest_profile(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));

	rcu_read_unlock();

	rcu_read_unlock();





	return profile ? &profile->label : NULL;

	return &candidate->label;

}

}





static const char *next_name(int xtype, const char *name)

static const char *next_name(int xtype, const char *name)

security/apparmor/policy.c

+2 −2
Original line number Original line Diff line number Diff line
@@ -1126,8 +1126,8 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj, 
	if (!name) {

	if (!name) {

		/* remove namespace - can only happen if fqname[0] == ':' */

		/* remove namespace - can only happen if fqname[0] == ':' */

		mutex_lock_nested(&ns->parent->lock, ns->level);

		mutex_lock_nested(&ns->parent->lock, ns->level);

		__aa_remove_ns(ns);

		__aa_bump_ns_revision(ns);

		__aa_bump_ns_revision(ns);

		__aa_remove_ns(ns);

		mutex_unlock(&ns->parent->lock);

		mutex_unlock(&ns->parent->lock);

	} else {

	} else {

		/* remove profile */

		/* remove profile */
@@ -1139,9 +1139,9 @@ ssize_t aa_remove_profiles(struct aa_ns *policy_ns, struct aa_label *subj, 
			goto fail_ns_lock;

			goto fail_ns_lock;

		}

		}

		name = profile->base.hname;

		name = profile->base.hname;

		__aa_bump_ns_revision(ns);

		__remove_profile(profile);

		__remove_profile(profile);

		__aa_labelset_update_subtree(ns);

		__aa_labelset_update_subtree(ns);

		__aa_bump_ns_revision(ns);

		mutex_unlock(&ns->lock);

		mutex_unlock(&ns->lock);

	}

	}