Messages in this thread | | | Date | Tue, 07 Dec 2010 13:29:26 -0800 | From | John Johansen <> | Subject | Re: [PATCH v2] APPARMOR: add sid to profile mapping and sid recycling |
| |
On 11/30/2010 01:56 AM, wzt.wzt@gmail.com wrote: > Add sid to profile mapping and sid recycling. > > A security identifier table (sidtab) is a hash table of aa_profile > structures indexed by sid value. > > sid_bitmap is a bitmap array for sid allocing and recycling. > > alloc_sid: find the first zero bit in the sid_bitmap array. > free_sid: clear the bit in the sid_bitmap array. > > v2: > - fix some bugs pointed by john.johansen. > - use list_for_each_entry_* to clean up the code. > - combine the sid alloc and profile addition, and it called from > aa_alloc_profile and aa_alloc_null_profile. > > Signed-off-by: Zhitong Wang <zhitong.wangzt@alibaba-inc.com> > > --- > security/apparmor/include/policy.h | 2 + > security/apparmor/include/sid.h | 28 +++++- > security/apparmor/lsm.c | 11 ++ > security/apparmor/policy.c | 77 +++++++++++--- > security/apparmor/sid.c | 211 ++++++++++++++++++++++++++++++++---- > 5 files changed, 292 insertions(+), 37 deletions(-) > > diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h > index aeda5cf..14ebd7f 100644 > --- a/security/apparmor/include/policy.h > +++ b/security/apparmor/include/policy.h > @@ -30,6 +30,8 @@ > #include "resource.h" > > extern const char *profile_mode_names[]; > +extern struct aa_sidtab *aa_sid_hash_table; > + why here instead of in include/sid.h ?
> #define APPARMOR_NAMES_MAX_INDEX 3 > > #define COMPLAIN_MODE(_profile) \ > diff --git a/security/apparmor/include/sid.h b/security/apparmor/include/sid.h > index 020db35..73c4bd6 100644 > --- a/security/apparmor/include/sid.h > +++ b/security/apparmor/include/sid.h > @@ -16,9 +16,33 @@ > > #include <linux/types.h> > > +#define AA_SIDTAB_HASH_BITS 7 > +#define AA_SIDTAB_HASH_BUCKETS (1 << AA_SIDTAB_HASH_BITS) > +#define AA_SIDTAB_HASH_MASK (AA_SIDTAB_HASH_BUCKETS - 1) > + > +#define AA_SIDTAB_SIZE AA_SIDTAB_HASH_BUCKETS > +#define AA_SIDTAB_HASH(sid) (sid & AA_SIDTAB_HASH_MASK) > + > +#define AA_SID_BITMAP_SIZE 1024 > + > struct aa_profile; > > -u32 aa_alloc_sid(void); > -void aa_free_sid(u32 sid); > +struct aa_sidtab_node { > + u32 sid; > + struct aa_profile *profile; > + struct list_head list; > +}; > + > +struct aa_sidtab { > + struct list_head sid_list[AA_SIDTAB_SIZE]; > + spinlock_t lock; > +}; > + > +u32 aa_alloc_sid(struct aa_profile *new_profile); > +void aa_sidtab_init(struct aa_sidtab *sid_hash_table); > +void init_sid_bitmap(void); > +int replace_profile_by_sid(u32 sid, struct aa_profile *new_profile); > +void free_sidtab(struct aa_sidtab *sid_hash_table); > +int aa_free_sid(u32 sid); > > #endif /* __AA_SID_H */ > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > index b7106f1..1758e8e 100644 > --- a/security/apparmor/lsm.c > +++ b/security/apparmor/lsm.c > @@ -34,6 +34,7 @@ > #include "include/path.h" > #include "include/policy.h" > #include "include/procattr.h" > +#include "include/sid.h" > > /* Flag indicating whether initialization completed */ > int apparmor_initialized __initdata; > @@ -907,6 +908,15 @@ static int __init apparmor_init(void) > return 0; > } > > + aa_sid_hash_table = kmalloc(sizeof(struct aa_sidtab), GFP_KERNEL); > + if (!aa_sid_hash_table) { > + AA_ERROR("Unable to allocate sid hash table\n"); > + return -ENOMEM; > + } > + > + aa_sidtab_init(aa_sid_hash_table); > + init_sid_bitmap(); > +
Hrmm, nothing wrong here, I have just preferred keeping the details of a units init with in the unit. So in this case I would create a new fn in sid.c and just call the fn from here.
Of course that is just my preference, its not a requirement.
> error = aa_alloc_root_ns(); > if (error) { > AA_ERROR("Unable to allocate default profile namespace\n"); > @@ -943,6 +953,7 @@ register_security_out: > aa_free_root_ns(); > > alloc_out: > + kfree(aa_sid_hash_table); > aa_destroy_aafs(); > > apparmor_enabled = 0; > diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c > index 4f0eade..01a3025 100644 > --- a/security/apparmor/policy.c > +++ b/security/apparmor/policy.c > @@ -292,7 +292,6 @@ static struct aa_namespace *alloc_namespace(const char *prefix, > if (!ns->unconfined) > goto fail_unconfined; > > - ns->unconfined->sid = aa_alloc_sid(); > ns->unconfined->flags = PFLAG_UNCONFINED | PFLAG_IX_ON_NAME_ERROR | > PFLAG_IMMUTABLE; > > @@ -510,6 +509,8 @@ static void __replace_profile(struct aa_profile *old, struct aa_profile *new) > /* released by free_profile */ > old->replacedby = aa_get_profile(new); > __list_remove_profile(old); > + > + replace_profile_by_sid(old->sid, new); > } > > static void __profile_list_release(struct list_head *head); > @@ -644,6 +645,7 @@ void __init aa_free_root_ns(void) > struct aa_profile *aa_alloc_profile(const char *hname) > { > struct aa_profile *profile; > + u32 sid; > > /* freed by free_profile - usually through aa_put_profile */ > profile = kzalloc(sizeof(*profile), GFP_KERNEL); > @@ -655,6 +657,59 @@ struct aa_profile *aa_alloc_profile(const char *hname) > return NULL; > } > > + sid = aa_alloc_sid(profile); > + if (sid == -1) { > + kfree(profile->base.hname); > + kzfree(profile); > + return NULL; > + } > + profile->sid = sid; > + > + return profile; > +} > + > + > +/** > + * aa_alloc_null_profile - allocate, initialize and return a new > + * null-X learning profile > + * > + * Returns: refcount profile or NULL on failure > + */ > +struct aa_profile *aa_alloc_null_profile(struct aa_profile *parent) > +{ > + struct aa_profile *profile; > + char *name; > + u32 sid; > + > + profile = kzalloc(sizeof(*profile), GFP_KERNEL); > + if (!profile) > + return NULL; > + > + sid = aa_alloc_sid(profile); > + if (sid == -1) { > + kfree(profile); > + return NULL; > + } > + > + /* freed below */ > + name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, GFP_KERNEL); > + if (!name) { > + kfree(profile); > + aa_free_sid(sid); > + return NULL; > + } > + > + sprintf(name, "%s//null-%x", parent->base.hname, sid); > + > + if (!policy_init(&profile->base, NULL, name)) { > + aa_free_sid(sid); > + kzfree(profile); > + return NULL; > + } > + > + profile->sid = sid; > + kfree(name); > +
Hrmm, no. I really can't see the justification here for recreating a special case of aa_alloc_profile. Yes allocating a name just to free it again is a pita but as more features get added its going to be more and more important to have a single init point for the profile.
perhaps a second patch passing and extra parameter to aa_alloc_profile indicating that the name can be directly assigned instead of duplicated
> /* refcount released by caller */ > return profile; > } > @@ -676,21 +731,11 @@ struct aa_profile *aa_alloc_profile(const char *hname) > struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat) > { > struct aa_profile *profile = NULL; > - char *name; > - u32 sid = aa_alloc_sid(); > - > - /* freed below */ > - name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, GFP_KERNEL); > - if (!name) > - goto fail; > - sprintf(name, "%s//null-%x", parent->base.hname, sid); > > - profile = aa_alloc_profile(name); > - kfree(name); > + profile = aa_alloc_null_profile(parent); > if (!profile) > goto fail; > > - profile->sid = sid; > profile->mode = APPARMOR_COMPLAIN; > profile->flags = PFLAG_NULL; > if (hat) > @@ -708,7 +753,6 @@ struct aa_profile *aa_new_null_profile(struct aa_profile *parent, int hat) > return profile; > > fail: > - aa_free_sid(sid); > return NULL; > } > > @@ -727,7 +771,7 @@ static void free_profile(struct aa_profile *profile) > AA_DEBUG("%s(%p)\n", __func__, profile); > > if (!profile) > - return; > + return ; > extra trailing space
> if (!list_empty(&profile->base.list)) { > AA_ERROR("%s: internal error, " > @@ -736,6 +780,9 @@ static void free_profile(struct aa_profile *profile) > BUG(); > } > > + if (aa_free_sid(profile->sid) == -1) > + return ; > + Hrmmm, as far as I can see this should never happen. If the profile is successfully created it has a sid allocated. So something is very wrong if this condition happens
In this case I would do BUG_ON(aa_free_sid(profile->sid) == -1);
> /* free children profiles */ > policy_destroy(&profile->base); > aa_put_profile(profile->parent); > @@ -747,7 +794,6 @@ static void free_profile(struct aa_profile *profile) > aa_free_cap_rules(&profile->caps); > aa_free_rlimit_rules(&profile->rlimits); > > - aa_free_sid(profile->sid); > aa_put_dfa(profile->xmatch); > > aa_put_profile(profile->replacedby); > @@ -945,7 +991,6 @@ static void __add_new_profile(struct aa_namespace *ns, struct aa_policy *policy, > profile->parent = aa_get_profile((struct aa_profile *) policy); > __list_add_profile(&policy->profiles, profile); > /* released on free_profile */ > - profile->sid = aa_alloc_sid(); > profile->ns = aa_get_namespace(ns); > } > > diff --git a/security/apparmor/sid.c b/security/apparmor/sid.c > index f0b34f7..4a4fcd4 100644 > --- a/security/apparmor/sid.c > +++ b/security/apparmor/sid.c > @@ -14,42 +14,215 @@ > * AppArmor allocates a unique sid for every profile loaded. If a profile > * is replaced it receives the sid of the profile it is replacing. > * > - * The sid value of 0 is invalid. > + * The sid value of 0 and -1 is invalid. > + * hrmm are two invalid values required. I don't really care which is used as an invalid value but I can't see a reason to have two values in the current code.
Also an invalid value of -1 when the type is u32 makes me twitch. Yes I know it can be done, but it would be cleaner to setup a define
#define invalid_sid ((u32) -1)
or change the sids type.
> + * A security identifier table (sidtab) is a hash table of aa_profile > + * structures indexed by sid value. > */ > > +#include <linux/slab.h> > #include <linux/spinlock.h> > +#include <linux/list.h> > #include <linux/errno.h> > -#include <linux/err.h> > > #include "include/sid.h" > > -/* global counter from which sids are allocated */ > -static u32 global_sid; > -static DEFINE_SPINLOCK(sid_lock); > +struct aa_sidtab *aa_sid_hash_table; > +u32 sid_bitmap[AA_SID_BITMAP_SIZE] = {0}; > > -/* TODO FIXME: add sid to profile mapping, and sid recycling */ > +/** > + * set_sid_bitmap - set bitmap with sid in the sid_bitmap array > + * @sid: sid to set in the sid_bitmap array > + */ > +static void set_sid_bitmap(u32 sid) > +{ > + sid_bitmap[sid / 32] |= (1 << (sid % 32)); > +} > > /** > - * aa_alloc_sid - allocate a new sid for a profile > + * clear_sid_bitmap - clear bitmap with sid in the sid_bitmap array > + * @sid: sid to clear in the sid_bitmap array > */ > -u32 aa_alloc_sid(void) > +static void clear_sid_bitmap(u32 sid) > { > - u32 sid; > + sid_bitmap[sid / 32] ^= (1 << (sid % 32)); > +} > + > +/** > + * alloc_sid - allocte a unique sid in the sid_bitmap array and add > + * new profile to sid hash table > + * > + * Returns: success return sid, failed return -1 > + */ > +u32 aa_alloc_sid(struct aa_profile *new_profile) > +{ > + struct aa_sidtab_node *node = NULL; > + int hash_value; > + u32 sid = 0; > + u32 i, j; > + > + node = kmalloc(sizeof(struct aa_sidtab_node), GFP_KERNEL); > + if (!node) > + return -1; > + > + node->profile = new_profile; > + INIT_LIST_HEAD(&node->list); > + > + /* find the first zero bit in the sid_bitmap array */ > + spin_lock(&aa_sid_hash_table->lock); > + for (i = 0; i < AA_SID_BITMAP_SIZE; i++) { > + for (j = 0; j < 32; j++) { > + if (!(sid_bitmap[i] & (1 << j))) { > + /* convert offset to sid */ > + sid = i * 32 + j; > + goto alloc_ok; > + } > + } > + } > + spin_unlock(&aa_sid_hash_table->lock); > + kfree(node); > + > + return -1; > + > +alloc_ok: > + node->sid = sid; > + hash_value = AA_SIDTAB_HASH(sid); > + list_add_tail(&(node->list), &aa_sid_hash_table->sid_list[hash_value]); > + set_sid_bitmap(sid); > + spin_unlock(&aa_sid_hash_table->lock); > > - /* > - * TODO FIXME: sid recycling - part of profile mapping table > - */ > - spin_lock(&sid_lock); > - sid = (++global_sid); > - spin_unlock(&sid_lock); > return sid; > } > > /** > - * aa_free_sid - free a sid > - * @sid: sid to free > + * aa_sidtab_init - init sid hash table > + * @sid_hash_table: sid hash table to be created > + * > + */ > +void aa_sidtab_init(struct aa_sidtab *sid_hash_table) > +{ > + int i; > + > + for (i = 0; i < AA_SIDTAB_SIZE; i++) > + INIT_LIST_HEAD(&sid_hash_table->sid_list[i]); > + > + spin_lock_init(&sid_hash_table->lock); > +} > + can be marked as __init
> +/** > + * init_sid_bitmap - init sid_bitmap array > + */ > +void init_sid_bitmap(void) > +{ > + /* The sid value of 0 is invalid */ > + sid_bitmap[0] = 1; > +} > + can be marked as __init
> +/** > + * free_sidtab_list - free memory of a aa_profile list > + * @list_head: list to be freed > + * > + * Requires: correct locks for the @list_head be held > + * > + */ > +static void free_sidtab_list(struct list_head *list_head) > +{ > + struct aa_sidtab_node *p = NULL; > + struct list_head *s = NULL, *q = NULL; > + > + list_for_each_safe(s, q, list_head) { > + p = list_entry(s, struct aa_sidtab_node, list); > + if (p) { > + list_del(s); > + kfree(p); > + } > + } > +} > + > +/** > + * free_sidtab - free memory of sid hash table > + * @sid_hash_table: sid hash table to be freed > */ > -void aa_free_sid(u32 sid) > +void free_sidtab(struct aa_sidtab *sid_hash_table) > { > - ; /* NOP ATM */ > + int i; > + > + spin_lock(&sid_hash_table->lock); > + for (i = 0; i < AA_SIDTAB_SIZE; i++) { > + if (list_empty(&sid_hash_table->sid_list[i])) > + continue; > + free_sidtab_list(&sid_hash_table->sid_list[i]); > + } > + spin_unlock(&sid_hash_table->lock); > + > + kfree(sid_hash_table); > +} > + having the ability to tear down and free memory is nice but are we ever going to use it? Once the module is inited we never actually never shutdown/cleanup the global structures.
> +/** > + * search_profile_by_sid - search a profile by sid > + * @sid: sid to be searched > + * > + * Requires: correct locks for the @list_head be held > + * Returns: success a profile struct, failed NULL > + */ > +static struct aa_sidtab_node *search_profile_by_sid(u32 sid) > +{ > + struct aa_sidtab_node *s = NULL; > + int hash_value = AA_SIDTAB_HASH(sid); > + > + list_for_each_entry(s, &aa_sid_hash_table->sid_list[hash_value], list) { > + if (s && s->sid == sid) > + return s; > + } > + > + return NULL; > +} > + > +/** > + * replace_profile_by_sid - replace a profile by sid in the sid hash table > + * @sid: sid to be deleted > + * @new_profile: ...
> + * Returns: sccesss 0, failed -1 how about returning -ENOENT? > + */ > +int replace_profile_by_sid(u32 sid, struct aa_profile *new_profile) > +{ > + struct aa_sidtab_node *node; > + > + spin_lock(&aa_sid_hash_table->lock); > + node = search_profile_by_sid(sid); > + if (!node) { > + spin_unlock(&aa_sid_hash_table->lock); > + return -1; > + } > + > + node->profile = new_profile; hrmm, I think I'm okay with this, I need to make another pass to make sure that this is cosher with profile references and life cycles.
> + spin_unlock(&aa_sid_hash_table->lock); > + > + return 0; > +} > + > +/** > + * aa_free_sid - delete a profile by sid in the sid hash table > + * @sid: sid to be deleted > + * > + * Returns: sccesss 0, failed -1 > + */ > +int aa_free_sid(u32 sid) > +{ > + struct aa_sidtab_node *node; > + > + spin_lock(&aa_sid_hash_table->lock); > + node = search_profile_by_sid(sid); > + if (!node) { > + spin_unlock(&aa_sid_hash_table->lock); > + return -1; > + } > + > + list_del(&(node->list)); > + clear_sid_bitmap(sid); > + spin_unlock(&aa_sid_hash_table->lock); > + > + kfree(node); > + > + return 0; > }
A few more general points (nothing that needs to be attended to immediately but could be done in the future). - at some point in the future sids will have to be able to map to either a single profile or a list of profiles. I don't think there is anything to do now, but it is something to keep in mind. - It would be nice if the sid table could be more dynamic. Maybe be allocated in chunks instead of all at once - it might be worth keeping a small (size limited) queue of the most recently freed sids, so that you can skip the free, alloc, search, when some sids have been freed recently
thanks Zhitong
| |