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 Signed-off-by: Ryan Lee <[email protected]> --- security/apparmor/domain.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) This version of the patch applies cleanly to the upstream kernel. diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 7d81111f628c..2d3b80423d20 100644 --- 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> @@ -1128,6 +1129,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; @@ -1138,6 +1140,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; @@ -1146,7 +1174,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred, name = hats[i]; label_for_each_in_ns(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 */ @@ -1206,6 +1234,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred, GLOBAL_ROOT_UID, info, error); } } + mutex_unlock(&ns->lock); return ERR_PTR(error); build: @@ -1218,7 +1247,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred, error = -ENOMEM; goto fail; } /* else if (IS_ERR) build_change_hat has logged error so return new */ - + mutex_unlock(&ns->lock); return new; } -- 2.43.0
