From dd51c84857630e77c139afe4d9bba65fc051dc3f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:05:43 -0700 Subject: apparmor: provide base for multiple profiles to be replaced at once previously profiles had to be loaded one at a time, which could result in cases where a replacement of a set would partially succeed, and then fail resulting in inconsistent policy. Allow multiple profiles to replaced "atomically" so that the replacement either succeeds or fails for the entire set of profiles. Signed-off-by: John Johansen --- security/apparmor/policy.c | 300 +++++++++++++++++++++++++++------------------ 1 file changed, 183 insertions(+), 117 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 0f345c4dee5f..407b442c0a2c 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -472,45 +472,6 @@ static void __list_remove_profile(struct aa_profile *profile) aa_put_profile(profile); } -/** - * __replace_profile - replace @old with @new on a list - * @old: profile to be replaced (NOT NULL) - * @new: profile to replace @old with (NOT NULL) - * - * Will duplicate and refcount elements that @new inherits from @old - * and will inherit @old children. - * - * refcount @new for list, put @old list refcount - * - * Requires: namespace list lock be held, or list not be shared - */ -static void __replace_profile(struct aa_profile *old, struct aa_profile *new) -{ - struct aa_policy *policy; - struct aa_profile *child, *tmp; - - if (old->parent) - policy = &old->parent->base; - else - policy = &old->ns->base; - - /* released when @new is freed */ - new->parent = aa_get_profile(old->parent); - new->ns = aa_get_namespace(old->ns); - __list_add_profile(&policy->profiles, new); - /* inherit children */ - list_for_each_entry_safe(child, tmp, &old->base.profiles, base.list) { - aa_put_profile(child->parent); - child->parent = aa_get_profile(new); - /* list refcount transferred to @new*/ - list_move(&child->base.list, &new->base.profiles); - } - - /* released by free_profile */ - old->replacedby = aa_get_profile(new); - __list_remove_profile(old); -} - static void __profile_list_release(struct list_head *head); /** @@ -952,25 +913,6 @@ static int replacement_allowed(struct aa_profile *profile, int noreplace, return 0; } -/** - * __add_new_profile - simple wrapper around __list_add_profile - * @ns: namespace that profile is being added to (NOT NULL) - * @policy: the policy container to add the profile to (NOT NULL) - * @profile: profile to add (NOT NULL) - * - * add a profile to a list and do other required basic allocations - */ -static void __add_new_profile(struct aa_namespace *ns, struct aa_policy *policy, - struct aa_profile *profile) -{ - if (policy != &ns->base) - /* released on profile replacement or free_profile */ - profile->parent = aa_get_profile((struct aa_profile *) policy); - __list_add_profile(&policy->profiles, profile); - /* released on free_profile */ - profile->ns = aa_get_namespace(ns); -} - /** * aa_audit_policy - Do auditing of policy changes * @op: policy operation being performed @@ -1019,6 +961,109 @@ bool aa_may_manage_policy(int op) return 1; } +static struct aa_profile *__list_lookup_parent(struct list_head *lh, + struct aa_profile *profile) +{ + const char *base = hname_tail(profile->base.hname); + long len = base - profile->base.hname; + struct aa_load_ent *ent; + + /* parent won't have trailing // so remove from len */ + if (len <= 2) + return NULL; + len -= 2; + + list_for_each_entry(ent, lh, list) { + if (ent->new == profile) + continue; + if (strncmp(ent->new->base.hname, profile->base.hname, len) == + 0 && ent->new->base.hname[len] == 0) + return ent->new; + } + + return NULL; +} + +/** + * __replace_profile - replace @old with @new on a list + * @old: profile to be replaced (NOT NULL) + * @new: profile to replace @old with (NOT NULL) + * + * Will duplicate and refcount elements that @new inherits from @old + * and will inherit @old children. + * + * refcount @new for list, put @old list refcount + * + * Requires: namespace list lock be held, or list not be shared + */ +static void __replace_profile(struct aa_profile *old, struct aa_profile *new) +{ + struct aa_profile *child, *tmp; + + if (!list_empty(&old->base.profiles)) { + LIST_HEAD(lh); + list_splice_init(&old->base.profiles, &lh); + + list_for_each_entry_safe(child, tmp, &lh, base.list) { + struct aa_profile *p; + + list_del_init(&child->base.list); + p = __find_child(&new->base.profiles, child->base.name); + if (p) { + /* @p replaces @child */ + __replace_profile(child, p); + continue; + } + + /* inherit @child and its children */ + /* TODO: update hname of inherited children */ + /* list refcount transferred to @new */ + list_add(&child->base.list, &new->base.profiles); + aa_put_profile(child->parent); + child->parent = aa_get_profile(new); + } + } + + if (!new->parent) + new->parent = aa_get_profile(old->parent); + /* released by free_profile */ + old->replacedby = aa_get_profile(new); + + if (list_empty(&new->base.list)) { + /* new is not on a list already */ + list_replace_init(&old->base.list, &new->base.list); + aa_get_profile(new); + aa_put_profile(old); + } else + __list_remove_profile(old); +} + +/** + * __lookup_replace - lookup replacement information for a profile + * @ns - namespace the lookup occurs in + * @hname - name of profile to lookup + * @noreplace - true if not replacing an existing profile + * @p - Returns: profile to be replaced + * @info - Returns: info string on why lookup failed + * + * Returns: profile to replace (no ref) on success else ptr error + */ +static int __lookup_replace(struct aa_namespace *ns, const char *hname, + bool noreplace, struct aa_profile **p, + const char **info) +{ + *p = aa_get_profile(__lookup_profile(&ns->base, hname)); + if (*p) { + int error = replacement_allowed(*p, noreplace, info); + if (error) { + *info = "profile can not be replaced"; + return error; + } + } + + return 0; +} + /** * aa_replace_profiles - replace profile(s) on the profile list * @udata: serialized data stream (NOT NULL) @@ -1033,21 +1078,17 @@ bool aa_may_manage_policy(int op) */ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) { - struct aa_policy *policy; - struct aa_profile *old_profile = NULL, *new_profile = NULL; - struct aa_profile *rename_profile = NULL; - struct aa_namespace *ns = NULL; const char *ns_name, *name = NULL, *info = NULL; + struct aa_namespace *ns = NULL; + struct aa_load_ent *ent, *tmp; int op = OP_PROF_REPL; ssize_t error; + LIST_HEAD(lh); /* released below */ - new_profile = aa_unpack(udata, size, &ns_name); - if (IS_ERR(new_profile)) { - error = PTR_ERR(new_profile); - new_profile = NULL; - goto fail; - } + error = aa_unpack(udata, size, &lh, &ns_name); + if (error) + goto out; /* released below */ ns = aa_prepare_namespace(ns_name); @@ -1058,71 +1099,96 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) goto fail; } - name = new_profile->base.hname; - write_lock(&ns->lock); - /* no ref on policy only use inside lock */ - policy = __lookup_parent(ns, new_profile->base.hname); - - if (!policy) { - info = "parent does not exist"; - error = -ENOENT; - goto audit; - } - - old_profile = __find_child(&policy->profiles, new_profile->base.name); - /* released below */ - aa_get_profile(old_profile); - - if (new_profile->rename) { - rename_profile = __lookup_profile(&ns->base, - new_profile->rename); - /* released below */ - aa_get_profile(rename_profile); - - if (!rename_profile) { - info = "profile to rename does not exist"; - name = new_profile->rename; - error = -ENOENT; - goto audit; + /* setup parent and ns info */ + list_for_each_entry(ent, &lh, list) { + struct aa_policy *policy; + + name = ent->new->base.hname; + error = __lookup_replace(ns, ent->new->base.hname, noreplace, + &ent->old, &info); + if (error) + goto fail_lock; + + if (ent->new->rename) { + error = __lookup_replace(ns, ent->new->rename, + noreplace, &ent->rename, + &info); + if (error) + goto fail_lock; } - } - - error = replacement_allowed(old_profile, noreplace, &info); - if (error) - goto audit; - error = replacement_allowed(rename_profile, noreplace, &info); - if (error) - goto audit; - -audit: - if (!old_profile && !rename_profile) - op = OP_PROF_LOAD; + /* released when @new is freed */ + ent->new->ns = aa_get_namespace(ns); + + if (ent->old || ent->rename) + continue; + + /* no ref on policy only use inside lock */ + policy = __lookup_parent(ns, ent->new->base.hname); + if (!policy) { + struct aa_profile *p; + p = __list_lookup_parent(&lh, ent->new); + if (!p) { + error = -ENOENT; + info = "parent does not exist"; + name = ent->new->base.hname; + goto fail_lock; + } + ent->new->parent = aa_get_profile(p); + } else if (policy != &ns->base) + /* released on profile replacement or free_profile */ + ent->new->parent = aa_get_profile((struct aa_profile *) + policy); + } - error = audit_policy(op, GFP_ATOMIC, name, info, error); + /* do actual replacement */ + list_for_each_entry_safe(ent, tmp, &lh, list) { + list_del_init(&ent->list); + op = (!ent->old && !ent->rename) ? OP_PROF_LOAD : OP_PROF_REPL; + + audit_policy(op, GFP_ATOMIC, ent->new->base.name, NULL, error); + + if (ent->old) { + __replace_profile(ent->old, ent->new); + if (ent->rename) + __replace_profile(ent->rename, ent->new); + } else if (ent->rename) { + __replace_profile(ent->rename, ent->new); + } else if (ent->new->parent) { + struct aa_profile *parent; + parent = aa_newest_version(ent->new->parent); + /* parent replaced in this atomic set? */ + if (parent != ent->new->parent) { + aa_get_profile(parent); + aa_put_profile(ent->new->parent); + ent->new->parent = parent; + } + __list_add_profile(&parent->base.profiles, ent->new); + } else + __list_add_profile(&ns->base.profiles, ent->new); - if (!error) { - if (rename_profile) - __replace_profile(rename_profile, new_profile); - if (old_profile) - __replace_profile(old_profile, new_profile); - if (!(old_profile || rename_profile)) - __add_new_profile(ns, policy, new_profile); + aa_load_ent_free(ent); } write_unlock(&ns->lock); out: aa_put_namespace(ns); - aa_put_profile(rename_profile); - aa_put_profile(old_profile); - aa_put_profile(new_profile); + if (error) return error; return size; +fail_lock: + write_unlock(&ns->lock); fail: error = audit_policy(op, GFP_KERNEL, name, info, error); + + list_for_each_entry_safe(ent, tmp, &lh, list) { + list_del_init(&ent->list); + aa_load_ent_free(ent); + } + goto out; } -- cgit v1.2.3 From 01e2b670aa898a39259bc85c78e3d74820f4d3b6 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:06:43 -0700 Subject: apparmor: convert profile lists to RCU based locking Signed-off-by: John Johansen --- security/apparmor/domain.c | 14 ++- security/apparmor/include/apparmor.h | 6 + security/apparmor/include/policy.h | 45 +++++++- security/apparmor/policy.c | 213 ++++++++++++++++++----------------- 4 files changed, 167 insertions(+), 111 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 01b7bd669a88..454bcd7f3452 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -144,7 +144,7 @@ static struct aa_profile *__attach_match(const char *name, int len = 0; struct aa_profile *profile, *candidate = NULL; - list_for_each_entry(profile, head, base.list) { + list_for_each_entry_rcu(profile, head, base.list) { if (profile->flags & PFLAG_NULL) continue; if (profile->xmatch && profile->xmatch_len > len) { @@ -177,9 +177,9 @@ static struct aa_profile *find_attach(struct aa_namespace *ns, { struct aa_profile *profile; - read_lock(&ns->lock); + rcu_read_lock(); profile = aa_get_profile(__attach_match(name, list)); - read_unlock(&ns->lock); + rcu_read_unlock(); return profile; } @@ -641,7 +641,10 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) if (count) { /* attempting to change into a new hat or switch to a sibling */ struct aa_profile *root; - root = PROFILE_IS_HAT(profile) ? profile->parent : profile; + if (PROFILE_IS_HAT(profile)) + root = aa_get_profile_rcu(&profile->parent); + else + root = aa_get_profile(profile); /* find first matching hat */ for (i = 0; i < count && !hat; i++) @@ -653,6 +656,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) error = -ECHILD; else error = -ENOENT; + aa_put_profile(root); goto out; } @@ -667,6 +671,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) /* freed below */ name = new_compound_name(root->base.hname, hats[0]); + aa_put_profile(root); target = name; /* released below */ hat = aa_new_null_profile(profile, 1); @@ -676,6 +681,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest) goto audit; } } else { + aa_put_profile(root); target = hat->base.hname; if (!PROFILE_IS_HAT(hat)) { info = "target not hat"; diff --git a/security/apparmor/include/apparmor.h b/security/apparmor/include/apparmor.h index 1ba2ca56a6ef..8fb1488a3cd4 100644 --- a/security/apparmor/include/apparmor.h +++ b/security/apparmor/include/apparmor.h @@ -78,6 +78,12 @@ static inline void *kvzalloc(size_t size) return __aa_kvmalloc(size, __GFP_ZERO); } +/* returns 0 if kref not incremented */ +static inline int kref_get_not0(struct kref *kref) +{ + return atomic_inc_not_zero(&kref->refcount); +} + /** * aa_strneq - compare null terminated @str to a non null terminated substring * @str: a null terminated string diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index b25491a3046a..82487a853353 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 on_list_rcu(X) (!list_empty(X) && (X)->prev != LIST_POISON2) + /* * FIXME: currently need a clean way to replace and remove profiles as a * set. It should be done at the namespace level. @@ -75,6 +77,7 @@ struct aa_profile; * @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 */ struct aa_policy { @@ -83,6 +86,7 @@ struct aa_policy { struct kref count; struct list_head list; struct list_head profiles; + struct rcu_head rcu; }; /* struct aa_ns_acct - accounting of profiles in namespace @@ -124,7 +128,7 @@ struct aa_ns_acct { struct aa_namespace { struct aa_policy base; struct aa_namespace *parent; - rwlock_t lock; + struct mutex lock; struct aa_ns_acct acct; struct aa_profile *unconfined; struct list_head sub_ns; @@ -166,7 +170,7 @@ struct aa_policydb { * attachments are determined by profile X transition rules. * * The @replacedby field is write protected by the profile lock. Reads - * are assumed to be atomic, and are done without locking. + * are assumed to be atomic. * * Profiles have a hierarchy where hats and children profiles keep * a reference to their parent. @@ -177,7 +181,7 @@ struct aa_policydb { */ struct aa_profile { struct aa_policy base; - struct aa_profile *parent; + struct aa_profile __rcu *parent; struct aa_namespace *ns; struct aa_profile *replacedby; @@ -295,6 +299,41 @@ static inline struct aa_profile *aa_get_profile(struct aa_profile *p) return p; } +/** + * aa_get_profile_not0 - increment refcount on profile @p found via lookup + * @p: profile (MAYBE NULL) + * + * Returns: pointer to @p if @p is NULL will return NULL + * Requires: @p must be held with valid refcount when called + */ +static inline struct aa_profile *aa_get_profile_not0(struct aa_profile *p) +{ + if (p && kref_get_not0(&p->base.count)) + return p; + + return NULL; +} + +/** + * aa_get_profile_rcu - increment a refcount profile that can be replaced + * @p: pointer to profile that can be replaced (NOT NULL) + * + * Returns: pointer to a refcounted profile. + * else NULL if no profile + */ +static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile __rcu **p) +{ + struct aa_profile *c; + + rcu_read_lock(); + do { + c = rcu_dereference(*p); + } while (c && !kref_get_not0(&c->base.count)); + rcu_read_unlock(); + + return c; +} + /** * aa_put_profile - decrement refcount on profile @p * @p: profile (MAYBE NULL) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 407b442c0a2c..25bbbb482bb6 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -153,13 +153,13 @@ static bool policy_init(struct aa_policy *policy, const char *prefix, static void policy_destroy(struct aa_policy *policy) { /* still contains profiles -- invalid */ - if (!list_empty(&policy->profiles)) { + if (on_list_rcu(&policy->profiles)) { AA_ERROR("%s: internal error, " "policy '%s' still contains profiles\n", __func__, policy->name); BUG(); } - if (!list_empty(&policy->list)) { + if (on_list_rcu(&policy->list)) { AA_ERROR("%s: internal error, policy '%s' still on list\n", __func__, policy->name); BUG(); @@ -174,7 +174,7 @@ static void policy_destroy(struct aa_policy *policy) * @head: list to search (NOT NULL) * @name: name to search for (NOT NULL) * - * Requires: correct locks for the @head list be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted policy that match @name or NULL if not found */ @@ -182,7 +182,7 @@ static struct aa_policy *__policy_find(struct list_head *head, const char *name) { struct aa_policy *policy; - list_for_each_entry(policy, head, list) { + list_for_each_entry_rcu(policy, head, list) { if (!strcmp(policy->name, name)) return policy; } @@ -195,7 +195,7 @@ static struct aa_policy *__policy_find(struct list_head *head, const char *name) * @str: string to search for (NOT NULL) * @len: length of match required * - * Requires: correct locks for the @head list be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted policy that match @str or NULL if not found * @@ -207,7 +207,7 @@ static struct aa_policy *__policy_strn_find(struct list_head *head, { struct aa_policy *policy; - list_for_each_entry(policy, head, list) { + list_for_each_entry_rcu(policy, head, list) { if (aa_strneq(policy->name, str, len)) return policy; } @@ -284,7 +284,7 @@ static struct aa_namespace *alloc_namespace(const char *prefix, goto fail_ns; INIT_LIST_HEAD(&ns->sub_ns); - rwlock_init(&ns->lock); + mutex_init(&ns->lock); /* released by free_namespace */ ns->unconfined = aa_alloc_profile("unconfined"); @@ -350,7 +350,7 @@ void aa_free_namespace_kref(struct kref *kref) * * Returns: unrefcounted namespace * - * Requires: ns lock be held + * Requires: rcu_read_lock be held */ static struct aa_namespace *__aa_find_namespace(struct list_head *head, const char *name) @@ -373,9 +373,9 @@ struct aa_namespace *aa_find_namespace(struct aa_namespace *root, { struct aa_namespace *ns = NULL; - read_lock(&root->lock); + rcu_read_lock(); ns = aa_get_namespace(__aa_find_namespace(&root->sub_ns, name)); - read_unlock(&root->lock); + rcu_read_unlock(); return ns; } @@ -392,7 +392,7 @@ static struct aa_namespace *aa_prepare_namespace(const char *name) root = aa_current_profile()->ns; - write_lock(&root->lock); + mutex_lock(&root->lock); /* if name isn't specified the profile is loaded to the current ns */ if (!name) { @@ -405,31 +405,17 @@ static struct aa_namespace *aa_prepare_namespace(const char *name) /* released by caller */ ns = aa_get_namespace(__aa_find_namespace(&root->sub_ns, name)); if (!ns) { - /* namespace not found */ - struct aa_namespace *new_ns; - write_unlock(&root->lock); - new_ns = alloc_namespace(root->base.hname, name); - if (!new_ns) - return NULL; - write_lock(&root->lock); - /* test for race when new_ns was allocated */ - ns = __aa_find_namespace(&root->sub_ns, name); - if (!ns) { - /* add parent ref */ - new_ns->parent = aa_get_namespace(root); - - list_add(&new_ns->base.list, &root->sub_ns); - /* add list ref */ - ns = aa_get_namespace(new_ns); - } else { - /* raced so free the new one */ - free_namespace(new_ns); - /* get reference on namespace */ - aa_get_namespace(ns); - } + ns = alloc_namespace(root->base.hname, name); + if (!ns) + goto out; + /* add parent ref */ + ns->parent = aa_get_namespace(root); + list_add_rcu(&ns->base.list, &root->sub_ns); + /* add list ref */ + aa_get_namespace(ns); } out: - write_unlock(&root->lock); + mutex_unlock(&root->lock); /* return ref */ return ns; @@ -447,7 +433,7 @@ out: static void __list_add_profile(struct list_head *list, struct aa_profile *profile) { - list_add(&profile->base.list, list); + list_add_rcu(&profile->base.list, list); /* get list reference */ aa_get_profile(profile); } @@ -466,10 +452,8 @@ static void __list_add_profile(struct list_head *list, */ static void __list_remove_profile(struct aa_profile *profile) { - list_del_init(&profile->base.list); - if (!(profile->flags & PFLAG_NO_LIST_REF)) - /* release list reference */ - aa_put_profile(profile); + list_del_rcu(&profile->base.list); + aa_put_profile(profile); } static void __profile_list_release(struct list_head *head); @@ -510,17 +494,40 @@ static void __ns_list_release(struct list_head *head); */ static void destroy_namespace(struct aa_namespace *ns) { + struct aa_profile *unconfined; + if (!ns) return; - write_lock(&ns->lock); + mutex_lock(&ns->lock); /* release all profiles in this namespace */ __profile_list_release(&ns->base.profiles); /* release all sub namespaces */ __ns_list_release(&ns->sub_ns); - write_unlock(&ns->lock); + 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); + + mutex_unlock(&ns->lock); +} + +static void aa_put_ns_rcu(struct rcu_head *head) +{ + struct aa_namespace *ns = container_of(head, struct aa_namespace, + base.rcu); + /* release ns->base.list ref */ + aa_put_namespace(ns); } /** @@ -531,26 +538,12 @@ static void destroy_namespace(struct aa_namespace *ns) */ static void __remove_namespace(struct aa_namespace *ns) { - struct aa_profile *unconfined = ns->unconfined; - /* remove ns from namespace list */ - list_del_init(&ns->base.list); - - /* - * 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); + list_del_rcu(&ns->base.list); destroy_namespace(ns); - /* release original ns->unconfined ref */ - aa_put_profile(unconfined); - /* release ns->base.list ref, from removal above */ - aa_put_namespace(ns); + call_rcu(&ns->base.rcu, aa_put_ns_rcu); } /** @@ -614,16 +607,9 @@ static void free_profile(struct aa_profile *profile) if (!profile) return; - if (!list_empty(&profile->base.list)) { - AA_ERROR("%s: internal error, " - "profile '%s' still on ns list\n", - __func__, profile->base.name); - BUG(); - } - /* free children profiles */ policy_destroy(&profile->base); - aa_put_profile(profile->parent); + aa_put_profile(rcu_access_pointer(profile->parent)); aa_put_namespace(profile->ns); kzfree(profile->rename); @@ -660,6 +646,16 @@ static void free_profile(struct aa_profile *profile) kzfree(profile); } +/** + * aa_free_profile_rcu - free aa_profile by rcu (called by aa_free_profile_kref) + * @head: rcu_head callback for freeing of a profile (NOT NULL) + */ +static void aa_free_profile_rcu(struct rcu_head *head) +{ + struct aa_profile *p = container_of(head, struct aa_profile, base.rcu); + free_profile(p); +} + /** * aa_free_profile_kref - free aa_profile by kref (called by aa_put_profile) * @kr: kref callback for freeing of a profile (NOT NULL) @@ -668,8 +664,7 @@ void aa_free_profile_kref(struct kref *kref) { struct aa_profile *p = container_of(kref, struct aa_profile, base.count); - - free_profile(p); + call_rcu(&p->base.rcu, aa_free_profile_rcu); } /** @@ -733,12 +728,12 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat) profile->flags |= PFLAG_HAT; /* released on free_profile */ - profile->parent = aa_get_profile(parent); + rcu_assign_pointer(profile->parent, aa_get_profile(parent)); profile->ns = aa_get_namespace(parent->ns); - write_lock(&profile->ns->lock); + mutex_lock(&profile->ns->lock); __list_add_profile(&parent->base.profiles, profile); - write_unlock(&profile->ns->lock); + mutex_unlock(&profile->ns->lock); /* refcount released by caller */ return profile; @@ -754,7 +749,7 @@ fail: * @head: list to search (NOT NULL) * @name: name of profile (NOT NULL) * - * Requires: ns lock protecting list be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted profile ptr, or NULL if not found */ @@ -769,7 +764,7 @@ static struct aa_profile *__find_child(struct list_head *head, const char *name) * @name: name of profile (NOT NULL) * @len: length of @name substring to match * - * Requires: ns lock protecting list be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted profile ptr, or NULL if not found */ @@ -790,9 +785,9 @@ struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name) { struct aa_profile *profile; - read_lock(&parent->ns->lock); + rcu_read_lock(); profile = aa_get_profile(__find_child(&parent->base.profiles, name)); - read_unlock(&parent->ns->lock); + rcu_read_unlock(); /* refcount released by caller */ return profile; @@ -807,7 +802,7 @@ struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name) * that matches hname does not need to exist, in general this * is used to load a new profile. * - * Requires: ns->lock be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted policy or NULL if not found */ @@ -839,7 +834,7 @@ static struct aa_policy *__lookup_parent(struct aa_namespace *ns, * @base: base list to start looking up profile name from (NOT NULL) * @hname: hierarchical profile name (NOT NULL) * - * Requires: ns->lock be held + * Requires: rcu_read_lock be held * * Returns: unrefcounted profile pointer or NULL if not found * @@ -878,9 +873,11 @@ struct aa_profile *aa_lookup_profile(struct aa_namespace *ns, const char *hname) { struct aa_profile *profile; - read_lock(&ns->lock); - profile = aa_get_profile(__lookup_profile(&ns->base, hname)); - read_unlock(&ns->lock); + rcu_read_lock(); + do { + profile = __lookup_profile(&ns->base, hname); + } while (profile && !aa_get_profile_not0(profile)); + rcu_read_unlock(); /* the unconfined profile is not in the regular profile list */ if (!profile && strcmp(hname, "unconfined") == 0) @@ -1002,7 +999,7 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new) if (!list_empty(&old->base.profiles)) { LIST_HEAD(lh); - list_splice_init(&old->base.profiles, &lh); + list_splice_init_rcu(&old->base.profiles, &lh, synchronize_rcu); list_for_each_entry_safe(child, tmp, &lh, base.list) { struct aa_profile *p; @@ -1018,20 +1015,24 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new) /* inherit @child and its children */ /* TODO: update hname of inherited children */ /* list refcount transferred to @new */ - list_add(&child->base.list, &new->base.profiles); - aa_put_profile(child->parent); - child->parent = aa_get_profile(new); + p = rcu_dereference_protected(child->parent, + mutex_is_locked(&child->ns->lock)); + rcu_assign_pointer(child->parent, aa_get_profile(new)); + list_add_rcu(&child->base.list, &new->base.profiles); + aa_put_profile(p); } } - if (!new->parent) - new->parent = aa_get_profile(old->parent); + if (!rcu_access_pointer(new->parent)) { + struct aa_profile *parent = rcu_dereference(old->parent); + rcu_assign_pointer(new->parent, aa_get_profile(parent)); + } /* released by free_profile */ old->replacedby = aa_get_profile(new); if (list_empty(&new->base.list)) { /* new is not on a list already */ - list_replace_init(&old->base.list, &new->base.list); + list_replace_rcu(&old->base.list, &new->base.list); aa_get_profile(new); aa_put_profile(old); } else @@ -1099,7 +1100,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) goto fail; } - write_lock(&ns->lock); + mutex_lock(&ns->lock); /* setup parent and ns info */ list_for_each_entry(ent, &lh, list) { struct aa_policy *policy; @@ -1135,11 +1136,12 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) name = ent->new->base.hname; goto fail_lock; } - ent->new->parent = aa_get_profile(p); - } else if (policy != &ns->base) + rcu_assign_pointer(ent->new->parent, aa_get_profile(p)); + } else if (policy != &ns->base) { /* released on profile replacement or free_profile */ - ent->new->parent = aa_get_profile((struct aa_profile *) - policy); + struct aa_profile *p = (struct aa_profile *) policy; + rcu_assign_pointer(ent->new->parent, aa_get_profile(p)); + } } /* do actual replacement */ @@ -1156,13 +1158,16 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) } else if (ent->rename) { __replace_profile(ent->rename, ent->new); } else if (ent->new->parent) { - struct aa_profile *parent; - parent = aa_newest_version(ent->new->parent); + struct aa_profile *parent, *newest; + parent = rcu_dereference_protected(ent->new->parent, + mutex_is_locked(&ns->lock)); + newest = aa_newest_version(parent); + /* parent replaced in this atomic set? */ - if (parent != ent->new->parent) { - aa_get_profile(parent); - aa_put_profile(ent->new->parent); - ent->new->parent = parent; + if (newest != parent) { + aa_get_profile(newest); + aa_put_profile(parent); + rcu_assign_pointer(ent->new->parent, newest); } __list_add_profile(&parent->base.profiles, ent->new); } else @@ -1170,7 +1175,7 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) aa_load_ent_free(ent); } - write_unlock(&ns->lock); + mutex_unlock(&ns->lock); out: aa_put_namespace(ns); @@ -1180,7 +1185,7 @@ out: return size; fail_lock: - write_unlock(&ns->lock); + mutex_unlock(&ns->lock); fail: error = audit_policy(op, GFP_KERNEL, name, info, error); @@ -1235,12 +1240,12 @@ ssize_t aa_remove_profiles(char *fqname, size_t size) if (!name) { /* remove namespace - can only happen if fqname[0] == ':' */ - write_lock(&ns->parent->lock); + mutex_lock(&ns->parent->lock); __remove_namespace(ns); - write_unlock(&ns->parent->lock); + mutex_unlock(&ns->parent->lock); } else { /* remove profile */ - write_lock(&ns->lock); + mutex_lock(&ns->lock); profile = aa_get_profile(__lookup_profile(&ns->base, name)); if (!profile) { error = -ENOENT; @@ -1249,7 +1254,7 @@ ssize_t aa_remove_profiles(char *fqname, size_t size) } name = profile->base.hname; __remove_profile(profile); - write_unlock(&ns->lock); + mutex_unlock(&ns->lock); } /* don't fail removal if audit fails */ @@ -1259,7 +1264,7 @@ ssize_t aa_remove_profiles(char *fqname, size_t size) return size; fail_ns_lock: - write_unlock(&ns->lock); + mutex_unlock(&ns->lock); aa_put_namespace(ns); fail: -- cgit v1.2.3 From 77b071b34045a0c65d0e1f85f3d47fd2b8b7a8a1 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:07:43 -0700 Subject: 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 --- security/apparmor/context.c | 16 +++---- security/apparmor/domain.c | 4 +- security/apparmor/include/context.h | 15 +++---- security/apparmor/include/policy.h | 78 ++++++++++++++++++++++++---------- security/apparmor/lsm.c | 14 +++--- security/apparmor/policy.c | 85 ++++++++++++++++++++----------------- 6 files changed, 125 insertions(+), 87 deletions(-) (limited to 'security/apparmor/policy.c') 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; enum profile_mode mode; - u32 flags; + long flags; u32 path_flags; int size; @@ -250,6 +258,7 @@ static inline void aa_put_namespace(struct aa_namespace *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); struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat); void aa_free_profile_kref(struct kref *kref); @@ -265,24 +274,6 @@ ssize_t aa_remove_profiles(char *name, size_t size); #define unconfined(X) ((X)->flags & PFLAG_UNCONFINED) -/** - * aa_newest_version - find the newest version of @profile - * @profile: the profile to check for newer versions of (NOT NULL) - * - * Returns: newest version of @profile, if @profile is the newest version - * return @profile. - * - * NOTE: the profile returned is not refcounted, The refcount on @profile - * must be held until the caller decides what to do with the returned newest - * version. - */ -static inline struct aa_profile *aa_newest_version(struct aa_profile *profile) -{ - while (profile->replacedby) - profile = profile->replacedby; - - return profile; -} /** * aa_get_profile - increment refcount on profile @p @@ -334,6 +325,25 @@ static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile __rcu **p) return c; } +/** + * aa_get_newest_profile - find the newest version of @profile + * @profile: the profile to check for newer versions of + * + * Returns: refcounted newest version of @profile taking into account + * replacement, renames and removals + * return @profile. + */ +static inline struct aa_profile *aa_get_newest_profile(struct aa_profile *p) +{ + if (!p) + return NULL; + + if (PROFILE_INVALID(p)) + return aa_get_profile_rcu(&p->replacedby->profile); + + return aa_get_profile(p); +} + /** * aa_put_profile - decrement refcount on profile @p * @p: profile (MAYBE NULL) @@ -344,6 +354,30 @@ static inline void aa_put_profile(struct aa_profile *p) kref_put(&p->base.count, aa_free_profile_kref); } +static inline struct aa_replacedby *aa_get_replacedby(struct aa_replacedby *p) +{ + if (p) + kref_get(&(p->count)); + + return p; +} + +static inline void aa_put_replacedby(struct aa_replacedby *p) +{ + if (p) + kref_put(&p->count, aa_free_replacedby_kref); +} + +/* requires profile list write lock held */ +static inline void __aa_update_replacedby(struct aa_profile *orig, + struct aa_profile *new) +{ + struct aa_profile *tmp = rcu_dereference(orig->replacedby->profile); + rcu_assign_pointer(orig->replacedby->profile, aa_get_profile(new)); + orig->flags |= PFLAG_INVALID; + aa_put_profile(tmp); +} + static inline int AUDIT_MODE(struct aa_profile *profile) { if (aa_g_audit != AUDIT_NORMAL) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 96506dfe51ec..c8c148a738f7 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -508,19 +508,21 @@ static int apparmor_getprocattr(struct task_struct *task, char *name, /* released below */ const struct cred *cred = get_task_cred(task); struct aa_task_cxt *cxt = cred_cxt(cred); + struct aa_profile *profile = NULL; if (strcmp(name, "current") == 0) - error = aa_getprocattr(aa_newest_version(cxt->profile), - value); + profile = aa_get_newest_profile(cxt->profile); else if (strcmp(name, "prev") == 0 && cxt->previous) - error = aa_getprocattr(aa_newest_version(cxt->previous), - value); + profile = aa_get_newest_profile(cxt->previous); else if (strcmp(name, "exec") == 0 && cxt->onexec) - error = aa_getprocattr(aa_newest_version(cxt->onexec), - value); + profile = aa_get_newest_profile(cxt->onexec); else error = -EINVAL; + if (profile) + error = aa_getprocattr(profile, value); + + aa_put_profile(profile); put_cred(cred); return error; diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 25bbbb482bb6..41b8f275c626 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -469,7 +469,7 @@ static void __remove_profile(struct aa_profile *profile) /* release any children lists first */ __profile_list_release(&profile->base.profiles); /* released by free_profile */ - profile->replacedby = aa_get_profile(profile->ns->unconfined); + __aa_update_replacedby(profile, profile->ns->unconfined); __list_remove_profile(profile); } @@ -588,6 +588,23 @@ void __init aa_free_root_ns(void) aa_put_namespace(ns); } + +static void free_replacedby(struct aa_replacedby *r) +{ + if (r) { + aa_put_profile(rcu_dereference(r->profile)); + kzfree(r); + } +} + + +void aa_free_replacedby_kref(struct kref *kref) +{ + struct aa_replacedby *r = container_of(kref, struct aa_replacedby, + count); + free_replacedby(r); +} + /** * free_profile - free a profile * @profile: the profile to free (MAYBE NULL) @@ -600,8 +617,6 @@ void __init aa_free_root_ns(void) */ static void free_profile(struct aa_profile *profile) { - struct aa_profile *p; - AA_DEBUG("%s(%p)\n", __func__, profile); if (!profile) @@ -620,28 +635,7 @@ static void free_profile(struct aa_profile *profile) aa_put_dfa(profile->xmatch); aa_put_dfa(profile->policy.dfa); - - /* put the profile reference for replacedby, but not via - * put_profile(kref_put). - * replacedby can form a long chain that can result in cascading - * frees that blows the stack because kref_put makes a nested fn - * call (it looks like recursion, with free_profile calling - * free_profile) for each profile in the chain lp#1056078. - */ - for (p = profile->replacedby; p; ) { - if (atomic_dec_and_test(&p->base.count.refcount)) { - /* no more refs on p, grab its replacedby */ - struct aa_profile *next = p->replacedby; - /* break the chain */ - p->replacedby = NULL; - /* now free p, chain is broken */ - free_profile(p); - - /* follow up with next profile in the chain */ - p = next; - } else - break; - } + aa_put_replacedby(profile->replacedby); kzfree(profile); } @@ -682,13 +676,22 @@ struct aa_profile *aa_alloc_profile(const char *hname) if (!profile) return NULL; - if (!policy_init(&profile->base, NULL, hname)) { - kzfree(profile); - return NULL; - } + profile->replacedby = kzalloc(sizeof(struct aa_replacedby), GFP_KERNEL); + if (!profile->replacedby) + goto fail; + kref_init(&profile->replacedby->count); + + if (!policy_init(&profile->base, NULL, hname)) + goto fail; /* refcount released by caller */ return profile; + +fail: + kzfree(profile->replacedby); + kzfree(profile); + + return NULL; } /** @@ -985,6 +988,7 @@ static struct aa_profile *__list_lookup_parent(struct list_head *lh, * __replace_profile - replace @old with @new on a list * @old: profile to be replaced (NOT NULL) * @new: profile to replace @old with (NOT NULL) + * @share_replacedby: transfer @old->replacedby to @new * * Will duplicate and refcount elements that @new inherits from @old * and will inherit @old children. @@ -993,7 +997,8 @@ static struct aa_profile *__list_lookup_parent(struct list_head *lh, * * Requires: namespace list lock be held, or list not be shared */ -static void __replace_profile(struct aa_profile *old, struct aa_profile *new) +static void __replace_profile(struct aa_profile *old, struct aa_profile *new, + bool share_replacedby) { struct aa_profile *child, *tmp; @@ -1008,7 +1013,7 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new) p = __find_child(&new->base.profiles, child->base.name); if (p) { /* @p replaces @child */ - __replace_profile(child, p); + __replace_profile(child, p, share_replacedby); continue; } @@ -1027,8 +1032,11 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new) struct aa_profile *parent = rcu_dereference(old->parent); rcu_assign_pointer(new->parent, aa_get_profile(parent)); } - /* released by free_profile */ - old->replacedby = aa_get_profile(new); + __aa_update_replacedby(old, new); + if (share_replacedby) { + aa_put_replacedby(new->replacedby); + new->replacedby = aa_get_replacedby(old->replacedby); + } if (list_empty(&new->base.list)) { /* new is not on a list already */ @@ -1152,23 +1160,24 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) audit_policy(op, GFP_ATOMIC, ent->new->base.name, NULL, error); if (ent->old) { - __replace_profile(ent->old, ent->new); + __replace_profile(ent->old, ent->new, 1); if (ent->rename) - __replace_profile(ent->rename, ent->new); + __replace_profile(ent->rename, ent->new, 0); } else if (ent->rename) { - __replace_profile(ent->rename, ent->new); + __replace_profile(ent->rename, ent->new, 0); } else if (ent->new->parent) { struct aa_profile *parent, *newest; parent = rcu_dereference_protected(ent->new->parent, mutex_is_locked(&ns->lock)); - newest = aa_newest_version(parent); + newest = aa_get_newest_profile(parent); /* parent replaced in this atomic set? */ if (newest != parent) { aa_get_profile(newest); aa_put_profile(parent); rcu_assign_pointer(ent->new->parent, newest); - } + } else + aa_put_profile(newest); __list_add_profile(&parent->base.profiles, ent->new); } else __list_add_profile(&ns->base.profiles, ent->new); -- cgit v1.2.3 From fa2ac468db510c653499a47c1ec3deb045bf4763 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:08:43 -0700 Subject: apparmor: update how unconfined is handled 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 Acked-by: Seth Arnold --- security/apparmor/domain.c | 2 +- security/apparmor/include/policy.h | 80 +++++++++++++++++++------------------- security/apparmor/policy.c | 68 +++++++++++++------------------- 3 files changed, 67 insertions(+), 83 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 5488d095af6f..bc28f2670ee4 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 e9f2baf4467e..1ddd5e5728b8 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -68,6 +68,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 */ @@ -78,7 +79,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 @@ -86,7 +86,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; @@ -157,6 +156,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 @@ -189,6 +189,7 @@ struct aa_replacedby { */ struct aa_profile { struct aa_policy base; + struct kref count; struct aa_profile __rcu *parent; struct aa_namespace *ns; @@ -223,40 +224,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); @@ -285,7 +252,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; } @@ -299,7 +266,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; @@ -319,7 +286,7 @@ static inline struct aa_profile *aa_get_profile_rcu(struct aa_profile __rcu **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; @@ -350,8 +317,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) @@ -378,6 +349,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 41b8f275c626..0ceee967434c 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,20 +323,33 @@ 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); } +/** + * aa_free_namespace_rcu - free aa_namespace by rcu + * @head: rcu_head callback for freeing of a profile (NOT NULL) + * + * rcu_head is to the unconfined profile associated with the namespace + */ +static void aa_free_namespace_rcu(struct rcu_head *head) +{ + struct aa_profile *p = container_of(head, struct aa_profile, base.rcu); + free_namespace(p->ns); +} + /** * aa_free_namespace_kref - free aa_namespace by kref (see aa_put_namespace) * @kr: kref callback for freeing of a namespace (NOT NULL) + * + * kref is to the unconfined profile associated with the namespace */ 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); + call_rcu(&p->base.rcu, aa_free_namespace_rcu); } /** @@ -494,8 +503,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; @@ -506,30 +513,11 @@ 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); } -static void aa_put_ns_rcu(struct rcu_head *head) -{ - struct aa_namespace *ns = container_of(head, struct aa_namespace, - base.rcu); - /* release ns->base.list ref */ - aa_put_namespace(ns); -} - /** * __remove_namespace - remove a namespace and all its children * @ns: namespace to be removed (NOT NULL) @@ -540,10 +528,8 @@ static void __remove_namespace(struct aa_namespace *ns) { /* remove ns from namespace list */ list_del_rcu(&ns->base.list); - destroy_namespace(ns); - - call_rcu(&ns->base.rcu, aa_put_ns_rcu); + aa_put_namespace(ns); } /** @@ -656,8 +642,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); } @@ -683,6 +668,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; @@ -884,7 +870,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; -- cgit v1.2.3 From 742058b0f3a2ed32e2a7349aff97989dc4e32452 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:10:43 -0700 Subject: apparmor: rework namespace free path namespaces now completely use the unconfined profile to track the refcount and rcu freeing cycle. So rework the code to simplify (track everything through the profile path right up to the end), and move the rcu_head from policy base to profile as the namespace no longer needs it. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/include/policy.h | 12 ++++-------- security/apparmor/policy.c | 33 ++++++--------------------------- 2 files changed, 10 insertions(+), 35 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 1ddd5e5728b8..4eafdd88f44e 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -80,7 +80,6 @@ struct aa_profile; * @name: name of the object * @hname - The hierarchical name * @list: list policy object is on - * @rcu: rcu head used when removing from @list * @profiles: head of the profiles list contained in the object */ struct aa_policy { @@ -88,7 +87,6 @@ struct aa_policy { char *hname; struct list_head list; struct list_head profiles; - struct rcu_head rcu; }; /* struct aa_ns_acct - accounting of profiles in namespace @@ -157,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 + * @rcu: rcu head used when removing from @list * @parent: parent of profile * @ns: namespace the profile is in * @replacedby: is set to the profile that replaced this profile @@ -190,6 +189,7 @@ struct aa_replacedby { struct aa_profile { struct aa_policy base; struct kref count; + struct rcu_head rcu; struct aa_profile __rcu *parent; struct aa_namespace *ns; @@ -317,12 +317,8 @@ 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) { - if (p->flags & PFLAG_NS_COUNT) - kref_put(&p->count, aa_free_namespace_kref); - else - kref_put(&p->count, aa_free_profile_kref); - } + if (p) + kref_put(&p->count, aa_free_profile_kref); } static inline struct aa_replacedby *aa_get_replacedby(struct aa_replacedby *p) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 0ceee967434c..aee2e71827cd 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -328,30 +328,6 @@ static void free_namespace(struct aa_namespace *ns) kzfree(ns); } -/** - * aa_free_namespace_rcu - free aa_namespace by rcu - * @head: rcu_head callback for freeing of a profile (NOT NULL) - * - * rcu_head is to the unconfined profile associated with the namespace - */ -static void aa_free_namespace_rcu(struct rcu_head *head) -{ - struct aa_profile *p = container_of(head, struct aa_profile, base.rcu); - free_namespace(p->ns); -} - -/** - * aa_free_namespace_kref - free aa_namespace by kref (see aa_put_namespace) - * @kr: kref callback for freeing of a namespace (NOT NULL) - * - * kref is to the unconfined profile associated with the namespace - */ -void aa_free_namespace_kref(struct kref *kref) -{ - struct aa_profile *p = container_of(kref, struct aa_profile, count); - call_rcu(&p->base.rcu, aa_free_namespace_rcu); -} - /** * __aa_find_namespace - find a namespace on a list by @name * @head: list to search for namespace on (NOT NULL) @@ -632,8 +608,11 @@ static void free_profile(struct aa_profile *profile) */ static void aa_free_profile_rcu(struct rcu_head *head) { - struct aa_profile *p = container_of(head, struct aa_profile, base.rcu); - free_profile(p); + struct aa_profile *p = container_of(head, struct aa_profile, rcu); + if (p->flags & PFLAG_NS_COUNT) + free_namespace(p->ns); + else + free_profile(p); } /** @@ -643,7 +622,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, count); - call_rcu(&p->base.rcu, aa_free_profile_rcu); + call_rcu(&p->rcu, aa_free_profile_rcu); } /** -- cgit v1.2.3 From 8651e1d6572bc2c061073f05fabcd7175789259d Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:11:43 -0700 Subject: apparmor: make free_profile available outside of policy.c Signed-off-by: John Johansen --- security/apparmor/include/policy.h | 1 + security/apparmor/policy.c | 9 ++++----- security/apparmor/policy_unpack.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 4eafdd88f44e..8a68226ff7f7 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -228,6 +228,7 @@ struct aa_namespace *aa_find_namespace(struct aa_namespace *root, void aa_free_replacedby_kref(struct kref *kref); struct aa_profile *aa_alloc_profile(const char *name); struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat); +void aa_free_profile(struct aa_profile *profile); void aa_free_profile_kref(struct kref *kref); struct aa_profile *aa_find_child(struct aa_profile *parent, const char *name); struct aa_profile *aa_lookup_profile(struct aa_namespace *ns, const char *name); diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index aee2e71827cd..7a80b0c7e0ce 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -307,7 +307,6 @@ 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) @@ -324,7 +323,7 @@ static void free_namespace(struct aa_namespace *ns) aa_put_namespace(ns->parent); ns->unconfined->ns = NULL; - free_profile(ns->unconfined); + aa_free_profile(ns->unconfined); kzfree(ns); } @@ -568,7 +567,7 @@ void aa_free_replacedby_kref(struct kref *kref) } /** - * free_profile - free a profile + * aa_free_profile - free a profile * @profile: the profile to free (MAYBE NULL) * * Free a profile, its hats and null_profile. All references to the profile, @@ -577,7 +576,7 @@ void aa_free_replacedby_kref(struct kref *kref) * If the profile was referenced from a task context, free_profile() will * be called from an rcu callback routine, so we must not sleep here. */ -static void free_profile(struct aa_profile *profile) +void aa_free_profile(struct aa_profile *profile) { AA_DEBUG("%s(%p)\n", __func__, profile); @@ -612,7 +611,7 @@ static void aa_free_profile_rcu(struct rcu_head *head) if (p->flags & PFLAG_NS_COUNT) free_namespace(p->ns); else - free_profile(p); + aa_free_profile(p); } /** diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 080a26b11f01..ce15313896ee 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -616,7 +616,7 @@ fail: else if (!name) name = "unknown"; audit_iface(profile, name, "failed to unpack profile", e, error); - aa_put_profile(profile); + aa_free_profile(profile); return ERR_PTR(error); } @@ -763,7 +763,7 @@ int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns) error = verify_profile(profile); if (error) { - aa_put_profile(profile); + aa_free_profile(profile); goto fail; } -- cgit v1.2.3 From 038165070aa55375d4bdd2f84b34a486feca63d6 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:12:43 -0700 Subject: apparmor: allow setting any profile into the unconfined state Allow emulating the default profile behavior from boot, by allowing loading of a profile in the unconfined state into a new NS. Signed-off-by: John Johansen Acked-by: Seth Arnold --- security/apparmor/domain.c | 4 ++-- security/apparmor/include/policy.h | 6 +++--- security/apparmor/include/policy_unpack.h | 7 +++++++ security/apparmor/policy.c | 6 ++++-- security/apparmor/policy_unpack.c | 8 ++++++-- 5 files changed, 22 insertions(+), 9 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index bc28f2670ee4..26c607c971f5 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -371,8 +371,8 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm) error = aa_path_name(&bprm->file->f_path, profile->path_flags, &buffer, &name, &info); if (error) { - if (profile->flags & - (PFLAG_IX_ON_NAME_ERROR | PFLAG_UNCONFINED)) + if (unconfined(profile) || + (profile->flags & PFLAG_IX_ON_NAME_ERROR)) error = 0; name = bprm->filename; goto audit; diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 8a68226ff7f7..65662e3c75cf 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -56,11 +56,11 @@ enum profile_mode { APPARMOR_ENFORCE, /* enforce access rules */ APPARMOR_COMPLAIN, /* allow and log access violations */ APPARMOR_KILL, /* kill task on access violation */ + APPARMOR_UNCONFINED, /* profile set to unconfined */ }; enum profile_flags { PFLAG_HAT = 1, /* profile is a hat */ - PFLAG_UNCONFINED = 2, /* profile is an unconfined profile */ PFLAG_NULL = 4, /* profile is null learning profile */ PFLAG_IX_ON_NAME_ERROR = 8, /* fallback to ix on name lookup fail */ PFLAG_IMMUTABLE = 0x10, /* don't allow changes/replacement */ @@ -199,7 +199,7 @@ struct aa_profile { struct aa_dfa *xmatch; int xmatch_len; enum audit_mode audit; - enum profile_mode mode; + long mode; long flags; u32 path_flags; int size; @@ -240,7 +240,7 @@ ssize_t aa_remove_profiles(char *name, size_t size); #define PROF_ADD 1 #define PROF_REPLACE 0 -#define unconfined(X) ((X)->flags & PFLAG_UNCONFINED) +#define unconfined(X) ((X)->mode == APPARMOR_UNCONFINED) /** diff --git a/security/apparmor/include/policy_unpack.h b/security/apparmor/include/policy_unpack.h index 0d7ad722b8ff..c214fb88b1bc 100644 --- a/security/apparmor/include/policy_unpack.h +++ b/security/apparmor/include/policy_unpack.h @@ -27,6 +27,13 @@ struct aa_load_ent { void aa_load_ent_free(struct aa_load_ent *ent); struct aa_load_ent *aa_load_ent_alloc(void); +#define PACKED_FLAG_HAT 1 + +#define PACKED_MODE_ENFORCE 0 +#define PACKED_MODE_COMPLAIN 1 +#define PACKED_MODE_KILL 2 +#define PACKED_MODE_UNCONFINED 3 + int aa_unpack(void *udata, size_t size, struct list_head *lh, const char **ns); #endif /* __POLICY_INTERFACE_H */ diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 7a80b0c7e0ce..2e4e2ecb25bc 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -96,6 +96,7 @@ const char *const profile_mode_names[] = { "enforce", "complain", "kill", + "unconfined", }; /** @@ -290,8 +291,9 @@ static struct aa_namespace *alloc_namespace(const char *prefix, if (!ns->unconfined) goto fail_unconfined; - ns->unconfined->flags = PFLAG_UNCONFINED | PFLAG_IX_ON_NAME_ERROR | - PFLAG_IMMUTABLE | PFLAG_NS_COUNT; + ns->unconfined->flags = PFLAG_IX_ON_NAME_ERROR | + PFLAG_IMMUTABLE | PFLAG_NS_COUNT; + ns->unconfined->mode = APPARMOR_UNCONFINED; /* ns and ns->unconfined share ns->unconfined refcount */ ns->unconfined->ns = ns; diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index ce15313896ee..cac0aa075787 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -511,12 +511,16 @@ static struct aa_profile *unpack_profile(struct aa_ext *e) goto fail; if (!unpack_u32(e, &tmp, NULL)) goto fail; - if (tmp) + if (tmp & PACKED_FLAG_HAT) profile->flags |= PFLAG_HAT; if (!unpack_u32(e, &tmp, NULL)) goto fail; - if (tmp) + if (tmp == PACKED_MODE_COMPLAIN) profile->mode = APPARMOR_COMPLAIN; + else if (tmp == PACKED_MODE_KILL) + profile->mode = APPARMOR_KILL; + else if (tmp == PACKED_MODE_UNCONFINED) + profile->mode = APPARMOR_UNCONFINED; if (!unpack_u32(e, &tmp, NULL)) goto fail; if (tmp) -- cgit v1.2.3 From 0d259f043f5f60f74c4fd020aac190cb6450e918 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 10 Jul 2013 21:13:43 -0700 Subject: apparmor: add interface files for profiles and namespaces Add basic interface files to access namespace and profile information. The interface files are created when a profile is loaded and removed when the profile or namespace is removed. Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 322 ++++++++++++++++++++++++++++++++- security/apparmor/include/apparmorfs.h | 38 ++++ security/apparmor/include/audit.h | 1 - security/apparmor/include/policy.h | 21 ++- security/apparmor/lsm.c | 6 +- security/apparmor/policy.c | 75 ++++++-- security/apparmor/procattr.c | 2 +- 7 files changed, 436 insertions(+), 29 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 3ed56e21a9fd..0fdd08c6ea59 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -12,6 +12,7 @@ * License. */ +#include #include #include #include @@ -27,6 +28,45 @@ #include "include/policy.h" #include "include/resource.h" +/** + * aa_mangle_name - mangle a profile name to std profile layout form + * @name: profile name to mangle (NOT NULL) + * @target: buffer to store mangled name, same length as @name (MAYBE NULL) + * + * Returns: length of mangled name + */ +static int mangle_name(char *name, char *target) +{ + char *t = target; + + while (*name == '/' || *name == '.') + name++; + + if (target) { + for (; *name; name++) { + if (*name == '/') + *(t)++ = '.'; + else if (isspace(*name)) + *(t)++ = '_'; + else if (isalnum(*name) || strchr("._-", *name)) + *(t)++ = *name; + } + + *t = 0; + } else { + int len = 0; + for (; *name; name++) { + if (isalnum(*name) || isspace(*name) || + strchr("/._-", *name)) + len++; + } + + return len; + } + + return t - target; +} + /** * aa_simple_write_to_buffer - common routine for getting policy from user * @op: operation doing the user buffer copy @@ -182,8 +222,263 @@ const struct file_operations aa_fs_seq_file_ops = { .release = single_release, }; -/** Base file system setup **/ +static int aa_fs_seq_profile_open(struct inode *inode, struct file *file, + int (*show)(struct seq_file *, void *)) +{ + struct aa_replacedby *r = aa_get_replacedby(inode->i_private); + int error = single_open(file, show, r); + + if (error) { + file->private_data = NULL; + aa_put_replacedby(r); + } + + return error; +} + +static int aa_fs_seq_profile_release(struct inode *inode, struct file *file) +{ + struct seq_file *seq = (struct seq_file *) file->private_data; + if (seq) + aa_put_replacedby(seq->private); + return single_release(inode, file); +} + +static int aa_fs_seq_profname_show(struct seq_file *seq, void *v) +{ + struct aa_replacedby *r = seq->private; + struct aa_profile *profile = aa_get_profile_rcu(&r->profile); + seq_printf(seq, "%s\n", profile->base.name); + aa_put_profile(profile); + + return 0; +} + +static int aa_fs_seq_profname_open(struct inode *inode, struct file *file) +{ + return aa_fs_seq_profile_open(inode, file, aa_fs_seq_profname_show); +} + +static const struct file_operations aa_fs_profname_fops = { + .owner = THIS_MODULE, + .open = aa_fs_seq_profname_open, + .read = seq_read, + .llseek = seq_lseek, + .release = aa_fs_seq_profile_release, +}; + +static int aa_fs_seq_profmode_show(struct seq_file *seq, void *v) +{ + struct aa_replacedby *r = seq->private; + struct aa_profile *profile = aa_get_profile_rcu(&r->profile); + seq_printf(seq, "%s\n", aa_profile_mode_names[profile->mode]); + aa_put_profile(profile); + + return 0; +} + +static int aa_fs_seq_profmode_open(struct inode *inode, struct file *file) +{ + return aa_fs_seq_profile_open(inode, file, aa_fs_seq_profmode_show); +} + +static const struct file_operations aa_fs_profmode_fops = { + .owner = THIS_MODULE, + .open = aa_fs_seq_profmode_open, + .read = seq_read, + .llseek = seq_lseek, + .release = aa_fs_seq_profile_release, +}; + +/** fns to setup dynamic per profile/namespace files **/ +void __aa_fs_profile_rmdir(struct aa_profile *profile) +{ + struct aa_profile *child; + int i; + + if (!profile) + return; + + list_for_each_entry(child, &profile->base.profiles, base.list) + __aa_fs_profile_rmdir(child); + + for (i = AAFS_PROF_SIZEOF - 1; i >= 0; --i) { + struct aa_replacedby *r; + if (!profile->dents[i]) + continue; + + r = profile->dents[i]->d_inode->i_private; + securityfs_remove(profile->dents[i]); + aa_put_replacedby(r); + profile->dents[i] = NULL; + } +} + +void __aa_fs_profile_migrate_dents(struct aa_profile *old, + struct aa_profile *new) +{ + int i; + + for (i = 0; i < AAFS_PROF_SIZEOF; i++) { + new->dents[i] = old->dents[i]; + old->dents[i] = NULL; + } +} + +static struct dentry *create_profile_file(struct dentry *dir, const char *name, + struct aa_profile *profile, + const struct file_operations *fops) +{ + struct aa_replacedby *r = aa_get_replacedby(profile->replacedby); + struct dentry *dent; + + dent = securityfs_create_file(name, S_IFREG | 0444, dir, r, fops); + if (IS_ERR(dent)) + aa_put_replacedby(r); + + return dent; +} + +/* requires lock be held */ +int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent) +{ + struct aa_profile *child; + struct dentry *dent = NULL, *dir; + int error; + + if (!parent) { + struct aa_profile *p; + p = aa_deref_parent(profile); + dent = prof_dir(p); + /* adding to parent that previously didn't have children */ + dent = securityfs_create_dir("profiles", dent); + if (IS_ERR(dent)) + goto fail; + prof_child_dir(p) = parent = dent; + } + + if (!profile->dirname) { + int len, id_len; + len = mangle_name(profile->base.name, NULL); + id_len = snprintf(NULL, 0, ".%ld", profile->ns->uniq_id); + + profile->dirname = kmalloc(len + id_len + 1, GFP_KERNEL); + if (!profile->dirname) + goto fail; + + mangle_name(profile->base.name, profile->dirname); + sprintf(profile->dirname + len, ".%ld", profile->ns->uniq_id++); + } + + dent = securityfs_create_dir(profile->dirname, parent); + if (IS_ERR(dent)) + goto fail; + prof_dir(profile) = dir = dent; + + dent = create_profile_file(dir, "name", profile, &aa_fs_profname_fops); + if (IS_ERR(dent)) + goto fail; + profile->dents[AAFS_PROF_NAME] = dent; + + dent = create_profile_file(dir, "mode", profile, &aa_fs_profmode_fops); + if (IS_ERR(dent)) + goto fail; + profile->dents[AAFS_PROF_MODE] = dent; + + list_for_each_entry(child, &profile->base.profiles, base.list) { + error = __aa_fs_profile_mkdir(child, prof_child_dir(profile)); + if (error) + goto fail2; + } + + return 0; + +fail: + error = PTR_ERR(dent); + +fail2: + __aa_fs_profile_rmdir(profile); + + return error; +} + +void __aa_fs_namespace_rmdir(struct aa_namespace *ns) +{ + struct aa_namespace *sub; + struct aa_profile *child; + int i; + + if (!ns) + return; + + list_for_each_entry(child, &ns->base.profiles, base.list) + __aa_fs_profile_rmdir(child); + + list_for_each_entry(sub, &ns->sub_ns, base.list) { + mutex_lock(&sub->lock); + __aa_fs_namespace_rmdir(sub); + mutex_unlock(&sub->lock); + } + + for (i = AAFS_NS_SIZEOF - 1; i >= 0; --i) { + securityfs_remove(ns->dents[i]); + ns->dents[i] = NULL; + } +} + +int __aa_fs_namespace_mkdir(struct aa_namespace *ns, struct dentry *parent, + const char *name) +{ + struct aa_namespace *sub; + struct aa_profile *child; + struct dentry *dent, *dir; + int error; + + if (!name) + name = ns->base.name; + + dent = securityfs_create_dir(name, parent); + if (IS_ERR(dent)) + goto fail; + ns_dir(ns) = dir = dent; + + dent = securityfs_create_dir("profiles", dir); + if (IS_ERR(dent)) + goto fail; + ns_subprofs_dir(ns) = dent; + dent = securityfs_create_dir("namespaces", dir); + if (IS_ERR(dent)) + goto fail; + ns_subns_dir(ns) = dent; + + list_for_each_entry(child, &ns->base.profiles, base.list) { + error = __aa_fs_profile_mkdir(child, ns_subprofs_dir(ns)); + if (error) + goto fail2; + } + + list_for_each_entry(sub, &ns->sub_ns, base.list) { + mutex_lock(&sub->lock); + error = __aa_fs_namespace_mkdir(sub, ns_subns_dir(ns), NULL); + mutex_unlock(&sub->lock); + if (error) + goto fail2; + } + + return 0; + +fail: + error = PTR_ERR(dent); + +fail2: + __aa_fs_namespace_rmdir(ns); + + return error; +} + + +/** Base file system setup **/ static struct aa_fs_entry aa_fs_entry_file[] = { AA_FS_FILE_STRING("mask", "create read write exec append mmap_exec " \ "link lock"), @@ -246,6 +541,7 @@ static int __init aafs_create_file(struct aa_fs_entry *fs_file, return error; } +static void __init aafs_remove_dir(struct aa_fs_entry *fs_dir); /** * aafs_create_dir - recursively create a directory entry in the securityfs * @fs_dir: aa_fs_entry (and all child entries) to build (NOT NULL) @@ -256,17 +552,16 @@ static int __init aafs_create_file(struct aa_fs_entry *fs_file, static int __init aafs_create_dir(struct aa_fs_entry *fs_dir, struct dentry *parent) { - int error; struct aa_fs_entry *fs_file; + struct dentry *dir; + int error; - fs_dir->dentry = securityfs_create_dir(fs_dir->name, parent); - if (IS_ERR(fs_dir->dentry)) { - error = PTR_ERR(fs_dir->dentry); - fs_dir->dentry = NULL; - goto failed; - } + dir = securityfs_create_dir(fs_dir->name, parent); + if (IS_ERR(dir)) + return PTR_ERR(dir); + fs_dir->dentry = dir; - for (fs_file = fs_dir->v.files; fs_file->name; ++fs_file) { + for (fs_file = fs_dir->v.files; fs_file && fs_file->name; ++fs_file) { if (fs_file->v_type == AA_FS_TYPE_DIR) error = aafs_create_dir(fs_file, fs_dir->dentry); else @@ -278,6 +573,8 @@ static int __init aafs_create_dir(struct aa_fs_entry *fs_dir, return 0; failed: + aafs_remove_dir(fs_dir); + return error; } @@ -302,7 +599,7 @@ static void __init aafs_remove_dir(struct aa_fs_entry *fs_dir) { struct aa_fs_entry *fs_file; - for (fs_file = fs_dir->v.files; fs_file->name; ++fs_file) { + for (fs_file = fs_dir->v.files; fs_file && fs_file->name; ++fs_file) { if (fs_file->v_type == AA_FS_TYPE_DIR) aafs_remove_dir(fs_file); else @@ -346,6 +643,11 @@ static int __init aa_create_aafs(void) if (error) goto error; + error = __aa_fs_namespace_mkdir(root_ns, aa_fs_entry.dentry, + "policy"); + if (error) + goto error; + /* TODO: add support for apparmorfs_null and apparmorfs_mnt */ /* Report that AppArmor fs is enabled */ diff --git a/security/apparmor/include/apparmorfs.h b/security/apparmor/include/apparmorfs.h index 7ea4769fab3f..2494e112f2bf 100644 --- a/security/apparmor/include/apparmorfs.h +++ b/security/apparmor/include/apparmorfs.h @@ -61,4 +61,42 @@ extern const struct file_operations aa_fs_seq_file_ops; extern void __init aa_destroy_aafs(void); +struct aa_profile; +struct aa_namespace; + +enum aafs_ns_type { + AAFS_NS_DIR, + AAFS_NS_PROFS, + AAFS_NS_NS, + AAFS_NS_COUNT, + AAFS_NS_MAX_COUNT, + AAFS_NS_SIZE, + AAFS_NS_MAX_SIZE, + AAFS_NS_OWNER, + AAFS_NS_SIZEOF, +}; + +enum aafs_prof_type { + AAFS_PROF_DIR, + AAFS_PROF_PROFS, + AAFS_PROF_NAME, + AAFS_PROF_MODE, + AAFS_PROF_SIZEOF, +}; + +#define ns_dir(X) ((X)->dents[AAFS_NS_DIR]) +#define ns_subns_dir(X) ((X)->dents[AAFS_NS_NS]) +#define ns_subprofs_dir(X) ((X)->dents[AAFS_NS_PROFS]) + +#define prof_dir(X) ((X)->dents[AAFS_PROF_DIR]) +#define prof_child_dir(X) ((X)->dents[AAFS_PROF_PROFS]) + +void __aa_fs_profile_rmdir(struct aa_profile *profile); +void __aa_fs_profile_migrate_dents(struct aa_profile *old, + struct aa_profile *new); +int __aa_fs_profile_mkdir(struct aa_profile *profile, struct dentry *parent); +void __aa_fs_namespace_rmdir(struct aa_namespace *ns); +int __aa_fs_namespace_mkdir(struct aa_namespace *ns, struct dentry *parent, + const char *name); + #endif /* __AA_APPARMORFS_H */ diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h index 69d8cae634e7..30e8d7687259 100644 --- a/security/apparmor/include/audit.h +++ b/security/apparmor/include/audit.h @@ -27,7 +27,6 @@ struct aa_profile; extern const char *const audit_mode_names[]; #define AUDIT_MAX_INDEX 5 - enum audit_mode { AUDIT_NORMAL, /* follow normal auditing of accesses */ AUDIT_QUIET_DENIED, /* quiet all denied access messages */ diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 65662e3c75cf..5c72231d1c42 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -29,8 +29,8 @@ #include "file.h" #include "resource.h" -extern const char *const profile_mode_names[]; -#define APPARMOR_NAMES_MAX_INDEX 3 +extern const char *const aa_profile_mode_names[]; +#define APPARMOR_MODE_NAMES_MAX_INDEX 4 #define PROFILE_MODE(_profile, _mode) \ ((aa_g_profile_mode == (_mode)) || \ @@ -110,6 +110,8 @@ struct aa_ns_acct { * @unconfined: special unconfined profile for the namespace * @sub_ns: list of namespaces under the current namespace. * @uniq_null: uniq value used for null learning profiles + * @uniq_id: a unique id count for the profiles in the namespace + * @dents: dentries for the namespaces file entries in apparmorfs * * An aa_namespace defines the set profiles that are searched to determine * which profile to attach to a task. Profiles can not be shared between @@ -133,6 +135,9 @@ struct aa_namespace { struct aa_profile *unconfined; struct list_head sub_ns; atomic_t uniq_null; + long uniq_id; + + struct dentry *dents[AAFS_NS_SIZEOF]; }; /* struct aa_policydb - match engine for a policy @@ -172,6 +177,9 @@ struct aa_replacedby { * @caps: capabilities for the profile * @rlimits: rlimits for the profile * + * @dents: dentries for the profiles file entries in apparmorfs + * @dirname: name of the profile dir in apparmorfs + * * The AppArmor profile contains the basic confinement data. Each profile * has a name, and exists in a namespace. The @name and @exec_match are * used to determine profile attachment against unconfined tasks. All other @@ -208,6 +216,9 @@ struct aa_profile { struct aa_file_rules file; struct aa_caps caps; struct aa_rlimit rlimits; + + char *dirname; + struct dentry *dents[AAFS_PROF_SIZEOF]; }; extern struct aa_namespace *root_ns; @@ -243,6 +254,12 @@ ssize_t aa_remove_profiles(char *name, size_t size); #define unconfined(X) ((X)->mode == APPARMOR_UNCONFINED) +static inline struct aa_profile *aa_deref_parent(struct aa_profile *p) +{ + return rcu_dereference_protected(p->parent, + mutex_is_locked(&p->ns->lock)); +} + /** * aa_get_profile - increment refcount on profile @p * @p: profile (MAYBE NULL) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index c8c148a738f7..edb3ce15e92d 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -843,7 +843,7 @@ static int param_get_mode(char *buffer, struct kernel_param *kp) if (!apparmor_enabled) return -EINVAL; - return sprintf(buffer, "%s", profile_mode_names[aa_g_profile_mode]); + return sprintf(buffer, "%s", aa_profile_mode_names[aa_g_profile_mode]); } static int param_set_mode(const char *val, struct kernel_param *kp) @@ -858,8 +858,8 @@ static int param_set_mode(const char *val, struct kernel_param *kp) if (!val) return -EINVAL; - for (i = 0; i < APPARMOR_NAMES_MAX_INDEX; i++) { - if (strcmp(val, profile_mode_names[i]) == 0) { + for (i = 0; i < APPARMOR_MODE_NAMES_MAX_INDEX; i++) { + if (strcmp(val, aa_profile_mode_names[i]) == 0) { aa_g_profile_mode = i; return 0; } diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 2e4e2ecb25bc..6172509fa2b7 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -92,7 +92,7 @@ /* root profile namespace */ struct aa_namespace *root_ns; -const char *const profile_mode_names[] = { +const char *const aa_profile_mode_names[] = { "enforce", "complain", "kill", @@ -394,7 +394,13 @@ static struct aa_namespace *aa_prepare_namespace(const char *name) ns = alloc_namespace(root->base.hname, name); if (!ns) goto out; - /* add parent ref */ + if (__aa_fs_namespace_mkdir(ns, ns_subns_dir(root), name)) { + AA_ERROR("Failed to create interface for ns %s\n", + ns->base.name); + free_namespace(ns); + ns = NULL; + goto out; + } ns->parent = aa_get_namespace(root); list_add_rcu(&ns->base.list, &root->sub_ns); /* add list ref */ @@ -456,6 +462,7 @@ static void __remove_profile(struct aa_profile *profile) __profile_list_release(&profile->base.profiles); /* released by free_profile */ __aa_update_replacedby(profile, profile->ns->unconfined); + __aa_fs_profile_rmdir(profile); __list_remove_profile(profile); } @@ -492,6 +499,7 @@ static void destroy_namespace(struct aa_namespace *ns) if (ns->parent) __aa_update_replacedby(ns->unconfined, ns->parent->unconfined); + __aa_fs_namespace_rmdir(ns); mutex_unlock(&ns->lock); } @@ -596,6 +604,7 @@ void aa_free_profile(struct aa_profile *profile) aa_free_cap_rules(&profile->caps); aa_free_rlimit_rules(&profile->rlimits); + kzfree(profile->dirname); aa_put_dfa(profile->xmatch); aa_put_dfa(profile->policy.dfa); aa_put_replacedby(profile->replacedby); @@ -986,8 +995,7 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new, /* inherit @child and its children */ /* TODO: update hname of inherited children */ /* list refcount transferred to @new */ - p = rcu_dereference_protected(child->parent, - mutex_is_locked(&child->ns->lock)); + p = aa_deref_parent(child); rcu_assign_pointer(child->parent, aa_get_profile(new)); list_add_rcu(&child->base.list, &new->base.profiles); aa_put_profile(p); @@ -995,14 +1003,18 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new, } if (!rcu_access_pointer(new->parent)) { - struct aa_profile *parent = rcu_dereference(old->parent); + struct aa_profile *parent = aa_deref_parent(old); rcu_assign_pointer(new->parent, aa_get_profile(parent)); } __aa_update_replacedby(old, new); if (share_replacedby) { aa_put_replacedby(new->replacedby); new->replacedby = aa_get_replacedby(old->replacedby); - } + } else if (!rcu_access_pointer(new->replacedby->profile)) + /* aafs interface uses replacedby */ + rcu_assign_pointer(new->replacedby->profile, + aa_get_profile(new)); + __aa_fs_profile_migrate_dents(old, new); if (list_empty(&new->base.list)) { /* new is not on a list already */ @@ -1118,7 +1130,33 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) } } - /* do actual replacement */ + /* create new fs entries for introspection if needed */ + list_for_each_entry(ent, &lh, list) { + if (ent->old) { + /* inherit old interface files */ + + /* if (ent->rename) + TODO: support rename */ + /* } else if (ent->rename) { + TODO: support rename */ + } else { + struct dentry *parent; + if (rcu_access_pointer(ent->new->parent)) { + struct aa_profile *p; + p = aa_deref_parent(ent->new); + parent = prof_child_dir(p); + } else + parent = ns_subprofs_dir(ent->new->ns); + error = __aa_fs_profile_mkdir(ent->new, parent); + } + + if (error) { + info = "failed to create "; + goto fail_lock; + } + } + + /* Done with checks that may fail - do actual replacement */ list_for_each_entry_safe(ent, tmp, &lh, list) { list_del_init(&ent->list); op = (!ent->old && !ent->rename) ? OP_PROF_LOAD : OP_PROF_REPL; @@ -1127,14 +1165,21 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) if (ent->old) { __replace_profile(ent->old, ent->new, 1); - if (ent->rename) + if (ent->rename) { + /* aafs interface uses replacedby */ + struct aa_replacedby *r = ent->new->replacedby; + rcu_assign_pointer(r->profile, + aa_get_profile(ent->new)); __replace_profile(ent->rename, ent->new, 0); + } } else if (ent->rename) { + /* aafs interface uses replacedby */ + rcu_assign_pointer(ent->new->replacedby->profile, + aa_get_profile(ent->new)); __replace_profile(ent->rename, ent->new, 0); } else if (ent->new->parent) { struct aa_profile *parent, *newest; - parent = rcu_dereference_protected(ent->new->parent, - mutex_is_locked(&ns->lock)); + parent = aa_deref_parent(ent->new); newest = aa_get_newest_profile(parent); /* parent replaced in this atomic set? */ @@ -1144,10 +1189,16 @@ ssize_t aa_replace_profiles(void *udata, size_t size, bool noreplace) rcu_assign_pointer(ent->new->parent, newest); } else aa_put_profile(newest); + /* aafs interface uses replacedby */ + rcu_assign_pointer(ent->new->replacedby->profile, + aa_get_profile(ent->new)); __list_add_profile(&parent->base.profiles, ent->new); - } else + } else { + /* aafs interface uses replacedby */ + rcu_assign_pointer(ent->new->replacedby->profile, + aa_get_profile(ent->new)); __list_add_profile(&ns->base.profiles, ent->new); - + } aa_load_ent_free(ent); } mutex_unlock(&ns->lock); diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c index 6c9390179b89..b125acc9aa26 100644 --- a/security/apparmor/procattr.c +++ b/security/apparmor/procattr.c @@ -37,7 +37,7 @@ int aa_getprocattr(struct aa_profile *profile, char **string) { char *str; int len = 0, mode_len = 0, ns_len = 0, name_len; - const char *mode_str = profile_mode_names[profile->mode]; + const char *mode_str = aa_profile_mode_names[profile->mode]; const char *ns_name = NULL; struct aa_namespace *ns = profile->ns; struct aa_namespace *current_ns = __aa_current_profile()->ns; -- cgit v1.2.3 From 4cd4fc77032dca46fe7475d81461e29145db247a Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 29 Sep 2013 08:39:22 -0700 Subject: apparmor: fix suspicious RCU usage warning in policy.c/policy.h The recent 3.12 pull request for apparmor was missing a couple rcu _protected access modifiers. Resulting in the follow suspicious RCU usage [ 29.804534] [ INFO: suspicious RCU usage. ] [ 29.804539] 3.11.0+ #5 Not tainted [ 29.804541] ------------------------------- [ 29.804545] security/apparmor/include/policy.h:363 suspicious rcu_dereference_check() usage! [ 29.804548] [ 29.804548] other info that might help us debug this: [ 29.804548] [ 29.804553] [ 29.804553] rcu_scheduler_active = 1, debug_locks = 1 [ 29.804558] 2 locks held by apparmor_parser/1268: [ 29.804560] #0: (sb_writers#9){.+.+.+}, at: [] file_start_write+0x27/0x29 [ 29.804576] #1: (&ns->lock){+.+.+.}, at: [] aa_replace_profiles+0x166/0x57c [ 29.804589] [ 29.804589] stack backtrace: [ 29.804595] CPU: 0 PID: 1268 Comm: apparmor_parser Not tainted 3.11.0+ #5 [ 29.804599] Hardware name: ASUSTeK Computer Inc. UL50VT /UL50VT , BIOS 217 03/01/2010 [ 29.804602] 0000000000000000 ffff8800b95a1d90 ffffffff8144eb9b ffff8800b94db540 [ 29.804611] ffff8800b95a1dc0 ffffffff81087439 ffff880138cc3a18 ffff880138cc3a18 [ 29.804619] ffff8800b9464a90 ffff880138cc3a38 ffff8800b95a1df0 ffffffff811f5084 [ 29.804628] Call Trace: [ 29.804636] [] dump_stack+0x4e/0x82 [ 29.804642] [] lockdep_rcu_suspicious+0xfc/0x105 [ 29.804649] [] __aa_update_replacedby+0x53/0x7f [ 29.804655] [] __replace_profile+0x11f/0x1ed [ 29.804661] [] aa_replace_profiles+0x410/0x57c [ 29.804668] [] profile_replace+0x35/0x4c [ 29.804674] [] vfs_write+0xad/0x113 [ 29.804680] [] SyS_write+0x44/0x7a [ 29.804687] [] system_call_fastpath+0x16/0x1b [ 29.804691] [ 29.804694] =============================== [ 29.804697] [ INFO: suspicious RCU usage. ] [ 29.804700] 3.11.0+ #5 Not tainted [ 29.804703] ------------------------------- [ 29.804706] security/apparmor/policy.c:566 suspicious rcu_dereference_check() usage! [ 29.804709] [ 29.804709] other info that might help us debug this: [ 29.804709] [ 29.804714] [ 29.804714] rcu_scheduler_active = 1, debug_locks = 1 [ 29.804718] 2 locks held by apparmor_parser/1268: [ 29.804721] #0: (sb_writers#9){.+.+.+}, at: [] file_start_write+0x27/0x29 [ 29.804733] #1: (&ns->lock){+.+.+.}, at: [] aa_replace_profiles+0x166/0x57c [ 29.804744] [ 29.804744] stack backtrace: [ 29.804750] CPU: 0 PID: 1268 Comm: apparmor_parser Not tainted 3.11.0+ #5 [ 29.804753] Hardware name: ASUSTeK Computer Inc. UL50VT /UL50VT , BIOS 217 03/01/2010 [ 29.804756] 0000000000000000 ffff8800b95a1d80 ffffffff8144eb9b ffff8800b94db540 [ 29.804764] ffff8800b95a1db0 ffffffff81087439 ffff8800b95b02b0 0000000000000000 [ 29.804772] ffff8800b9efba08 ffff880138cc3a38 ffff8800b95a1dd0 ffffffff811f4f94 [ 29.804779] Call Trace: [ 29.804786] [] dump_stack+0x4e/0x82 [ 29.804791] [] lockdep_rcu_suspicious+0xfc/0x105 [ 29.804798] [] aa_free_replacedby_kref+0x4d/0x62 [ 29.804804] [] ? aa_put_namespace+0x17/0x17 [ 29.804810] [] kref_put+0x36/0x40 [ 29.804816] [] __replace_profile+0x13a/0x1ed [ 29.804822] [] aa_replace_profiles+0x410/0x57c [ 29.804829] [] profile_replace+0x35/0x4c [ 29.804835] [] vfs_write+0xad/0x113 [ 29.804840] [] SyS_write+0x44/0x7a [ 29.804847] [] system_call_fastpath+0x16/0x1b Reported-by: miles.lane@gmail.com CC: paulmck@linux.vnet.ibm.com Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/include/policy.h | 4 +++- security/apparmor/policy.c | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index f2d4b6348cbc..c28b0f20ab53 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -360,7 +360,9 @@ static inline void aa_put_replacedby(struct aa_replacedby *p) static inline void __aa_update_replacedby(struct aa_profile *orig, struct aa_profile *new) { - struct aa_profile *tmp = rcu_dereference(orig->replacedby->profile); + struct aa_profile *tmp; + tmp = rcu_dereference_protected(orig->replacedby->profile, + mutex_is_locked(&orig->ns->lock)); rcu_assign_pointer(orig->replacedby->profile, aa_get_profile(new)); orig->flags |= PFLAG_INVALID; aa_put_profile(tmp); diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 6172509fa2b7..345bec07a27d 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -563,7 +563,8 @@ void __init aa_free_root_ns(void) static void free_replacedby(struct aa_replacedby *r) { if (r) { - aa_put_profile(rcu_dereference(r->profile)); + /* r->profile will not be updated any more as r is dead */ + aa_put_profile(rcu_dereference_protected(r->profile, true)); kzfree(r); } } -- cgit v1.2.3 From 5cb3e91ebd0405519795f243adbfc4ed2a6fe53f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 14 Oct 2013 11:44:34 -0700 Subject: apparmor: fix memleak of the profile hash BugLink: http://bugs.launchpad.net/bugs/1235523 This fixes the following kmemleak trace: unreferenced object 0xffff8801e8c35680 (size 32): comm "apparmor_parser", pid 691, jiffies 4294895667 (age 13230.876s) hex dump (first 32 bytes): e0 d3 4e b5 ac 6d f4 ed 3f cb ee 48 1c fd 40 cf ..N..m..?..H..@. 5b cc e9 93 00 00 00 00 00 00 00 00 00 00 00 00 [............... backtrace: [] kmemleak_alloc+0x4e/0xb0 [] __kmalloc+0x103/0x290 [] aa_calc_profile_hash+0x6c/0x150 [] aa_unpack+0x39d/0xd50 [] aa_replace_profiles+0x3d/0xd80 [] profile_replace+0x37/0x50 [] vfs_write+0xbd/0x1e0 [] SyS_write+0x4c/0xa0 [] system_call_fastpath+0x1a/0x1f [] 0xffffffffffffffff Signed-off-by: John Johansen Signed-off-by: James Morris --- security/apparmor/policy.c | 1 + 1 file changed, 1 insertion(+) (limited to 'security/apparmor/policy.c') diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 345bec07a27d..705c2879d3a9 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -610,6 +610,7 @@ void aa_free_profile(struct aa_profile *profile) aa_put_dfa(profile->policy.dfa); aa_put_replacedby(profile->replacedby); + kzfree(profile->hash); kzfree(profile); } -- cgit v1.2.3