[apparmor] [PATCH 22/43] apparmor: update how unconfined is handled
John Johansen
john.johansen at canonical.com
Fri Feb 8 21:00:58 UTC 2013
ns->unconfined is being used read side without locking, nor rcu but is
being updated when a namespace is removed. This works for the root ns
which is never removed but has a race window and can cause failures when
children namespaces are removed.
Also ns and ns->unconfined have a circular refcounting dependency that
is problematic and must be broken. Currently this is done incorrectly
when the namespace is destroyed.
Fix this by forward referencing unconfined via the replacedby infrastructure
instead of directly updating the ns->unconfined pointer.
Remove the circular refcount dependency by making the ns and its unconfined
profile share the same refcount.
Signed-off-by: John Johansen <john.johansen at canonical.com>
Acked-by: Seth Arnold <seth.arnold at canonical.com>
---
security/apparmor/domain.c | 2 +-
security/apparmor/include/policy.h | 80 ++++++++++++++++++------------------
security/apparmor/policy.c | 42 ++++++-------------
3 files changed, 52 insertions(+), 72 deletions(-)
diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index a49745c..4582a77 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -434,7 +434,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
new_profile = aa_get_profile(profile);
goto x_clear;
} else if (perms.xindex & AA_X_UNCONFINED) {
- new_profile = aa_get_profile(ns->unconfined);
+ new_profile = aa_get_newest_profile(ns->unconfined);
info = "ux fallback";
} else {
error = -ENOENT;
diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h
index a22f586..0367c9e 100644
--- a/security/apparmor/include/policy.h
+++ b/security/apparmor/include/policy.h
@@ -67,6 +67,7 @@ enum profile_flags {
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 */
+ PFLAG_NS_COUNT = 0x400, /* carries NS ref count */
/* These flags must correspond with PATH_flags */
PFLAG_MEDIATE_DELETED = 0x10000, /* mediate instead delegate deleted */
@@ -77,7 +78,6 @@ struct aa_profile;
/* struct aa_policy - common part of both namespaces and profiles
* @name: name of the object
* @hname - The hierarchical name
- * @count: reference count of the obj
* @list: list policy object is on
* @rcu: rcu head used when removing from @list
* @profiles: head of the profiles list contained in the object
@@ -85,7 +85,6 @@ struct aa_profile;
struct aa_policy {
char *name;
char *hname;
- struct kref count;
struct list_head list;
struct list_head profiles;
struct rcu_head rcu;
@@ -156,6 +155,7 @@ struct aa_replacedby {
/* struct aa_profile - basic confinement data
* @base - base components of the profile (name, refcount, lists, lock ...)
+ * @count: reference count of the obj
* @parent: parent of profile
* @ns: namespace the profile is in
* @replacedby: is set to the profile that replaced this profile
@@ -188,6 +188,7 @@ struct aa_replacedby {
*/
struct aa_profile {
struct aa_policy base;
+ struct kref count;
struct aa_profile __rcu *parent;
struct aa_namespace *ns;
@@ -222,40 +223,6 @@ void aa_free_namespace_kref(struct kref *kref);
struct aa_namespace *aa_find_namespace(struct aa_namespace *root,
const char *name);
-static inline struct aa_policy *aa_get_common(struct aa_policy *c)
-{
- if (c)
- kref_get(&c->count);
-
- return c;
-}
-
-/**
- * aa_get_namespace - increment references count on @ns
- * @ns: namespace to increment reference count of (MAYBE NULL)
- *
- * Returns: pointer to @ns, if @ns is NULL returns NULL
- * Requires: @ns must be held with valid refcount when called
- */
-static inline struct aa_namespace *aa_get_namespace(struct aa_namespace *ns)
-{
- if (ns)
- kref_get(&(ns->base.count));
-
- return ns;
-}
-
-/**
- * aa_put_namespace - decrement refcount on @ns
- * @ns: namespace to put reference of
- *
- * Decrement reference count of @ns and if no longer in use free it
- */
-static inline void aa_put_namespace(struct aa_namespace *ns)
-{
- if (ns)
- kref_put(&ns->base.count, aa_free_namespace_kref);
-}
void aa_free_replacedby_kref(struct kref *kref);
struct aa_profile *aa_alloc_profile(const char *name);
@@ -284,7 +251,7 @@ ssize_t aa_remove_profiles(char *name, size_t size);
static inline struct aa_profile *aa_get_profile(struct aa_profile *p)
{
if (p)
- kref_get(&(p->base.count));
+ kref_get(&(p->count));
return p;
}
@@ -298,7 +265,7 @@ static inline struct aa_profile *aa_get_profile(struct aa_profile *p)
*/
static inline struct aa_profile *aa_get_profile_not0(struct aa_profile *p)
{
- if (p && kref_get_not0(&p->base.count))
+ if (p && kref_get_not0(&p->count))
return p;
return NULL;
@@ -318,7 +285,7 @@ static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile **p)
rcu_read_lock();
do {
c = rcu_dereference(*p);
- } while (c && !kref_get_not0(&c->base.count));
+ } while (c && !kref_get_not0(&c->count));
rcu_read_unlock();
return c;
@@ -349,8 +316,12 @@ static inline struct aa_profile *aa_get_newest_profile(struct aa_profile *p)
*/
static inline void aa_put_profile(struct aa_profile *p)
{
- if (p)
- kref_put(&p->base.count, aa_free_profile_kref);
+ if (p) {
+ if (p->flags & PFLAG_NS_COUNT)
+ kref_put(&p->count, aa_free_namespace_kref);
+ else
+ kref_put(&p->count, aa_free_profile_kref);
+ }
}
static inline struct aa_replacedby *aa_get_replacedby(struct aa_replacedby *p)
@@ -377,6 +348,33 @@ static inline void __aa_update_replacedby(struct aa_profile *orig,
aa_put_profile(tmp);
}
+/**
+ * aa_get_namespace - increment references count on @ns
+ * @ns: namespace to increment reference count of (MAYBE NULL)
+ *
+ * Returns: pointer to @ns, if @ns is NULL returns NULL
+ * Requires: @ns must be held with valid refcount when called
+ */
+static inline struct aa_namespace *aa_get_namespace(struct aa_namespace *ns)
+{
+ if (ns)
+ aa_get_profile(ns->unconfined);
+
+ return ns;
+}
+
+/**
+ * aa_put_namespace - decrement refcount on @ns
+ * @ns: namespace to put reference of
+ *
+ * Decrement reference count of @ns and if no longer in use free it
+ */
+static inline void aa_put_namespace(struct aa_namespace *ns)
+{
+ if (ns)
+ aa_put_profile(ns->unconfined);
+}
+
static inline int AUDIT_MODE(struct aa_profile *profile)
{
if (aa_g_audit != AUDIT_NORMAL)
diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c
index eafde2c..1c22fbc 100644
--- a/security/apparmor/policy.c
+++ b/security/apparmor/policy.c
@@ -141,7 +141,6 @@ static bool policy_init(struct aa_policy *policy, const char *prefix,
policy->name = (char *)hname_tail(policy->hname);
INIT_LIST_HEAD(&policy->list);
INIT_LIST_HEAD(&policy->profiles);
- kref_init(&policy->count);
return 1;
}
@@ -292,14 +291,10 @@ static struct aa_namespace *alloc_namespace(const char *prefix,
goto fail_unconfined;
ns->unconfined->flags = PFLAG_UNCONFINED | PFLAG_IX_ON_NAME_ERROR |
- PFLAG_IMMUTABLE;
+ PFLAG_IMMUTABLE | PFLAG_NS_COUNT;
- /*
- * released by free_namespace, however __remove_namespace breaks
- * the cyclic references (ns->unconfined, and unconfined->ns) and
- * replaces with refs to parent namespace unconfined
- */
- ns->unconfined->ns = aa_get_namespace(ns);
+ /* ns and ns->unconfined share ns->unconfined refcount */
+ ns->unconfined->ns = ns;
atomic_set(&ns->uniq_null, 0);
@@ -312,6 +307,7 @@ fail_ns:
return NULL;
}
+static void free_profile(struct aa_profile *profile);
/**
* free_namespace - free a profile namespace
* @ns: the namespace to free (MAYBE NULL)
@@ -327,10 +323,8 @@ static void free_namespace(struct aa_namespace *ns)
policy_destroy(&ns->base);
aa_put_namespace(ns->parent);
- if (ns->unconfined && ns->unconfined->ns == ns)
- ns->unconfined->ns = NULL;
-
- aa_put_profile(ns->unconfined);
+ ns->unconfined->ns = NULL;
+ free_profile(ns->unconfined);
kzfree(ns);
}
@@ -340,7 +334,8 @@ static void free_namespace(struct aa_namespace *ns)
*/
void aa_free_namespace_kref(struct kref *kref)
{
- free_namespace(container_of(kref, struct aa_namespace, base.count));
+ struct aa_profile *p = container_of(kref, struct aa_profile, count);
+ free_namespace(p->ns);
}
/**
@@ -531,8 +526,6 @@ static void __ns_list_release(struct list_head *head);
*/
static void destroy_namespace(struct aa_namespace *ns)
{
- struct aa_profile *unconfined;
-
if (!ns)
return;
@@ -543,19 +536,8 @@ static void destroy_namespace(struct aa_namespace *ns)
/* release all sub namespaces */
__ns_list_release(&ns->sub_ns);
- unconfined = ns->unconfined;
- /*
- * break the ns, unconfined profile cyclic reference and forward
- * all new unconfined profiles requests to the parent namespace
- * This will result in all confined tasks that have a profile
- * being removed, inheriting the parent->unconfined profile.
- */
if (ns->parent)
- ns->unconfined = aa_get_profile(ns->parent->unconfined);
-
- /* release original ns->unconfined ref */
- aa_put_profile(unconfined);
-
+ __aa_update_replacedby(ns->unconfined, ns->parent->unconfined);
mutex_unlock(&ns->lock);
}
@@ -700,8 +682,7 @@ static void aa_free_profile_rcu(struct rcu_head *head)
*/
void aa_free_profile_kref(struct kref *kref)
{
- struct aa_profile *p = container_of(kref, struct aa_profile,
- base.count);
+ struct aa_profile *p = container_of(kref, struct aa_profile, count);
call_rcu(&p->base.rcu, aa_free_profile_rcu);
}
@@ -727,6 +708,7 @@ struct aa_profile *aa_alloc_profile(const char *hname)
if (!policy_init(&profile->base, NULL, hname))
goto fail;
+ kref_init(&profile->count);
/* refcount released by caller */
return profile;
@@ -928,7 +910,7 @@ struct aa_profile *aa_lookup_profile(struct aa_namespace *ns, const char *hname)
/* the unconfined profile is not in the regular profile list */
if (!profile && strcmp(hname, "unconfined") == 0)
- profile = aa_get_profile(ns->unconfined);
+ profile = aa_get_newest_profile(ns->unconfined);
/* refcount released by caller */
return profile;
--
1.7.10.4
More information about the AppArmor
mailing list