From 0eb2f2962489edb6943fe8a853de024c53aa91b2 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 31 Jul 2019 19:29:14 -0400 Subject: selinux: shuffle around policydb.c to get rid of forward declarations No code changes, but move a lot of the policydb destructors higher up so we can get rid of a forward declaration. This patch does expose a few old checkpatch.pl errors, but those will be dealt with in a separate (set of) patches. Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 376 ++++++++++++++++++++--------------------- 1 file changed, 187 insertions(+), 189 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 38d0083204f1..d5effce86304 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -178,6 +178,193 @@ static struct policydb_compat_info *policydb_lookup_compat(int version) return info; } +/* + * The following *_destroy functions are used to + * free any memory allocated for each kind of + * symbol data in the policy database. + */ + +static int perm_destroy(void *key, void *datum, void *p) +{ + kfree(key); + kfree(datum); + return 0; +} + +static int common_destroy(void *key, void *datum, void *p) +{ + struct common_datum *comdatum; + + kfree(key); + if (datum) { + comdatum = datum; + hashtab_map(comdatum->permissions.table, perm_destroy, NULL); + hashtab_destroy(comdatum->permissions.table); + } + kfree(datum); + return 0; +} + +static void constraint_expr_destroy(struct constraint_expr *expr) +{ + if (expr) { + ebitmap_destroy(&expr->names); + if (expr->type_names) { + ebitmap_destroy(&expr->type_names->types); + ebitmap_destroy(&expr->type_names->negset); + kfree(expr->type_names); + } + kfree(expr); + } +} + +static int cls_destroy(void *key, void *datum, void *p) +{ + struct class_datum *cladatum; + struct constraint_node *constraint, *ctemp; + struct constraint_expr *e, *etmp; + + kfree(key); + if (datum) { + cladatum = datum; + hashtab_map(cladatum->permissions.table, perm_destroy, NULL); + hashtab_destroy(cladatum->permissions.table); + constraint = cladatum->constraints; + while (constraint) { + e = constraint->expr; + while (e) { + etmp = e; + e = e->next; + constraint_expr_destroy(etmp); + } + ctemp = constraint; + constraint = constraint->next; + kfree(ctemp); + } + + constraint = cladatum->validatetrans; + while (constraint) { + e = constraint->expr; + while (e) { + etmp = e; + e = e->next; + constraint_expr_destroy(etmp); + } + ctemp = constraint; + constraint = constraint->next; + kfree(ctemp); + } + kfree(cladatum->comkey); + } + kfree(datum); + return 0; +} + +static int role_destroy(void *key, void *datum, void *p) +{ + struct role_datum *role; + + kfree(key); + if (datum) { + role = datum; + ebitmap_destroy(&role->dominates); + ebitmap_destroy(&role->types); + } + kfree(datum); + return 0; +} + +static int type_destroy(void *key, void *datum, void *p) +{ + kfree(key); + kfree(datum); + return 0; +} + +static int user_destroy(void *key, void *datum, void *p) +{ + struct user_datum *usrdatum; + + kfree(key); + if (datum) { + usrdatum = datum; + ebitmap_destroy(&usrdatum->roles); + ebitmap_destroy(&usrdatum->range.level[0].cat); + ebitmap_destroy(&usrdatum->range.level[1].cat); + ebitmap_destroy(&usrdatum->dfltlevel.cat); + } + kfree(datum); + return 0; +} + +static int sens_destroy(void *key, void *datum, void *p) +{ + struct level_datum *levdatum; + + kfree(key); + if (datum) { + levdatum = datum; + if (levdatum->level) + ebitmap_destroy(&levdatum->level->cat); + kfree(levdatum->level); + } + kfree(datum); + return 0; +} + +static int cat_destroy(void *key, void *datum, void *p) +{ + kfree(key); + kfree(datum); + return 0; +} + +static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = +{ + common_destroy, + cls_destroy, + role_destroy, + type_destroy, + user_destroy, + cond_destroy_bool, + sens_destroy, + cat_destroy, +}; + +static int filenametr_destroy(void *key, void *datum, void *p) +{ + struct filename_trans *ft = key; + kfree(ft->name); + kfree(key); + kfree(datum); + cond_resched(); + return 0; +} + +static int range_tr_destroy(void *key, void *datum, void *p) +{ + struct mls_range *rt = datum; + kfree(key); + ebitmap_destroy(&rt->level[0].cat); + ebitmap_destroy(&rt->level[1].cat); + kfree(datum); + cond_resched(); + return 0; +} + +static void ocontext_destroy(struct ocontext *c, int i) +{ + if (!c) + return; + + context_destroy(&c->context[0]); + context_destroy(&c->context[1]); + if (i == OCON_ISID || i == OCON_FS || + i == OCON_NETIF || i == OCON_FSUSE) + kfree(c->u.name); + kfree(c); +} + /* * Initialize the role table. */ @@ -274,8 +461,6 @@ static int rangetr_cmp(struct hashtab *h, const void *k1, const void *k2) return v; } -static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap); - /* * Initialize a policy database structure. */ @@ -569,193 +754,6 @@ out: return rc; } -/* - * The following *_destroy functions are used to - * free any memory allocated for each kind of - * symbol data in the policy database. - */ - -static int perm_destroy(void *key, void *datum, void *p) -{ - kfree(key); - kfree(datum); - return 0; -} - -static int common_destroy(void *key, void *datum, void *p) -{ - struct common_datum *comdatum; - - kfree(key); - if (datum) { - comdatum = datum; - hashtab_map(comdatum->permissions.table, perm_destroy, NULL); - hashtab_destroy(comdatum->permissions.table); - } - kfree(datum); - return 0; -} - -static void constraint_expr_destroy(struct constraint_expr *expr) -{ - if (expr) { - ebitmap_destroy(&expr->names); - if (expr->type_names) { - ebitmap_destroy(&expr->type_names->types); - ebitmap_destroy(&expr->type_names->negset); - kfree(expr->type_names); - } - kfree(expr); - } -} - -static int cls_destroy(void *key, void *datum, void *p) -{ - struct class_datum *cladatum; - struct constraint_node *constraint, *ctemp; - struct constraint_expr *e, *etmp; - - kfree(key); - if (datum) { - cladatum = datum; - hashtab_map(cladatum->permissions.table, perm_destroy, NULL); - hashtab_destroy(cladatum->permissions.table); - constraint = cladatum->constraints; - while (constraint) { - e = constraint->expr; - while (e) { - etmp = e; - e = e->next; - constraint_expr_destroy(etmp); - } - ctemp = constraint; - constraint = constraint->next; - kfree(ctemp); - } - - constraint = cladatum->validatetrans; - while (constraint) { - e = constraint->expr; - while (e) { - etmp = e; - e = e->next; - constraint_expr_destroy(etmp); - } - ctemp = constraint; - constraint = constraint->next; - kfree(ctemp); - } - kfree(cladatum->comkey); - } - kfree(datum); - return 0; -} - -static int role_destroy(void *key, void *datum, void *p) -{ - struct role_datum *role; - - kfree(key); - if (datum) { - role = datum; - ebitmap_destroy(&role->dominates); - ebitmap_destroy(&role->types); - } - kfree(datum); - return 0; -} - -static int type_destroy(void *key, void *datum, void *p) -{ - kfree(key); - kfree(datum); - return 0; -} - -static int user_destroy(void *key, void *datum, void *p) -{ - struct user_datum *usrdatum; - - kfree(key); - if (datum) { - usrdatum = datum; - ebitmap_destroy(&usrdatum->roles); - ebitmap_destroy(&usrdatum->range.level[0].cat); - ebitmap_destroy(&usrdatum->range.level[1].cat); - ebitmap_destroy(&usrdatum->dfltlevel.cat); - } - kfree(datum); - return 0; -} - -static int sens_destroy(void *key, void *datum, void *p) -{ - struct level_datum *levdatum; - - kfree(key); - if (datum) { - levdatum = datum; - if (levdatum->level) - ebitmap_destroy(&levdatum->level->cat); - kfree(levdatum->level); - } - kfree(datum); - return 0; -} - -static int cat_destroy(void *key, void *datum, void *p) -{ - kfree(key); - kfree(datum); - return 0; -} - -static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = -{ - common_destroy, - cls_destroy, - role_destroy, - type_destroy, - user_destroy, - cond_destroy_bool, - sens_destroy, - cat_destroy, -}; - -static int filenametr_destroy(void *key, void *datum, void *p) -{ - struct filename_trans *ft = key; - kfree(ft->name); - kfree(key); - kfree(datum); - cond_resched(); - return 0; -} - -static int range_tr_destroy(void *key, void *datum, void *p) -{ - struct mls_range *rt = datum; - kfree(key); - ebitmap_destroy(&rt->level[0].cat); - ebitmap_destroy(&rt->level[1].cat); - kfree(datum); - cond_resched(); - return 0; -} - -static void ocontext_destroy(struct ocontext *c, int i) -{ - if (!c) - return; - - context_destroy(&c->context[0]); - context_destroy(&c->context[1]); - if (i == OCON_ISID || i == OCON_FS || - i == OCON_NETIF || i == OCON_FSUSE) - kfree(c->u.name); - kfree(c); -} - /* * Free any memory allocated by a policy database structure. */ -- cgit v1.2.3 From 2492acaf1e53a13a528e31769aa3df15b4d9f6ef Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 29 Jul 2019 10:41:16 +0200 Subject: selinux: policydb - fix some checkpatch.pl warnings Fix most of the code style warnings discovered when moving code around. Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index d5effce86304..943c39b07bbb 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -334,6 +334,7 @@ static int (*destroy_f[SYM_NUM]) (void *key, void *datum, void *datap) = static int filenametr_destroy(void *key, void *datum, void *p) { struct filename_trans *ft = key; + kfree(ft->name); kfree(key); kfree(datum); @@ -344,6 +345,7 @@ static int filenametr_destroy(void *key, void *datum, void *p) static int range_tr_destroy(void *key, void *datum, void *p) { struct mls_range *rt = datum; + kfree(key); ebitmap_destroy(&rt->level[0].cat); ebitmap_destroy(&rt->level[1].cat); @@ -439,6 +441,7 @@ static int filenametr_cmp(struct hashtab *h, const void *k1, const void *k2) static u32 rangetr_hash(struct hashtab *h, const void *k) { const struct range_trans *key = k; + return (key->source_type + (key->target_type << 3) + (key->target_class << 5)) & (h->size - 1); } @@ -488,7 +491,8 @@ static int policydb_init(struct policydb *p) if (rc) goto out; - p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, (1 << 10)); + p->filename_trans = hashtab_create(filenametr_hash, filenametr_cmp, + (1 << 10)); if (!p->filename_trans) { rc = -ENOMEM; goto out; @@ -664,9 +668,9 @@ static void hash_eval(struct hashtab *h, const char *hash_name) struct hashtab_info info; hashtab_stat(h, &info); - pr_debug("SELinux: %s: %d entries and %d/%d buckets used, " - "longest chain length %d\n", hash_name, h->nel, - info.slots_used, h->size, info.max_chain_len); + pr_debug("SELinux: %s: %d entries and %d/%d buckets used, longest chain length %d\n", + hash_name, h->nel, info.slots_used, h->size, + info.max_chain_len); } static void symtab_hash_eval(struct symtab *s) -- cgit v1.2.3 From f07ea1d4eda2574c6b0f99576db61c86ec27ff5b Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Mon, 29 Jul 2019 10:41:17 +0200 Subject: selinux: policydb - rename type_val_to_struct_array The name is overly long and inconsistent with the other *_val_to_struct members. Dropping the "_array" prefix makes the code easier to read and gets rid of one line over 80 characters warning. Signed-off-by: Ondrej Mosnacek Signed-off-by: Paul Moore --- security/selinux/ss/policydb.c | 14 +++++++------- security/selinux/ss/policydb.h | 2 +- security/selinux/ss/services.c | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 943c39b07bbb..a9dabe563ea6 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -590,7 +590,7 @@ static int type_index(void *key, void *datum, void *datap) || typdatum->bounds > p->p_types.nprim) return -EINVAL; p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key; - p->type_val_to_struct_array[typdatum->value - 1] = typdatum; + p->type_val_to_struct[typdatum->value - 1] = typdatum; } return 0; @@ -732,10 +732,10 @@ static int policydb_index(struct policydb *p) if (!p->user_val_to_struct) return -ENOMEM; - p->type_val_to_struct_array = kvcalloc(p->p_types.nprim, - sizeof(*p->type_val_to_struct_array), - GFP_KERNEL); - if (!p->type_val_to_struct_array) + p->type_val_to_struct = kvcalloc(p->p_types.nprim, + sizeof(*p->type_val_to_struct), + GFP_KERNEL); + if (!p->type_val_to_struct) return -ENOMEM; rc = cond_init_bool_indexes(p); @@ -781,7 +781,7 @@ void policydb_destroy(struct policydb *p) kfree(p->class_val_to_struct); kfree(p->role_val_to_struct); kfree(p->user_val_to_struct); - kvfree(p->type_val_to_struct_array); + kvfree(p->type_val_to_struct); avtab_destroy(&p->te_avtab); @@ -1726,7 +1726,7 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap) return -EINVAL; } - upper = p->type_val_to_struct_array[upper->bounds - 1]; + upper = p->type_val_to_struct[upper->bounds - 1]; BUG_ON(!upper); if (upper->attribute) { diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 27039149ff0a..05fc672831aa 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -255,7 +255,7 @@ struct policydb { struct class_datum **class_val_to_struct; struct role_datum **role_val_to_struct; struct user_datum **user_val_to_struct; - struct type_datum **type_val_to_struct_array; + struct type_datum **type_val_to_struct; /* type enforcement access vectors and transitions */ struct avtab te_avtab; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index d3a8f6fbc552..ca56cd045bf9 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -544,13 +544,13 @@ static void type_attribute_bounds_av(struct policydb *policydb, struct type_datum *target; u32 masked = 0; - source = policydb->type_val_to_struct_array[scontext->type - 1]; + source = policydb->type_val_to_struct[scontext->type - 1]; BUG_ON(!source); if (!source->bounds) return; - target = policydb->type_val_to_struct_array[tcontext->type - 1]; + target = policydb->type_val_to_struct[tcontext->type - 1]; BUG_ON(!target); memset(&lo_avd, 0, sizeof(lo_avd)); @@ -893,7 +893,7 @@ int security_bounded_transition(struct selinux_state *state, index = new_context->type; while (true) { - type = policydb->type_val_to_struct_array[index - 1]; + type = policydb->type_val_to_struct[index - 1]; BUG_ON(!type); /* not bounded anymore */ -- cgit v1.2.3 From 9b80c36353ed4cce324af21244a65984db21991b Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Thu, 1 Aug 2019 17:55:06 -0400 Subject: selinux: always return a secid from the network caches if we find one Previously if we couldn't find an entry in the cache and we failed to allocate memory for a new cache entry we would fail the network object label lookup; this is obviously not ideal. This patch fixes this so that we return the object label even if we can't cache the object at this point in time due to memory pressure. The GitHub issue tracker is below: * https://github.com/SELinuxProject/selinux-kernel/issues/3 Signed-off-by: Paul Moore --- security/selinux/netif.c | 31 +++++++++++++------------------ security/selinux/netnode.c | 30 ++++++++++++++---------------- security/selinux/netport.c | 24 +++++++++++------------- 3 files changed, 38 insertions(+), 47 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/netif.c b/security/selinux/netif.c index 8c738c189942..cbe6ec246412 100644 --- a/security/selinux/netif.c +++ b/security/selinux/netif.c @@ -135,9 +135,9 @@ static void sel_netif_destroy(struct sel_netif *netif) */ static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid) { - int ret; + int ret = 0; struct sel_netif *netif; - struct sel_netif *new = NULL; + struct sel_netif *new; struct net_device *dev; /* NOTE: we always use init's network namespace since we don't @@ -154,32 +154,27 @@ static int sel_netif_sid_slow(struct net *ns, int ifindex, u32 *sid) netif = sel_netif_find(ns, ifindex); if (netif != NULL) { *sid = netif->nsec.sid; - ret = 0; goto out; } - new = kzalloc(sizeof(*new), GFP_ATOMIC); - if (new == NULL) { - ret = -ENOMEM; - goto out; - } - ret = security_netif_sid(&selinux_state, dev->name, &new->nsec.sid); - if (ret != 0) - goto out; - new->nsec.ns = ns; - new->nsec.ifindex = ifindex; - ret = sel_netif_insert(new); + + ret = security_netif_sid(&selinux_state, dev->name, sid); if (ret != 0) goto out; - *sid = new->nsec.sid; + new = kzalloc(sizeof(*new), GFP_ATOMIC); + if (new) { + new->nsec.ns = ns; + new->nsec.ifindex = ifindex; + new->nsec.sid = *sid; + if (sel_netif_insert(new)) + kfree(new); + } out: spin_unlock_bh(&sel_netif_lock); dev_put(dev); - if (unlikely(ret)) { + if (unlikely(ret)) pr_warn("SELinux: failure in %s(), unable to determine network interface label (%d)\n", __func__, ifindex); - kfree(new); - } return ret; } diff --git a/security/selinux/netnode.c b/security/selinux/netnode.c index afa0d432436b..df590aaa33f4 100644 --- a/security/selinux/netnode.c +++ b/security/selinux/netnode.c @@ -199,9 +199,9 @@ static void sel_netnode_insert(struct sel_netnode *node) */ static int sel_netnode_sid_slow(void *addr, u16 family, u32 *sid) { - int ret = -ENOMEM; + int ret; struct sel_netnode *node; - struct sel_netnode *new = NULL; + struct sel_netnode *new; spin_lock_bh(&sel_netnode_lock); node = sel_netnode_find(addr, family); @@ -210,38 +210,36 @@ static int sel_netnode_sid_slow(void *addr, u16 family, u32 *sid) spin_unlock_bh(&sel_netnode_lock); return 0; } + new = kzalloc(sizeof(*new), GFP_ATOMIC); - if (new == NULL) - goto out; switch (family) { case PF_INET: ret = security_node_sid(&selinux_state, PF_INET, addr, sizeof(struct in_addr), sid); - new->nsec.addr.ipv4 = *(__be32 *)addr; + if (new) + new->nsec.addr.ipv4 = *(__be32 *)addr; break; case PF_INET6: ret = security_node_sid(&selinux_state, PF_INET6, addr, sizeof(struct in6_addr), sid); - new->nsec.addr.ipv6 = *(struct in6_addr *)addr; + if (new) + new->nsec.addr.ipv6 = *(struct in6_addr *)addr; break; default: BUG(); ret = -EINVAL; } - if (ret != 0) - goto out; - - new->nsec.family = family; - new->nsec.sid = *sid; - sel_netnode_insert(new); + if (ret == 0 && new) { + new->nsec.family = family; + new->nsec.sid = *sid; + sel_netnode_insert(new); + } else + kfree(new); -out: spin_unlock_bh(&sel_netnode_lock); - if (unlikely(ret)) { + if (unlikely(ret)) pr_warn("SELinux: failure in %s(), unable to determine network node label\n", __func__); - kfree(new); - } return ret; } diff --git a/security/selinux/netport.c b/security/selinux/netport.c index 7a141cadbffc..936d630a938d 100644 --- a/security/selinux/netport.c +++ b/security/selinux/netport.c @@ -147,9 +147,9 @@ static void sel_netport_insert(struct sel_netport *port) */ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) { - int ret = -ENOMEM; + int ret; struct sel_netport *port; - struct sel_netport *new = NULL; + struct sel_netport *new; spin_lock_bh(&sel_netport_lock); port = sel_netport_find(protocol, pnum); @@ -158,25 +158,23 @@ static int sel_netport_sid_slow(u8 protocol, u16 pnum, u32 *sid) spin_unlock_bh(&sel_netport_lock); return 0; } - new = kzalloc(sizeof(*new), GFP_ATOMIC); - if (new == NULL) - goto out; + ret = security_port_sid(&selinux_state, protocol, pnum, sid); if (ret != 0) goto out; - - new->psec.port = pnum; - new->psec.protocol = protocol; - new->psec.sid = *sid; - sel_netport_insert(new); + new = kzalloc(sizeof(*new), GFP_ATOMIC); + if (new) { + new->psec.port = pnum; + new->psec.protocol = protocol; + new->psec.sid = *sid; + sel_netport_insert(new); + } out: spin_unlock_bh(&sel_netport_lock); - if (unlikely(ret)) { + if (unlikely(ret)) pr_warn("SELinux: failure in %s(), unable to determine network port label\n", __func__); - kfree(new); - } return ret; } -- cgit v1.2.3 From ac5656d8a4cdd93cd2c74355ed12e5617817e0e7 Mon Sep 17 00:00:00 2001 From: Aaron Goidel Date: Mon, 12 Aug 2019 11:20:00 -0400 Subject: fanotify, inotify, dnotify, security: add security hook for fs notifications As of now, setting watches on filesystem objects has, at most, applied a check for read access to the inode, and in the case of fanotify, requires CAP_SYS_ADMIN. No specific security hook or permission check has been provided to control the setting of watches. Using any of inotify, dnotify, or fanotify, it is possible to observe, not only write-like operations, but even read access to a file. Modeling the watch as being merely a read from the file is insufficient for the needs of SELinux. This is due to the fact that read access should not necessarily imply access to information about when another process reads from a file. Furthermore, fanotify watches grant more power to an application in the form of permission events. While notification events are solely, unidirectional (i.e. they only pass information to the receiving application), permission events are blocking. Permission events make a request to the receiving application which will then reply with a decision as to whether or not that action may be completed. This causes the issue of the watching application having the ability to exercise control over the triggering process. Without drawing a distinction within the permission check, the ability to read would imply the greater ability to control an application. Additionally, mount and superblock watches apply to all files within the same mount or superblock. Read access to one file should not necessarily imply the ability to watch all files accessed within a given mount or superblock. In order to solve these issues, a new LSM hook is implemented and has been placed within the system calls for marking filesystem objects with inotify, fanotify, and dnotify watches. These calls to the hook are placed at the point at which the target path has been resolved and are provided with the path struct, the mask of requested notification events, and the type of object on which the mark is being set (inode, superblock, or mount). The mask and obj_type have already been translated into common FS_* values shared by the entirety of the fs notification infrastructure. The path struct is passed rather than just the inode so that the mount is available, particularly for mount watches. This also allows for use of the hook by pathname-based security modules. However, since the hook is intended for use even by inode based security modules, it is not placed under the CONFIG_SECURITY_PATH conditional. Otherwise, the inode-based security modules would need to enable all of the path hooks, even though they do not use any of them. This only provides a hook at the point of setting a watch, and presumes that permission to set a particular watch implies the ability to receive all notification about that object which match the mask. This is all that is required for SELinux. If other security modules require additional hooks or infrastructure to control delivery of notification, these can be added by them. It does not make sense for us to propose hooks for which we have no implementation. The understanding that all notifications received by the requesting application are all strictly of a type for which the application has been granted permission shows that this implementation is sufficient in its coverage. Security modules wishing to provide complete control over fanotify must also implement a security_file_open hook that validates that the access requested by the watching application is authorized. Fanotify has the issue that it returns a file descriptor with the file mode specified during fanotify_init() to the watching process on event. This is already covered by the LSM security_file_open hook if the security module implements checking of the requested file mode there. Otherwise, a watching process can obtain escalated access to a file for which it has not been authorized. The selinux_path_notify hook implementation works by adding five new file permissions: watch, watch_mount, watch_sb, watch_reads, and watch_with_perm (descriptions about which will follow), and one new filesystem permission: watch (which is applied to superblock checks). The hook then decides which subset of these permissions must be held by the requesting application based on the contents of the provided mask and the obj_type. The selinux_file_open hook already checks the requested file mode and therefore ensures that a watching process cannot escalate its access through fanotify. The watch, watch_mount, and watch_sb permissions are the baseline permissions for setting a watch on an object and each are a requirement for any watch to be set on a file, mount, or superblock respectively. It should be noted that having either of the other two permissions (watch_reads and watch_with_perm) does not imply the watch, watch_mount, or watch_sb permission. Superblock watches further require the filesystem watch permission to the superblock. As there is no labeled object in view for mounts, there is no specific check for mount watches beyond watch_mount to the inode. Such a check could be added in the future, if a suitable labeled object existed representing the mount. The watch_reads permission is required to receive notifications from read-exclusive events on filesystem objects. These events include accessing a file for the purpose of reading and closing a file which has been opened read-only. This distinction has been drawn in order to provide a direct indication in the policy for this otherwise not obvious capability. Read access to a file should not necessarily imply the ability to observe read events on a file. Finally, watch_with_perm only applies to fanotify masks since it is the only way to set a mask which allows for the blocking, permission event. This permission is needed for any watch which is of this type. Though fanotify requires CAP_SYS_ADMIN, this is insufficient as it gives implicit trust to root, which we do not do, and does not support least privilege. Signed-off-by: Aaron Goidel Acked-by: Casey Schaufler Acked-by: Jan Kara Signed-off-by: Paul Moore --- security/selinux/hooks.c | 47 +++++++++++++++++++++++++++++++++++++ security/selinux/include/classmap.h | 5 ++-- 2 files changed, 50 insertions(+), 2 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index f77b314d0575..d55571c585ff 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -92,6 +92,8 @@ #include #include /* for hashlen_string() */ #include +#include +#include #include "avc.h" #include "objsec.h" @@ -3261,6 +3263,50 @@ static int selinux_inode_removexattr(struct dentry *dentry, const char *name) return -EACCES; } +static int selinux_path_notify(const struct path *path, u64 mask, + unsigned int obj_type) +{ + int ret; + u32 perm; + + struct common_audit_data ad; + + ad.type = LSM_AUDIT_DATA_PATH; + ad.u.path = *path; + + /* + * Set permission needed based on the type of mark being set. + * Performs an additional check for sb watches. + */ + switch (obj_type) { + case FSNOTIFY_OBJ_TYPE_VFSMOUNT: + perm = FILE__WATCH_MOUNT; + break; + case FSNOTIFY_OBJ_TYPE_SB: + perm = FILE__WATCH_SB; + ret = superblock_has_perm(current_cred(), path->dentry->d_sb, + FILESYSTEM__WATCH, &ad); + if (ret) + return ret; + break; + case FSNOTIFY_OBJ_TYPE_INODE: + perm = FILE__WATCH; + break; + default: + return -EINVAL; + } + + /* blocking watches require the file:watch_with_perm permission */ + if (mask & (ALL_FSNOTIFY_PERM_EVENTS)) + perm |= FILE__WATCH_WITH_PERM; + + /* watches on read-like events need the file:watch_reads permission */ + if (mask & (FS_ACCESS | FS_ACCESS_PERM | FS_CLOSE_NOWRITE)) + perm |= FILE__WATCH_READS; + + return path_has_perm(current_cred(), path, perm); +} + /* * Copy the inode security context value to the user. * @@ -6798,6 +6844,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(inode_getsecid, selinux_inode_getsecid), LSM_HOOK_INIT(inode_copy_up, selinux_inode_copy_up), LSM_HOOK_INIT(inode_copy_up_xattr, selinux_inode_copy_up_xattr), + LSM_HOOK_INIT(path_notify, selinux_path_notify), LSM_HOOK_INIT(kernfs_init_security, selinux_kernfs_init_security), diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h index 201f7e588a29..32e9b03be3dd 100644 --- a/security/selinux/include/classmap.h +++ b/security/selinux/include/classmap.h @@ -7,7 +7,8 @@ #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \ "rename", "execute", "quotaon", "mounton", "audit_access", \ - "open", "execmod" + "open", "execmod", "watch", "watch_mount", "watch_sb", \ + "watch_with_perm", "watch_reads" #define COMMON_SOCK_PERMS COMMON_FILE_SOCK_PERMS, "bind", "connect", \ "listen", "accept", "getopt", "setopt", "shutdown", "recvfrom", \ @@ -60,7 +61,7 @@ struct security_class_mapping secclass_map[] = { { "filesystem", { "mount", "remount", "unmount", "getattr", "relabelfrom", "relabelto", "associate", "quotamod", - "quotaget", NULL } }, + "quotaget", "watch", NULL } }, { "file", { COMMON_FILE_PERMS, "execute_no_trans", "entrypoint", NULL } }, -- cgit v1.2.3 From 116f21bb967fcef1fa360fe591a2947481788020 Mon Sep 17 00:00:00 2001 From: Ondrej Mosnacek Date: Wed, 14 Aug 2019 15:33:20 +0200 Subject: selinux: avoid atomic_t usage in sidtab As noted in Documentation/atomic_t.txt, if we don't need the RMW atomic operations, we should only use READ_ONCE()/WRITE_ONCE() + smp_rmb()/smp_wmb() where necessary (or the combined variants smp_load_acquire()/smp_store_release()). This patch converts the sidtab code to use regular u32 for the counter and reverse lookup cache and use the appropriate operations instead of atomic_get()/atomic_set(). Note that when reading/updating the reverse lookup cache we don't need memory barriers as it doesn't need to be consistent or accurate. We can now also replace some atomic ops with regular loads (when under spinlock) and stores (for conversion target fields that are always accessed under the master table's spinlock). We can now also bump SIDTAB_MAX to U32_MAX as we can use the full u32 range again. Suggested-by: Jann Horn Signed-off-by: Ondrej Mosnacek Reviewed-by: Jann Horn Signed-off-by: Paul Moore --- security/selinux/ss/sidtab.c | 48 +++++++++++++++++++------------------------- security/selinux/ss/sidtab.h | 19 +++++++++++++----- 2 files changed, 35 insertions(+), 32 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index 1f0a6eaa2d6a..7d49994e8d5f 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -12,7 +12,7 @@ #include #include #include -#include +#include #include "flask.h" #include "security.h" #include "sidtab.h" @@ -23,14 +23,14 @@ int sidtab_init(struct sidtab *s) memset(s->roots, 0, sizeof(s->roots)); + /* max count is SIDTAB_MAX so valid index is always < SIDTAB_MAX */ for (i = 0; i < SIDTAB_RCACHE_SIZE; i++) - atomic_set(&s->rcache[i], -1); + s->rcache[i] = SIDTAB_MAX; for (i = 0; i < SECINITSID_NUM; i++) s->isids[i].set = 0; - atomic_set(&s->count, 0); - + s->count = 0; s->convert = NULL; spin_lock_init(&s->lock); @@ -130,14 +130,12 @@ static struct context *sidtab_do_lookup(struct sidtab *s, u32 index, int alloc) static struct context *sidtab_lookup(struct sidtab *s, u32 index) { - u32 count = (u32)atomic_read(&s->count); + /* read entries only after reading count */ + u32 count = smp_load_acquire(&s->count); if (index >= count) return NULL; - /* read entries after reading count */ - smp_rmb(); - return sidtab_do_lookup(s, index, 0); } @@ -210,10 +208,10 @@ static int sidtab_find_context(union sidtab_entry_inner entry, static void sidtab_rcache_update(struct sidtab *s, u32 index, u32 pos) { while (pos > 0) { - atomic_set(&s->rcache[pos], atomic_read(&s->rcache[pos - 1])); + WRITE_ONCE(s->rcache[pos], READ_ONCE(s->rcache[pos - 1])); --pos; } - atomic_set(&s->rcache[0], (int)index); + WRITE_ONCE(s->rcache[0], index); } static void sidtab_rcache_push(struct sidtab *s, u32 index) @@ -227,14 +225,14 @@ static int sidtab_rcache_search(struct sidtab *s, struct context *context, u32 i; for (i = 0; i < SIDTAB_RCACHE_SIZE; i++) { - int v = atomic_read(&s->rcache[i]); + u32 v = READ_ONCE(s->rcache[i]); - if (v < 0) + if (v >= SIDTAB_MAX) continue; - if (context_cmp(sidtab_do_lookup(s, (u32)v, 0), context)) { - sidtab_rcache_update(s, (u32)v, i); - *index = (u32)v; + if (context_cmp(sidtab_do_lookup(s, v, 0), context)) { + sidtab_rcache_update(s, v, i); + *index = v; return 0; } } @@ -245,8 +243,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, u32 *index) { unsigned long flags; - u32 count = (u32)atomic_read(&s->count); - u32 count_locked, level, pos; + u32 count, count_locked, level, pos; struct sidtab_convert_params *convert; struct context *dst, *dst_convert; int rc; @@ -255,11 +252,10 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, if (rc == 0) return 0; + /* read entries only after reading count */ + count = smp_load_acquire(&s->count); level = sidtab_level_from_count(count); - /* read entries after reading count */ - smp_rmb(); - pos = 0; rc = sidtab_find_context(s->roots[level], &pos, count, level, context, index); @@ -272,7 +268,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, spin_lock_irqsave(&s->lock, flags); convert = s->convert; - count_locked = (u32)atomic_read(&s->count); + count_locked = s->count; level = sidtab_level_from_count(count_locked); /* if count has changed before we acquired the lock, then catch up */ @@ -320,7 +316,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, } /* at this point we know the insert won't fail */ - atomic_set(&convert->target->count, count + 1); + convert->target->count = count + 1; } if (context->len) @@ -331,9 +327,7 @@ static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, *index = count; /* write entries before writing new count */ - smp_wmb(); - - atomic_set(&s->count, count + 1); + smp_store_release(&s->count, count + 1); rc = 0; out_unlock: @@ -423,7 +417,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) return -EBUSY; } - count = (u32)atomic_read(&s->count); + count = s->count; level = sidtab_level_from_count(count); /* allocate last leaf in the new sidtab (to avoid race with @@ -436,7 +430,7 @@ int sidtab_convert(struct sidtab *s, struct sidtab_convert_params *params) } /* set count in case no new entries are added during conversion */ - atomic_set(¶ms->target->count, count); + params->target->count = count; /* enable live convert of new entries */ s->convert = params; diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index bbd5c0d1f3bd..1f4763141aa1 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -40,8 +40,8 @@ union sidtab_entry_inner { #define SIDTAB_LEAF_ENTRIES \ (SIDTAB_NODE_ALLOC_SIZE / sizeof(struct sidtab_entry_leaf)) -#define SIDTAB_MAX_BITS 31 /* limited to INT_MAX due to atomic_t range */ -#define SIDTAB_MAX (((u32)1 << SIDTAB_MAX_BITS) - 1) +#define SIDTAB_MAX_BITS 32 +#define SIDTAB_MAX U32_MAX /* ensure enough tree levels for SIDTAB_MAX entries */ #define SIDTAB_MAX_LEVEL \ DIV_ROUND_UP(SIDTAB_MAX_BITS - size_to_shift(SIDTAB_LEAF_ENTRIES), \ @@ -69,13 +69,22 @@ struct sidtab_convert_params { #define SIDTAB_RCACHE_SIZE 3 struct sidtab { + /* + * lock-free read access only for as many items as a prior read of + * 'count' + */ union sidtab_entry_inner roots[SIDTAB_MAX_LEVEL + 1]; - atomic_t count; + /* + * access atomically via {READ|WRITE}_ONCE(); only increment under + * spinlock + */ + u32 count; + /* access only under spinlock */ struct sidtab_convert_params *convert; spinlock_t lock; - /* reverse lookup cache */ - atomic_t rcache[SIDTAB_RCACHE_SIZE]; + /* reverse lookup cache - access atomically via {READ|WRITE}_ONCE() */ + u32 rcache[SIDTAB_RCACHE_SIZE]; /* index == SID - 1 (no entry for SECSID_NULL) */ struct sidtab_isid_entry isids[SECINITSID_NUM]; -- cgit v1.2.3 From 169ce0c081cd85f78388bb6c1638c1ad7b81bde7 Mon Sep 17 00:00:00 2001 From: Stephen Smalley Date: Wed, 4 Sep 2019 10:32:48 -0400 Subject: selinux: fix residual uses of current_security() for the SELinux blob We need to use selinux_cred() to fetch the SELinux cred blob instead of directly using current->security or current_security(). There were a couple of lingering uses of current_security() in the SELinux code that were apparently missed during the earlier conversions. IIUC, this would only manifest as a bug if multiple security modules including SELinux are enabled and SELinux is not first in the lsm order. After this change, there appear to be no other users of current_security() in-tree; perhaps we should remove it altogether. Fixes: bbd3662a8348 ("Infrastructure management of the cred security blob") Signed-off-by: Stephen Smalley Acked-by: Casey Schaufler Reviewed-by: James Morris Signed-off-by: Paul Moore --- security/selinux/hooks.c | 2 +- security/selinux/include/objsec.h | 20 ++++++++++---------- 2 files changed, 11 insertions(+), 11 deletions(-) (limited to 'security/selinux') diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index d55571c585ff..f1b763eceef9 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -3435,7 +3435,7 @@ static int selinux_inode_copy_up_xattr(const char *name) static int selinux_kernfs_init_security(struct kernfs_node *kn_dir, struct kernfs_node *kn) { - const struct task_security_struct *tsec = current_security(); + const struct task_security_struct *tsec = selinux_cred(current_cred()); u32 parent_sid, newsid, clen; int rc; char *context; diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h index 231262d8eac9..d2e00c7595dd 100644 --- a/security/selinux/include/objsec.h +++ b/security/selinux/include/objsec.h @@ -40,16 +40,6 @@ struct task_security_struct { u32 sockcreate_sid; /* fscreate SID */ }; -/* - * get the subjective security ID of the current task - */ -static inline u32 current_sid(void) -{ - const struct task_security_struct *tsec = current_security(); - - return tsec->sid; -} - enum label_initialized { LABEL_INVALID, /* invalid or not initialized */ LABEL_INITIALIZED, /* initialized */ @@ -188,4 +178,14 @@ static inline struct ipc_security_struct *selinux_ipc( return ipc->security + selinux_blob_sizes.lbs_ipc; } +/* + * get the subjective security ID of the current task + */ +static inline u32 current_sid(void) +{ + const struct task_security_struct *tsec = selinux_cred(current_cred()); + + return tsec->sid; +} + #endif /* _SELINUX_OBJSEC_H_ */ -- cgit v1.2.3