| Message ID | 20181113135255.26045-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix ENOMEM errors during policy reload | expand |
On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: > This patch is non-functional and moves handling of initial SIDs into a > separate table. Note that the SIDs stored in the main table are now > shifted by SECINITSID_NUM and converted to/from the actual SIDs > transparently by helper functions. When you say "non-functional", you mean it doesn't make any functional changes, right? As opposed to leaving the kernel in a broken intermediate state until your 3rd patch? I think you need to double check that all references to state->ss->sidtab are preceded by a check of state->initialized since it could be NULL before first policy load and a system could come up with SELinux enabled but never load a policy. Aren't you still wasting a slot in the initial SIDs array, or did I miss something? > > This change doesn't make much sense on its own, but it simplifies > further sidtab overhaul in a succeeding patch. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > security/selinux/ss/policydb.c | 10 ++- > security/selinux/ss/services.c | 88 ++++++++++-------- > security/selinux/ss/services.h | 2 +- > security/selinux/ss/sidtab.c | 158 +++++++++++++++++++-------------- > security/selinux/ss/sidtab.h | 14 +-- > 5 files changed, 162 insertions(+), 110 deletions(-) > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > index f4eadd3f7350..21e4bdbcf994 100644 > --- a/security/selinux/ss/policydb.c > +++ b/security/selinux/ss/policydb.c > @@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) > if (!c->context[0].user) { > pr_err("SELinux: SID %s was never defined.\n", > c->u.name); > + sidtab_destroy(s); > + goto out; > + } > + if (c->sid[0] > SECINITSID_NUM) { > + pr_err("SELinux: Initial SID %s out of range.\n", > + c->u.name); > + sidtab_destroy(s); > goto out; > } > > - rc = sidtab_insert(s, c->sid[0], &c->context[0]); > + rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); > if (rc) { > pr_err("SELinux: unable to load initial SID %s.\n", > c->u.name); > + sidtab_destroy(s); > goto out; > } > } > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 7337db24a6a8..30170d4c567a 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > if (!user) > tclass = unmap_class(&state->ss->map, orig_tclass); > @@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > rc = -EINVAL; > old_context = sidtab_search(sidtab, old_sid); > @@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct selinux_state *state, > goto allow; > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > scontext = sidtab_search(sidtab, ssid); > if (!scontext) { > @@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state, > goto allow; > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > scontext = sidtab_search(sidtab, ssid); > if (!scontext) { > @@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state, > goto allow; > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > scontext = sidtab_search(sidtab, ssid); > if (!scontext) { > @@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct selinux_state *state, > } > read_lock(&state->ss->policy_rwlock); > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > if (force) > context = sidtab_search_force(sidtab, sid); > else > @@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct selinux_state *state, > } > read_lock(&state->ss->policy_rwlock); > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > rc = string_to_context_struct(policydb, sidtab, scontext2, > &context, def_sid); > if (rc == -EINVAL && force) { > @@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state *state, > } > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > scontext = sidtab_search(sidtab, ssid); > if (!scontext) { > @@ -1925,10 +1925,7 @@ static int convert_context(u32 key, > struct user_datum *usrdatum; > char *s; > u32 len; > - int rc = 0; > - > - if (key <= SECINITSID_NUM) > - goto out; > + int rc; > > args = p; > > @@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state *state, > int security_load_policy(struct selinux_state *state, void *data, size_t len) > { > struct policydb *policydb; > - struct sidtab *sidtab; > + struct sidtab *oldsidtab, *newsidtab; > struct policydb *oldpolicydb, *newpolicydb; > - struct sidtab oldsidtab, newsidtab; > struct selinux_mapping *oldmapping; > struct selinux_map newmap; > struct convert_context_args args; > @@ -2108,27 +2104,37 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > newpolicydb = oldpolicydb + 1; > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + > + newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL); > + if (!newsidtab) { > + rc = -ENOMEM; > + goto out; > + } > > if (!state->initialized) { > rc = policydb_read(policydb, fp); > - if (rc) > + if (rc) { > + kfree(newsidtab); > goto out; > + } > > policydb->len = len; > rc = selinux_set_mapping(policydb, secclass_map, > &state->ss->map); > if (rc) { > + kfree(newsidtab); > policydb_destroy(policydb); > goto out; > } > > - rc = policydb_load_isids(policydb, sidtab); > + rc = policydb_load_isids(policydb, newsidtab); > if (rc) { > + kfree(newsidtab); > policydb_destroy(policydb); > goto out; > } > > + state->ss->sidtab = newsidtab; > security_load_policycaps(state); > state->initialized = 1; > seqno = ++state->ss->latest_granting; > @@ -2141,13 +2147,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > goto out; > } > > + oldsidtab = state->ss->sidtab; > + > #if 0 > - sidtab_hash_eval(sidtab, "sids"); > + sidtab_hash_eval(oldsidtab, "sids"); > #endif > > rc = policydb_read(newpolicydb, fp); > - if (rc) > + if (rc) { > + kfree(newsidtab); > goto out; > + } > > newpolicydb->len = len; > /* If switching between different policy types, log MLS status */ > @@ -2156,10 +2166,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > else if (!policydb->mls_enabled && newpolicydb->mls_enabled) > pr_info("SELinux: Enabling MLS support...\n"); > > - rc = policydb_load_isids(newpolicydb, &newsidtab); > + rc = policydb_load_isids(newpolicydb, newsidtab); > if (rc) { > pr_err("SELinux: unable to load the initial SIDs\n"); > policydb_destroy(newpolicydb); > + kfree(newsidtab); > goto out; > } > > @@ -2180,7 +2191,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > args.state = state; > args.oldp = policydb; > args.newp = newpolicydb; > - rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args); > + rc = sidtab_convert(oldsidtab, newsidtab, convert_context, &args); > if (rc) { > pr_err("SELinux: unable to convert the internal" > " representation of contexts in the new SID" > @@ -2190,12 +2201,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > /* Save the old policydb and SID table to free later. */ > memcpy(oldpolicydb, policydb, sizeof(*policydb)); > - sidtab_set(&oldsidtab, sidtab); > > /* Install the new policydb and SID table. */ > write_lock_irq(&state->ss->policy_rwlock); > memcpy(policydb, newpolicydb, sizeof(*policydb)); > - sidtab_set(sidtab, &newsidtab); > + state->ss->sidtab = newsidtab; > security_load_policycaps(state); > oldmapping = state->ss->map.mapping; > state->ss->map.mapping = newmap.mapping; > @@ -2205,7 +2215,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > /* Free the old policydb and SID table. */ > policydb_destroy(oldpolicydb); > - sidtab_destroy(&oldsidtab); > + sidtab_destroy(oldsidtab); > + kfree(oldsidtab); > kfree(oldmapping); > > avc_ss_reset(state->avc, seqno); > @@ -2219,7 +2230,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > err: > kfree(newmap.mapping); > - sidtab_destroy(&newsidtab); > + sidtab_destroy(newsidtab); > + kfree(newsidtab); > policydb_destroy(newpolicydb); > > out: > @@ -2256,7 +2268,7 @@ int security_port_sid(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > c = policydb->ocontexts[OCON_PORT]; > while (c) { > @@ -2302,7 +2314,7 @@ int security_ib_pkey_sid(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > c = policydb->ocontexts[OCON_IBPKEY]; > while (c) { > @@ -2348,7 +2360,7 @@ int security_ib_endport_sid(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > c = policydb->ocontexts[OCON_IBENDPORT]; > while (c) { > @@ -2394,7 +2406,7 @@ int security_netif_sid(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > c = policydb->ocontexts[OCON_NETIF]; > while (c) { > @@ -2459,7 +2471,7 @@ int security_node_sid(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > switch (domain) { > case AF_INET: { > @@ -2559,7 +2571,7 @@ int security_get_user_sids(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > context_init(&usercon); > > @@ -2661,7 +2673,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, > u32 *sid) > { > struct policydb *policydb = &state->ss->policydb; > - struct sidtab *sidtab = &state->ss->sidtab; > + struct sidtab *sidtab = state->ss->sidtab; > int len; > u16 sclass; > struct genfs *genfs; > @@ -2747,7 +2759,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > - sidtab = &state->ss->sidtab; > + sidtab = state->ss->sidtab; > > c = policydb->ocontexts[OCON_FSUSE]; > while (c) { > @@ -2953,7 +2965,7 @@ int security_sid_mls_copy(struct selinux_state *state, > u32 sid, u32 mls_sid, u32 *new_sid) > { > struct policydb *policydb = &state->ss->policydb; > - struct sidtab *sidtab = &state->ss->sidtab; > + struct sidtab *sidtab = state->ss->sidtab; > struct context *context1; > struct context *context2; > struct context newcon; > @@ -3044,7 +3056,7 @@ int security_net_peersid_resolve(struct selinux_state *state, > u32 *peer_sid) > { > struct policydb *policydb = &state->ss->policydb; > - struct sidtab *sidtab = &state->ss->sidtab; > + struct sidtab *sidtab = state->ss->sidtab; > int rc; > struct context *nlbl_ctx; > struct context *xfrm_ctx; > @@ -3405,7 +3417,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, > goto out; > } > > - ctxt = sidtab_search(&state->ss->sidtab, sid); > + ctxt = sidtab_search(state->ss->sidtab, sid); > if (unlikely(!ctxt)) { > WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n", > sid); > @@ -3568,7 +3580,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state, > u32 *sid) > { > struct policydb *policydb = &state->ss->policydb; > - struct sidtab *sidtab = &state->ss->sidtab; > + struct sidtab *sidtab = state->ss->sidtab; > int rc; > struct context *ctx; > struct context ctx_new; > @@ -3646,7 +3658,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, > read_lock(&state->ss->policy_rwlock); > > rc = -ENOENT; > - ctx = sidtab_search(&state->ss->sidtab, sid); > + ctx = sidtab_search(state->ss->sidtab, sid); > if (ctx == NULL) > goto out; > > diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h > index 24c7bdcc8075..9a36de860368 100644 > --- a/security/selinux/ss/services.h > +++ b/security/selinux/ss/services.h > @@ -24,7 +24,7 @@ struct selinux_map { > }; > > struct selinux_ss { > - struct sidtab sidtab; > + struct sidtab *sidtab; > struct policydb policydb; > rwlock_t policy_rwlock; > u32 latest_granting; > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > index e66a2ab3d1c2..049ac1e6fd06 100644 > --- a/security/selinux/ss/sidtab.c > +++ b/security/selinux/ss/sidtab.c > @@ -22,16 +22,24 @@ int sidtab_init(struct sidtab *s) > s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC); > if (!s->htable) > return -ENOMEM; > + > + for (i = 0; i <= SECINITSID_NUM; i++) > + s->isids[i].set = 0; > + > for (i = 0; i < SIDTAB_SIZE; i++) > s->htable[i] = NULL; > + > + for (i = 0; i < SIDTAB_CACHE_LEN; i++) > + s->cache[i] = NULL; > + > s->nel = 0; > - s->next_sid = 1; > + s->next_sid = 0; > s->shutdown = 0; > spin_lock_init(&s->lock); > return 0; > } > > -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > +static int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > { > int hvalue; > struct sidtab_node *prev, *cur, *newnode; > @@ -76,34 +84,53 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > return 0; > } > > -static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context) > +{ > + struct sidtab_isid_entry *entry = &s->isids[sid]; > + int rc = context_cpy(&entry->context, context); > + if (rc) > + return rc; > + > + entry->set = 1; > + return 0; > +} > + > +static struct context *sidtab_lookup(struct sidtab *s, u32 sid) > { > int hvalue; > struct sidtab_node *cur; > > - if (!s) > - return NULL; > - > hvalue = SIDTAB_HASH(sid); > cur = s->htable[hvalue]; > while (cur && sid > cur->sid) > cur = cur->next; > > - if (force && cur && sid == cur->sid && cur->context.len) > - return &cur->context; > + if (!cur || sid != cur->sid) > + return NULL; > > - if (!cur || sid != cur->sid || cur->context.len) { > - /* Remap invalid SIDs to the unlabeled SID. */ > - sid = SECINITSID_UNLABELED; > - hvalue = SIDTAB_HASH(sid); > - cur = s->htable[hvalue]; > - while (cur && sid > cur->sid) > - cur = cur->next; > - if (!cur || sid != cur->sid) > - return NULL; > + return &cur->context; > +} > + > +static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > +{ > + struct context *context; > + struct sidtab_isid_entry *entry; > + > + if (!s) > + return NULL; > + > + if (sid > SECINITSID_NUM) { > + u32 index = sid - (SECINITSID_NUM + 1); > + context = sidtab_lookup(s, index); > + } else { > + entry = &s->isids[sid]; > + context = entry->set ? &entry->context : NULL; > } > + if (context && (!context->len || force)) > + return context; > > - return &cur->context; > + entry = &s->isids[SECINITSID_UNLABELED]; > + return entry->set ? &entry->context : NULL; > } > > struct context *sidtab_search(struct sidtab *s, u32 sid) > @@ -145,11 +172,7 @@ out: > static int clone_sid(u32 sid, struct context *context, void *arg) > { > struct sidtab *s = arg; > - > - if (sid > SECINITSID_NUM) > - return sidtab_insert(s, sid, context); > - else > - return 0; > + return sidtab_insert(s, sid, context); > } > > int sidtab_convert(struct sidtab *s, struct sidtab *news, > @@ -183,8 +206,8 @@ static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc > s->cache[0] = n; > } > > -static inline u32 sidtab_search_context(struct sidtab *s, > - struct context *context) > +static inline int sidtab_search_context(struct sidtab *s, > + struct context *context, u32 *sid) > { > int i; > struct sidtab_node *cur; > @@ -194,15 +217,17 @@ static inline u32 sidtab_search_context(struct sidtab *s, > while (cur) { > if (context_cmp(&cur->context, context)) { > sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); > - return cur->sid; > + *sid = cur->sid; > + return 0; > } > cur = cur->next; > } > } > - return 0; > + return -ENOENT; > } > > -static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > +static inline int sidtab_search_cache(struct sidtab *s, struct context *context, > + u32 *sid) > { > int i; > struct sidtab_node *node; > @@ -210,54 +235,67 @@ static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > for (i = 0; i < SIDTAB_CACHE_LEN; i++) { > node = s->cache[i]; > if (unlikely(!node)) > - return 0; > + return -ENOENT; > if (context_cmp(&node->context, context)) { > sidtab_update_cache(s, node, i); > - return node->sid; > + *sid = node->sid; > + return 0; > } > } > - return 0; > + return -ENOENT; > } > > -int sidtab_context_to_sid(struct sidtab *s, > - struct context *context, > - u32 *out_sid) > +static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > + u32 *sid) > { > - u32 sid; > - int ret = 0; > + int ret; > unsigned long flags; > > - *out_sid = SECSID_NULL; > - > - sid = sidtab_search_cache(s, context); > - if (!sid) > - sid = sidtab_search_context(s, context); > - if (!sid) { > + ret = sidtab_search_cache(s, context, sid); > + if (ret) > + ret = sidtab_search_context(s, context, sid); > + if (ret) { > spin_lock_irqsave(&s->lock, flags); > /* Rescan now that we hold the lock. */ > - sid = sidtab_search_context(s, context); > - if (sid) > + ret = sidtab_search_context(s, context, sid); > + if (!ret) > goto unlock_out; > /* No SID exists for the context. Allocate a new one. */ > - if (s->next_sid == UINT_MAX || s->shutdown) { > + if (s->next_sid == (UINT_MAX - SECINITSID_NUM - 1) || s->shutdown) { > ret = -ENOMEM; > goto unlock_out; > } > - sid = s->next_sid++; > + *sid = s->next_sid++; > if (context->len) > pr_info("SELinux: Context %s is not valid (left unmapped).\n", > context->str); > - ret = sidtab_insert(s, sid, context); > + ret = sidtab_insert(s, *sid, context); > if (ret) > s->next_sid--; > unlock_out: > spin_unlock_irqrestore(&s->lock, flags); > } > > - if (ret) > - return ret; > + return ret; > +} > + > +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid) > +{ > + int rc; > + u32 i; > + > + for (i = 0; i <= SECINITSID_NUM; i++) { > + struct sidtab_isid_entry *entry = &s->isids[i]; > + if (entry->set && context_cmp(context, &entry->context)) { > + *sid = i; > + return 0; > + } > + } > > - *out_sid = sid; > + rc = sidtab_reverse_lookup(s, context, sid); > + if (rc) > + return rc; > + *sid += SECINITSID_NUM + 1; > return 0; > } > > @@ -296,6 +334,11 @@ void sidtab_destroy(struct sidtab *s) > if (!s) > return; > > + for (i = 0; i <= SECINITSID_NUM; i++) > + if (s->isids[i].set) > + context_destroy(&s->isids[i].context); > + > + > for (i = 0; i < SIDTAB_SIZE; i++) { > cur = s->htable[i]; > while (cur) { > @@ -311,18 +354,3 @@ void sidtab_destroy(struct sidtab *s) > s->nel = 0; > s->next_sid = 1; > } > - > -void sidtab_set(struct sidtab *dst, struct sidtab *src) > -{ > - unsigned long flags; > - int i; > - > - spin_lock_irqsave(&src->lock, flags); > - dst->htable = src->htable; > - dst->nel = src->nel; > - dst->next_sid = src->next_sid; > - dst->shutdown = 0; > - for (i = 0; i < SIDTAB_CACHE_LEN; i++) > - dst->cache[i] = NULL; > - spin_unlock_irqrestore(&src->lock, flags); > -} > diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > index 26c74fe7afc0..e181db3589bc 100644 > --- a/security/selinux/ss/sidtab.h > +++ b/security/selinux/ss/sidtab.h > @@ -22,6 +22,11 @@ struct sidtab_node { > > #define SIDTAB_SIZE SIDTAB_HASH_BUCKETS > > +struct sidtab_isid_entry { > + int set; > + struct context context; > +}; > + > struct sidtab { > struct sidtab_node **htable; > unsigned int nel; /* number of elements */ > @@ -30,10 +35,12 @@ struct sidtab { > #define SIDTAB_CACHE_LEN 3 > struct sidtab_node *cache[SIDTAB_CACHE_LEN]; > spinlock_t lock; > + > + struct sidtab_isid_entry isids[SECINITSID_NUM + 1]; > }; > > int sidtab_init(struct sidtab *s); > -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context); > +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context); > struct context *sidtab_search(struct sidtab *s, u32 sid); > struct context *sidtab_search_force(struct sidtab *s, u32 sid); > > @@ -43,13 +50,10 @@ int sidtab_convert(struct sidtab *s, struct sidtab *news, > void *args), > void *args); > > -int sidtab_context_to_sid(struct sidtab *s, > - struct context *context, > - u32 *sid); > +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); > > void sidtab_hash_eval(struct sidtab *h, char *tag); > void sidtab_destroy(struct sidtab *s); > -void sidtab_set(struct sidtab *dst, struct sidtab *src); > > #endif /* _SS_SIDTAB_H_ */ > >
On Tue, Nov 13, 2018 at 10:35 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: > > This patch is non-functional and moves handling of initial SIDs into a > > separate table. Note that the SIDs stored in the main table are now > > shifted by SECINITSID_NUM and converted to/from the actual SIDs > > transparently by helper functions. > > When you say "non-functional", you mean it doesn't make any functional > changes, right? As opposed to leaving the kernel in a broken > intermediate state until your 3rd patch? I mean it *should* be non-functional on its own, unless I made a mistake. I admit I didn't do very much testing on this patch, but I at least checked that I can boot the kernel, load a policy and that reported file contexts make sense. > > I think you need to double check that all references to > state->ss->sidtab are preceded by a check of state->initialized since it > could be NULL before first policy load and a system could come up with > SELinux enabled but never load a policy. Well, the original code does not initialize the sidtab field with sidtab_init() either so state->ss->sidtab.htable would be NULL (or some garbage pointer) after boot and any use of sidtab would cause a GPF anyway. Looking at the places that reference the sidtab (which are all highlighted in the patch because of the switch to pointer type in the struct selinux_ss definition), these don't seem to check for state->initialized: - security_port_sid - security_ib_pkey_sid - security_ib_endport_sid - security_netif_sid - security_node_sid - security_sid_mls_copy, called from: - selinux_socket_unix_stream_connect (avc_has_perm is called before) - selinux_conn_sid, called from: - selinux_sctp_assoc_request (selinux_policycap_extsockclass is called before) - selinux_inet_conn_request - selinux_ip_postroute (selinux_policycap_netpeer is called before) - security_net_peersid_resolve, called from: - selinux_skb_peerlbl_sid, called from: - selinux_socket_sock_rcv_skb (selinux_policycap_netpeer is called before) - selinux_socket_getpeersec_dgram - selinux_sctp_assoc_request (again) - selinux_inet_conn_request (again) - selinux_inet_conn_established - selinux_ip_forward (selinux_policycap_netpeer is called before) - selinux_ip_postroute (again) - selinux_audit_rule_match - security_netlbl_secattr_to_sid, called from: - selinux_netlbl_sidlookup_cached, called from: - selinux_netlbl_skbuff_getsid (netlbl_enabled is called before) - selinux_netlbl_sock_rcv_skb (netlbl_enabled is called before) - security_netlbl_sid_to_secattr, called from: - selinux_netlbl_sock_genattr, called from: - selinux_netlbl_socket_post_create, called from: - selinux_socket_post_create - selinux_netlbl_skbuff_setsid, called from: - selinux_ip_forward (again) - selinux_netlbl_sctp_assoc_request, called from: - selinux_sctp_assoc_request (again) - selinux_netlbl_inet_conn_request, called from: - selinux_inet_conn_request (again) I suppose in some (or all?) of these cases state->initialized might be implicitly always 1, but at least in these it wasn't immediately obvious to me. Either way, such cases would already be buggy before this patch, since they would happily access the policy structure (likely hitting some NULL/invalid pointers) and most likely also dereference the invalid htable pointer in sidtab. > > Aren't you still wasting a slot in the initial SIDs array, or did I miss > something? Yes, I am, because the original code doesn't seem to guard against SECSID_NULL being loaded as initial SID into sidtab. I asked in the other thread whether this is considered a bug, but I didn't get a reply, so I left it sort of bug-to-bug compatible for now... Anyway, it doesn't seem to make much sense to keep that behavior so I will probably just go ahead and add the check to policydb_load_isids() and shrink the table. You can expect it to be resolved in the final patchset. > > > > > This change doesn't make much sense on its own, but it simplifies > > further sidtab overhaul in a succeeding patch. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > --- > > security/selinux/ss/policydb.c | 10 ++- > > security/selinux/ss/services.c | 88 ++++++++++-------- > > security/selinux/ss/services.h | 2 +- > > security/selinux/ss/sidtab.c | 158 +++++++++++++++++++-------------- > > security/selinux/ss/sidtab.h | 14 +-- > > 5 files changed, 162 insertions(+), 110 deletions(-) > > > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > > index f4eadd3f7350..21e4bdbcf994 100644 > > --- a/security/selinux/ss/policydb.c > > +++ b/security/selinux/ss/policydb.c > > @@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) > > if (!c->context[0].user) { > > pr_err("SELinux: SID %s was never defined.\n", > > c->u.name); > > + sidtab_destroy(s); > > + goto out; > > + } > > + if (c->sid[0] > SECINITSID_NUM) { > > + pr_err("SELinux: Initial SID %s out of range.\n", > > + c->u.name); > > + sidtab_destroy(s); > > goto out; > > } > > > > - rc = sidtab_insert(s, c->sid[0], &c->context[0]); > > + rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); > > if (rc) { > > pr_err("SELinux: unable to load initial SID %s.\n", > > c->u.name); > > + sidtab_destroy(s); > > goto out; > > } > > } > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > > index 7337db24a6a8..30170d4c567a 100644 > > --- a/security/selinux/ss/services.c > > +++ b/security/selinux/ss/services.c > > @@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > if (!user) > > tclass = unmap_class(&state->ss->map, orig_tclass); > > @@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > rc = -EINVAL; > > old_context = sidtab_search(sidtab, old_sid); > > @@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct selinux_state *state, > > goto allow; > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > scontext = sidtab_search(sidtab, ssid); > > if (!scontext) { > > @@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state, > > goto allow; > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > scontext = sidtab_search(sidtab, ssid); > > if (!scontext) { > > @@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state, > > goto allow; > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > scontext = sidtab_search(sidtab, ssid); > > if (!scontext) { > > @@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct selinux_state *state, > > } > > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > if (force) > > context = sidtab_search_force(sidtab, sid); > > else > > @@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct selinux_state *state, > > } > > read_lock(&state->ss->policy_rwlock); > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > rc = string_to_context_struct(policydb, sidtab, scontext2, > > &context, def_sid); > > if (rc == -EINVAL && force) { > > @@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state *state, > > } > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > scontext = sidtab_search(sidtab, ssid); > > if (!scontext) { > > @@ -1925,10 +1925,7 @@ static int convert_context(u32 key, > > struct user_datum *usrdatum; > > char *s; > > u32 len; > > - int rc = 0; > > - > > - if (key <= SECINITSID_NUM) > > - goto out; > > + int rc; > > > > args = p; > > > > @@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state *state, > > int security_load_policy(struct selinux_state *state, void *data, size_t len) > > { > > struct policydb *policydb; > > - struct sidtab *sidtab; > > + struct sidtab *oldsidtab, *newsidtab; > > struct policydb *oldpolicydb, *newpolicydb; > > - struct sidtab oldsidtab, newsidtab; > > struct selinux_mapping *oldmapping; > > struct selinux_map newmap; > > struct convert_context_args args; > > @@ -2108,27 +2104,37 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > newpolicydb = oldpolicydb + 1; > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + > > + newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL); > > + if (!newsidtab) { > > + rc = -ENOMEM; > > + goto out; > > + } > > > > if (!state->initialized) { > > rc = policydb_read(policydb, fp); > > - if (rc) > > + if (rc) { > > + kfree(newsidtab); > > goto out; > > + } > > > > policydb->len = len; > > rc = selinux_set_mapping(policydb, secclass_map, > > &state->ss->map); > > if (rc) { > > + kfree(newsidtab); > > policydb_destroy(policydb); > > goto out; > > } > > > > - rc = policydb_load_isids(policydb, sidtab); > > + rc = policydb_load_isids(policydb, newsidtab); > > if (rc) { > > + kfree(newsidtab); > > policydb_destroy(policydb); > > goto out; > > } > > > > + state->ss->sidtab = newsidtab; > > security_load_policycaps(state); > > state->initialized = 1; > > seqno = ++state->ss->latest_granting; > > @@ -2141,13 +2147,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > goto out; > > } > > > > + oldsidtab = state->ss->sidtab; > > + > > #if 0 > > - sidtab_hash_eval(sidtab, "sids"); > > + sidtab_hash_eval(oldsidtab, "sids"); > > #endif > > > > rc = policydb_read(newpolicydb, fp); > > - if (rc) > > + if (rc) { > > + kfree(newsidtab); > > goto out; > > + } > > > > newpolicydb->len = len; > > /* If switching between different policy types, log MLS status */ > > @@ -2156,10 +2166,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > else if (!policydb->mls_enabled && newpolicydb->mls_enabled) > > pr_info("SELinux: Enabling MLS support...\n"); > > > > - rc = policydb_load_isids(newpolicydb, &newsidtab); > > + rc = policydb_load_isids(newpolicydb, newsidtab); > > if (rc) { > > pr_err("SELinux: unable to load the initial SIDs\n"); > > policydb_destroy(newpolicydb); > > + kfree(newsidtab); > > goto out; > > } > > > > @@ -2180,7 +2191,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > args.state = state; > > args.oldp = policydb; > > args.newp = newpolicydb; > > - rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args); > > + rc = sidtab_convert(oldsidtab, newsidtab, convert_context, &args); > > if (rc) { > > pr_err("SELinux: unable to convert the internal" > > " representation of contexts in the new SID" > > @@ -2190,12 +2201,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > > > /* Save the old policydb and SID table to free later. */ > > memcpy(oldpolicydb, policydb, sizeof(*policydb)); > > - sidtab_set(&oldsidtab, sidtab); > > > > /* Install the new policydb and SID table. */ > > write_lock_irq(&state->ss->policy_rwlock); > > memcpy(policydb, newpolicydb, sizeof(*policydb)); > > - sidtab_set(sidtab, &newsidtab); > > + state->ss->sidtab = newsidtab; > > security_load_policycaps(state); > > oldmapping = state->ss->map.mapping; > > state->ss->map.mapping = newmap.mapping; > > @@ -2205,7 +2215,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > > > /* Free the old policydb and SID table. */ > > policydb_destroy(oldpolicydb); > > - sidtab_destroy(&oldsidtab); > > + sidtab_destroy(oldsidtab); > > + kfree(oldsidtab); > > kfree(oldmapping); > > > > avc_ss_reset(state->avc, seqno); > > @@ -2219,7 +2230,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > > > > err: > > kfree(newmap.mapping); > > - sidtab_destroy(&newsidtab); > > + sidtab_destroy(newsidtab); > > + kfree(newsidtab); > > policydb_destroy(newpolicydb); > > > > out: > > @@ -2256,7 +2268,7 @@ int security_port_sid(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > c = policydb->ocontexts[OCON_PORT]; > > while (c) { > > @@ -2302,7 +2314,7 @@ int security_ib_pkey_sid(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > c = policydb->ocontexts[OCON_IBPKEY]; > > while (c) { > > @@ -2348,7 +2360,7 @@ int security_ib_endport_sid(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > c = policydb->ocontexts[OCON_IBENDPORT]; > > while (c) { > > @@ -2394,7 +2406,7 @@ int security_netif_sid(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > c = policydb->ocontexts[OCON_NETIF]; > > while (c) { > > @@ -2459,7 +2471,7 @@ int security_node_sid(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > switch (domain) { > > case AF_INET: { > > @@ -2559,7 +2571,7 @@ int security_get_user_sids(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > context_init(&usercon); > > > > @@ -2661,7 +2673,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, > > u32 *sid) > > { > > struct policydb *policydb = &state->ss->policydb; > > - struct sidtab *sidtab = &state->ss->sidtab; > > + struct sidtab *sidtab = state->ss->sidtab; > > int len; > > u16 sclass; > > struct genfs *genfs; > > @@ -2747,7 +2759,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) > > read_lock(&state->ss->policy_rwlock); > > > > policydb = &state->ss->policydb; > > - sidtab = &state->ss->sidtab; > > + sidtab = state->ss->sidtab; > > > > c = policydb->ocontexts[OCON_FSUSE]; > > while (c) { > > @@ -2953,7 +2965,7 @@ int security_sid_mls_copy(struct selinux_state *state, > > u32 sid, u32 mls_sid, u32 *new_sid) > > { > > struct policydb *policydb = &state->ss->policydb; > > - struct sidtab *sidtab = &state->ss->sidtab; > > + struct sidtab *sidtab = state->ss->sidtab; > > struct context *context1; > > struct context *context2; > > struct context newcon; > > @@ -3044,7 +3056,7 @@ int security_net_peersid_resolve(struct selinux_state *state, > > u32 *peer_sid) > > { > > struct policydb *policydb = &state->ss->policydb; > > - struct sidtab *sidtab = &state->ss->sidtab; > > + struct sidtab *sidtab = state->ss->sidtab; > > int rc; > > struct context *nlbl_ctx; > > struct context *xfrm_ctx; > > @@ -3405,7 +3417,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, > > goto out; > > } > > > > - ctxt = sidtab_search(&state->ss->sidtab, sid); > > + ctxt = sidtab_search(state->ss->sidtab, sid); > > if (unlikely(!ctxt)) { > > WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n", > > sid); > > @@ -3568,7 +3580,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state, > > u32 *sid) > > { > > struct policydb *policydb = &state->ss->policydb; > > - struct sidtab *sidtab = &state->ss->sidtab; > > + struct sidtab *sidtab = state->ss->sidtab; > > int rc; > > struct context *ctx; > > struct context ctx_new; > > @@ -3646,7 +3658,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, > > read_lock(&state->ss->policy_rwlock); > > > > rc = -ENOENT; > > - ctx = sidtab_search(&state->ss->sidtab, sid); > > + ctx = sidtab_search(state->ss->sidtab, sid); > > if (ctx == NULL) > > goto out; > > > > diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h > > index 24c7bdcc8075..9a36de860368 100644 > > --- a/security/selinux/ss/services.h > > +++ b/security/selinux/ss/services.h > > @@ -24,7 +24,7 @@ struct selinux_map { > > }; > > > > struct selinux_ss { > > - struct sidtab sidtab; > > + struct sidtab *sidtab; > > struct policydb policydb; > > rwlock_t policy_rwlock; > > u32 latest_granting; > > diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > > index e66a2ab3d1c2..049ac1e6fd06 100644 > > --- a/security/selinux/ss/sidtab.c > > +++ b/security/selinux/ss/sidtab.c > > @@ -22,16 +22,24 @@ int sidtab_init(struct sidtab *s) > > s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC); > > if (!s->htable) > > return -ENOMEM; > > + > > + for (i = 0; i <= SECINITSID_NUM; i++) > > + s->isids[i].set = 0; > > + > > for (i = 0; i < SIDTAB_SIZE; i++) > > s->htable[i] = NULL; > > + > > + for (i = 0; i < SIDTAB_CACHE_LEN; i++) > > + s->cache[i] = NULL; > > + > > s->nel = 0; > > - s->next_sid = 1; > > + s->next_sid = 0; > > s->shutdown = 0; > > spin_lock_init(&s->lock); > > return 0; > > } > > > > -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > > +static int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > > { > > int hvalue; > > struct sidtab_node *prev, *cur, *newnode; > > @@ -76,34 +84,53 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > > return 0; > > } > > > > -static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > > +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context) > > +{ > > + struct sidtab_isid_entry *entry = &s->isids[sid]; > > + int rc = context_cpy(&entry->context, context); > > + if (rc) > > + return rc; > > + > > + entry->set = 1; > > + return 0; > > +} > > + > > +static struct context *sidtab_lookup(struct sidtab *s, u32 sid) > > { > > int hvalue; > > struct sidtab_node *cur; > > > > - if (!s) > > - return NULL; > > - > > hvalue = SIDTAB_HASH(sid); > > cur = s->htable[hvalue]; > > while (cur && sid > cur->sid) > > cur = cur->next; > > > > - if (force && cur && sid == cur->sid && cur->context.len) > > - return &cur->context; > > + if (!cur || sid != cur->sid) > > + return NULL; > > > > - if (!cur || sid != cur->sid || cur->context.len) { > > - /* Remap invalid SIDs to the unlabeled SID. */ > > - sid = SECINITSID_UNLABELED; > > - hvalue = SIDTAB_HASH(sid); > > - cur = s->htable[hvalue]; > > - while (cur && sid > cur->sid) > > - cur = cur->next; > > - if (!cur || sid != cur->sid) > > - return NULL; > > + return &cur->context; > > +} > > + > > +static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > > +{ > > + struct context *context; > > + struct sidtab_isid_entry *entry; > > + > > + if (!s) > > + return NULL; > > + > > + if (sid > SECINITSID_NUM) { > > + u32 index = sid - (SECINITSID_NUM + 1); > > + context = sidtab_lookup(s, index); > > + } else { > > + entry = &s->isids[sid]; > > + context = entry->set ? &entry->context : NULL; > > } > > + if (context && (!context->len || force)) > > + return context; > > > > - return &cur->context; > > + entry = &s->isids[SECINITSID_UNLABELED]; > > + return entry->set ? &entry->context : NULL; > > } > > > > struct context *sidtab_search(struct sidtab *s, u32 sid) > > @@ -145,11 +172,7 @@ out: > > static int clone_sid(u32 sid, struct context *context, void *arg) > > { > > struct sidtab *s = arg; > > - > > - if (sid > SECINITSID_NUM) > > - return sidtab_insert(s, sid, context); > > - else > > - return 0; > > + return sidtab_insert(s, sid, context); > > } > > > > int sidtab_convert(struct sidtab *s, struct sidtab *news, > > @@ -183,8 +206,8 @@ static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc > > s->cache[0] = n; > > } > > > > -static inline u32 sidtab_search_context(struct sidtab *s, > > - struct context *context) > > +static inline int sidtab_search_context(struct sidtab *s, > > + struct context *context, u32 *sid) > > { > > int i; > > struct sidtab_node *cur; > > @@ -194,15 +217,17 @@ static inline u32 sidtab_search_context(struct sidtab *s, > > while (cur) { > > if (context_cmp(&cur->context, context)) { > > sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); > > - return cur->sid; > > + *sid = cur->sid; > > + return 0; > > } > > cur = cur->next; > > } > > } > > - return 0; > > + return -ENOENT; > > } > > > > -static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > > +static inline int sidtab_search_cache(struct sidtab *s, struct context *context, > > + u32 *sid) > > { > > int i; > > struct sidtab_node *node; > > @@ -210,54 +235,67 @@ static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > > for (i = 0; i < SIDTAB_CACHE_LEN; i++) { > > node = s->cache[i]; > > if (unlikely(!node)) > > - return 0; > > + return -ENOENT; > > if (context_cmp(&node->context, context)) { > > sidtab_update_cache(s, node, i); > > - return node->sid; > > + *sid = node->sid; > > + return 0; > > } > > } > > - return 0; > > + return -ENOENT; > > } > > > > -int sidtab_context_to_sid(struct sidtab *s, > > - struct context *context, > > - u32 *out_sid) > > +static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > > + u32 *sid) > > { > > - u32 sid; > > - int ret = 0; > > + int ret; > > unsigned long flags; > > > > - *out_sid = SECSID_NULL; > > - > > - sid = sidtab_search_cache(s, context); > > - if (!sid) > > - sid = sidtab_search_context(s, context); > > - if (!sid) { > > + ret = sidtab_search_cache(s, context, sid); > > + if (ret) > > + ret = sidtab_search_context(s, context, sid); > > + if (ret) { > > spin_lock_irqsave(&s->lock, flags); > > /* Rescan now that we hold the lock. */ > > - sid = sidtab_search_context(s, context); > > - if (sid) > > + ret = sidtab_search_context(s, context, sid); > > + if (!ret) > > goto unlock_out; > > /* No SID exists for the context. Allocate a new one. */ > > - if (s->next_sid == UINT_MAX || s->shutdown) { > > + if (s->next_sid == (UINT_MAX - SECINITSID_NUM - 1) || s->shutdown) { > > ret = -ENOMEM; > > goto unlock_out; > > } > > - sid = s->next_sid++; > > + *sid = s->next_sid++; > > if (context->len) > > pr_info("SELinux: Context %s is not valid (left unmapped).\n", > > context->str); > > - ret = sidtab_insert(s, sid, context); > > + ret = sidtab_insert(s, *sid, context); > > if (ret) > > s->next_sid--; > > unlock_out: > > spin_unlock_irqrestore(&s->lock, flags); > > } > > > > - if (ret) > > - return ret; > > + return ret; > > +} > > + > > +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid) > > +{ > > + int rc; > > + u32 i; > > + > > + for (i = 0; i <= SECINITSID_NUM; i++) { > > + struct sidtab_isid_entry *entry = &s->isids[i]; > > + if (entry->set && context_cmp(context, &entry->context)) { > > + *sid = i; > > + return 0; > > + } > > + } > > > > - *out_sid = sid; > > + rc = sidtab_reverse_lookup(s, context, sid); > > + if (rc) > > + return rc; > > + *sid += SECINITSID_NUM + 1; > > return 0; > > } > > > > @@ -296,6 +334,11 @@ void sidtab_destroy(struct sidtab *s) > > if (!s) > > return; > > > > + for (i = 0; i <= SECINITSID_NUM; i++) > > + if (s->isids[i].set) > > + context_destroy(&s->isids[i].context); > > + > > + > > for (i = 0; i < SIDTAB_SIZE; i++) { > > cur = s->htable[i]; > > while (cur) { > > @@ -311,18 +354,3 @@ void sidtab_destroy(struct sidtab *s) > > s->nel = 0; > > s->next_sid = 1; > > } > > - > > -void sidtab_set(struct sidtab *dst, struct sidtab *src) > > -{ > > - unsigned long flags; > > - int i; > > - > > - spin_lock_irqsave(&src->lock, flags); > > - dst->htable = src->htable; > > - dst->nel = src->nel; > > - dst->next_sid = src->next_sid; > > - dst->shutdown = 0; > > - for (i = 0; i < SIDTAB_CACHE_LEN; i++) > > - dst->cache[i] = NULL; > > - spin_unlock_irqrestore(&src->lock, flags); > > -} > > diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > > index 26c74fe7afc0..e181db3589bc 100644 > > --- a/security/selinux/ss/sidtab.h > > +++ b/security/selinux/ss/sidtab.h > > @@ -22,6 +22,11 @@ struct sidtab_node { > > > > #define SIDTAB_SIZE SIDTAB_HASH_BUCKETS > > > > +struct sidtab_isid_entry { > > + int set; > > + struct context context; > > +}; > > + > > struct sidtab { > > struct sidtab_node **htable; > > unsigned int nel; /* number of elements */ > > @@ -30,10 +35,12 @@ struct sidtab { > > #define SIDTAB_CACHE_LEN 3 > > struct sidtab_node *cache[SIDTAB_CACHE_LEN]; > > spinlock_t lock; > > + > > + struct sidtab_isid_entry isids[SECINITSID_NUM + 1]; > > }; > > > > int sidtab_init(struct sidtab *s); > > -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context); > > +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context); > > struct context *sidtab_search(struct sidtab *s, u32 sid); > > struct context *sidtab_search_force(struct sidtab *s, u32 sid); > > > > @@ -43,13 +50,10 @@ int sidtab_convert(struct sidtab *s, struct sidtab *news, > > void *args), > > void *args); > > > > -int sidtab_context_to_sid(struct sidtab *s, > > - struct context *context, > > - u32 *sid); > > +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); > > > > void sidtab_hash_eval(struct sidtab *h, char *tag); > > void sidtab_destroy(struct sidtab *s); > > -void sidtab_set(struct sidtab *dst, struct sidtab *src); > > > > #endif /* _SS_SIDTAB_H_ */ > > > > > -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.
On 11/14/18 4:45 AM, Ondrej Mosnacek wrote: > On Tue, Nov 13, 2018 at 10:35 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: >> On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: >>> This patch is non-functional and moves handling of initial SIDs into a >>> separate table. Note that the SIDs stored in the main table are now >>> shifted by SECINITSID_NUM and converted to/from the actual SIDs >>> transparently by helper functions. >> >> When you say "non-functional", you mean it doesn't make any functional >> changes, right? As opposed to leaving the kernel in a broken >> intermediate state until your 3rd patch? > > I mean it *should* be non-functional on its own, unless I made a > mistake. I admit I didn't do very much testing on this patch, but I at > least checked that I can boot the kernel, load a policy and that > reported file contexts make sense. Commonly non-functional means "not working". I think you mean "this patch makes no functional changes". > >> >> I think you need to double check that all references to >> state->ss->sidtab are preceded by a check of state->initialized since it >> could be NULL before first policy load and a system could come up with >> SELinux enabled but never load a policy. > > Well, the original code does not initialize the sidtab field with > sidtab_init() either so state->ss->sidtab.htable would be NULL (or > some garbage pointer) after boot and any use of sidtab would cause a > GPF anyway. Prior to the patch, the sidtab is directly embedded in the selinux_ss, which is static and thus all fields are initialized to 0/NULL. But you could access the sidtab itself without any problem since it was not a pointer. Likewise for the policydb. When you change the sidtab to be a pointer, then you have to deal with the possibility that it will be NULL. So you might be introducing new situations where we would trigger a GPF. > > Looking at the places that reference the sidtab (which are all > highlighted in the patch because of the switch to pointer type in the > struct selinux_ss definition), these don't seem to check for > state->initialized: > - security_port_sid In this case, policydb->ocontexts[OCON_PORT] will be NULL at initialization, so c will be NULL and we will just set *out_sid to SECINITSID_PORT and return without ever accessing the sidtab. > - security_ib_pkey_sid > - security_ib_endport_sid > - security_netif_sid > - security_node_sid These are all similar. > - security_sid_mls_copy, called from: This one checks state->initialized and returns if not set. > - selinux_socket_unix_stream_connect (avc_has_perm is called before) > - selinux_conn_sid, called from: > - selinux_sctp_assoc_request (selinux_policycap_extsockclass is > called before) > - selinux_inet_conn_request > - selinux_ip_postroute (selinux_policycap_netpeer is called before) > - security_net_peersid_resolve, called from: This one should always return before taking the policy rwlock when !state->initialized because you can't have configured network labels without a policy IIUC (so xfrm_sid and/or nlbl_sid should be NULL), or regardless, policydb->mls_enabled will be 0 at this point. > - selinux_skb_peerlbl_sid, called from: > - selinux_socket_sock_rcv_skb (selinux_policycap_netpeer is called before) > - selinux_socket_getpeersec_dgram > - selinux_sctp_assoc_request (again) > - selinux_inet_conn_request (again) > - selinux_inet_conn_established > - selinux_ip_forward (selinux_policycap_netpeer is called before) > - selinux_ip_postroute (again) > - selinux_audit_rule_match This one can't be called without a prior selinux_audit_rule_init, which checks state->initialized. > - security_netlbl_secattr_to_sid, called from: This one checks state->initialized. > - selinux_netlbl_sidlookup_cached, called from: > - selinux_netlbl_skbuff_getsid (netlbl_enabled is called before) > - selinux_netlbl_sock_rcv_skb (netlbl_enabled is called before) > - security_netlbl_sid_to_secattr, called from: Ditto. > - selinux_netlbl_sock_genattr, called from: > - selinux_netlbl_socket_post_create, called from: > - selinux_socket_post_create > - selinux_netlbl_skbuff_setsid, called from: > - selinux_ip_forward (again) > - selinux_netlbl_sctp_assoc_request, called from: > - selinux_sctp_assoc_request (again) > - selinux_netlbl_inet_conn_request, called from: > - selinux_inet_conn_request (again) > > I suppose in some (or all?) of these cases state->initialized might be > implicitly always 1, but at least in these it wasn't immediately > obvious to me. > > Either way, such cases would already be buggy before this patch, since > they would happily access the policy structure (likely hitting some > NULL/invalid pointers) and most likely also dereference the invalid > htable pointer in sidtab. > >> >> Aren't you still wasting a slot in the initial SIDs array, or did I miss >> something? > > Yes, I am, because the original code doesn't seem to guard against > SECSID_NULL being loaded as initial SID into sidtab. I asked in the > other thread whether this is considered a bug, but I didn't get a > reply, so I left it sort of bug-to-bug compatible for now... Anyway, > it doesn't seem to make much sense to keep that behavior so I will > probably just go ahead and add the check to policydb_load_isids() and > shrink the table. You can expect it to be resolved in the final > patchset. Yes, that would be a bug. > >> >>> >>> This change doesn't make much sense on its own, but it simplifies >>> further sidtab overhaul in a succeeding patch. >>> >>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> >>> --- >>> security/selinux/ss/policydb.c | 10 ++- >>> security/selinux/ss/services.c | 88 ++++++++++-------- >>> security/selinux/ss/services.h | 2 +- >>> security/selinux/ss/sidtab.c | 158 +++++++++++++++++++-------------- >>> security/selinux/ss/sidtab.h | 14 +-- >>> 5 files changed, 162 insertions(+), 110 deletions(-) >>> >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c >>> index f4eadd3f7350..21e4bdbcf994 100644 >>> --- a/security/selinux/ss/policydb.c >>> +++ b/security/selinux/ss/policydb.c >>> @@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) >>> if (!c->context[0].user) { >>> pr_err("SELinux: SID %s was never defined.\n", >>> c->u.name); >>> + sidtab_destroy(s); >>> + goto out; >>> + } >>> + if (c->sid[0] > SECINITSID_NUM) { >>> + pr_err("SELinux: Initial SID %s out of range.\n", >>> + c->u.name); >>> + sidtab_destroy(s); >>> goto out; >>> } >>> >>> - rc = sidtab_insert(s, c->sid[0], &c->context[0]); >>> + rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); >>> if (rc) { >>> pr_err("SELinux: unable to load initial SID %s.\n", >>> c->u.name); >>> + sidtab_destroy(s); >>> goto out; >>> } >>> } >>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c >>> index 7337db24a6a8..30170d4c567a 100644 >>> --- a/security/selinux/ss/services.c >>> +++ b/security/selinux/ss/services.c >>> @@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> if (!user) >>> tclass = unmap_class(&state->ss->map, orig_tclass); >>> @@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> rc = -EINVAL; >>> old_context = sidtab_search(sidtab, old_sid); >>> @@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct selinux_state *state, >>> goto allow; >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> scontext = sidtab_search(sidtab, ssid); >>> if (!scontext) { >>> @@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state, >>> goto allow; >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> scontext = sidtab_search(sidtab, ssid); >>> if (!scontext) { >>> @@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state, >>> goto allow; >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> scontext = sidtab_search(sidtab, ssid); >>> if (!scontext) { >>> @@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct selinux_state *state, >>> } >>> read_lock(&state->ss->policy_rwlock); >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> if (force) >>> context = sidtab_search_force(sidtab, sid); >>> else >>> @@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct selinux_state *state, >>> } >>> read_lock(&state->ss->policy_rwlock); >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> rc = string_to_context_struct(policydb, sidtab, scontext2, >>> &context, def_sid); >>> if (rc == -EINVAL && force) { >>> @@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state *state, >>> } >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> scontext = sidtab_search(sidtab, ssid); >>> if (!scontext) { >>> @@ -1925,10 +1925,7 @@ static int convert_context(u32 key, >>> struct user_datum *usrdatum; >>> char *s; >>> u32 len; >>> - int rc = 0; >>> - >>> - if (key <= SECINITSID_NUM) >>> - goto out; >>> + int rc; >>> >>> args = p; >>> >>> @@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state *state, >>> int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> { >>> struct policydb *policydb; >>> - struct sidtab *sidtab; >>> + struct sidtab *oldsidtab, *newsidtab; >>> struct policydb *oldpolicydb, *newpolicydb; >>> - struct sidtab oldsidtab, newsidtab; >>> struct selinux_mapping *oldmapping; >>> struct selinux_map newmap; >>> struct convert_context_args args; >>> @@ -2108,27 +2104,37 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> newpolicydb = oldpolicydb + 1; >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + >>> + newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL); >>> + if (!newsidtab) { >>> + rc = -ENOMEM; >>> + goto out; >>> + } >>> >>> if (!state->initialized) { >>> rc = policydb_read(policydb, fp); >>> - if (rc) >>> + if (rc) { >>> + kfree(newsidtab); >>> goto out; >>> + } >>> >>> policydb->len = len; >>> rc = selinux_set_mapping(policydb, secclass_map, >>> &state->ss->map); >>> if (rc) { >>> + kfree(newsidtab); >>> policydb_destroy(policydb); >>> goto out; >>> } >>> >>> - rc = policydb_load_isids(policydb, sidtab); >>> + rc = policydb_load_isids(policydb, newsidtab); >>> if (rc) { >>> + kfree(newsidtab); >>> policydb_destroy(policydb); >>> goto out; >>> } >>> >>> + state->ss->sidtab = newsidtab; >>> security_load_policycaps(state); >>> state->initialized = 1; >>> seqno = ++state->ss->latest_granting; >>> @@ -2141,13 +2147,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> goto out; >>> } >>> >>> + oldsidtab = state->ss->sidtab; >>> + >>> #if 0 >>> - sidtab_hash_eval(sidtab, "sids"); >>> + sidtab_hash_eval(oldsidtab, "sids"); >>> #endif >>> >>> rc = policydb_read(newpolicydb, fp); >>> - if (rc) >>> + if (rc) { >>> + kfree(newsidtab); >>> goto out; >>> + } >>> >>> newpolicydb->len = len; >>> /* If switching between different policy types, log MLS status */ >>> @@ -2156,10 +2166,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> else if (!policydb->mls_enabled && newpolicydb->mls_enabled) >>> pr_info("SELinux: Enabling MLS support...\n"); >>> >>> - rc = policydb_load_isids(newpolicydb, &newsidtab); >>> + rc = policydb_load_isids(newpolicydb, newsidtab); >>> if (rc) { >>> pr_err("SELinux: unable to load the initial SIDs\n"); >>> policydb_destroy(newpolicydb); >>> + kfree(newsidtab); >>> goto out; >>> } >>> >>> @@ -2180,7 +2191,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> args.state = state; >>> args.oldp = policydb; >>> args.newp = newpolicydb; >>> - rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args); >>> + rc = sidtab_convert(oldsidtab, newsidtab, convert_context, &args); >>> if (rc) { >>> pr_err("SELinux: unable to convert the internal" >>> " representation of contexts in the new SID" >>> @@ -2190,12 +2201,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> >>> /* Save the old policydb and SID table to free later. */ >>> memcpy(oldpolicydb, policydb, sizeof(*policydb)); >>> - sidtab_set(&oldsidtab, sidtab); >>> >>> /* Install the new policydb and SID table. */ >>> write_lock_irq(&state->ss->policy_rwlock); >>> memcpy(policydb, newpolicydb, sizeof(*policydb)); >>> - sidtab_set(sidtab, &newsidtab); >>> + state->ss->sidtab = newsidtab; >>> security_load_policycaps(state); >>> oldmapping = state->ss->map.mapping; >>> state->ss->map.mapping = newmap.mapping; >>> @@ -2205,7 +2215,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> >>> /* Free the old policydb and SID table. */ >>> policydb_destroy(oldpolicydb); >>> - sidtab_destroy(&oldsidtab); >>> + sidtab_destroy(oldsidtab); >>> + kfree(oldsidtab); >>> kfree(oldmapping); >>> >>> avc_ss_reset(state->avc, seqno); >>> @@ -2219,7 +2230,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) >>> >>> err: >>> kfree(newmap.mapping); >>> - sidtab_destroy(&newsidtab); >>> + sidtab_destroy(newsidtab); >>> + kfree(newsidtab); >>> policydb_destroy(newpolicydb); >>> >>> out: >>> @@ -2256,7 +2268,7 @@ int security_port_sid(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> c = policydb->ocontexts[OCON_PORT]; >>> while (c) { >>> @@ -2302,7 +2314,7 @@ int security_ib_pkey_sid(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> c = policydb->ocontexts[OCON_IBPKEY]; >>> while (c) { >>> @@ -2348,7 +2360,7 @@ int security_ib_endport_sid(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> c = policydb->ocontexts[OCON_IBENDPORT]; >>> while (c) { >>> @@ -2394,7 +2406,7 @@ int security_netif_sid(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> c = policydb->ocontexts[OCON_NETIF]; >>> while (c) { >>> @@ -2459,7 +2471,7 @@ int security_node_sid(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> switch (domain) { >>> case AF_INET: { >>> @@ -2559,7 +2571,7 @@ int security_get_user_sids(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> context_init(&usercon); >>> >>> @@ -2661,7 +2673,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, >>> u32 *sid) >>> { >>> struct policydb *policydb = &state->ss->policydb; >>> - struct sidtab *sidtab = &state->ss->sidtab; >>> + struct sidtab *sidtab = state->ss->sidtab; >>> int len; >>> u16 sclass; >>> struct genfs *genfs; >>> @@ -2747,7 +2759,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) >>> read_lock(&state->ss->policy_rwlock); >>> >>> policydb = &state->ss->policydb; >>> - sidtab = &state->ss->sidtab; >>> + sidtab = state->ss->sidtab; >>> >>> c = policydb->ocontexts[OCON_FSUSE]; >>> while (c) { >>> @@ -2953,7 +2965,7 @@ int security_sid_mls_copy(struct selinux_state *state, >>> u32 sid, u32 mls_sid, u32 *new_sid) >>> { >>> struct policydb *policydb = &state->ss->policydb; >>> - struct sidtab *sidtab = &state->ss->sidtab; >>> + struct sidtab *sidtab = state->ss->sidtab; >>> struct context *context1; >>> struct context *context2; >>> struct context newcon; >>> @@ -3044,7 +3056,7 @@ int security_net_peersid_resolve(struct selinux_state *state, >>> u32 *peer_sid) >>> { >>> struct policydb *policydb = &state->ss->policydb; >>> - struct sidtab *sidtab = &state->ss->sidtab; >>> + struct sidtab *sidtab = state->ss->sidtab; >>> int rc; >>> struct context *nlbl_ctx; >>> struct context *xfrm_ctx; >>> @@ -3405,7 +3417,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, >>> goto out; >>> } >>> >>> - ctxt = sidtab_search(&state->ss->sidtab, sid); >>> + ctxt = sidtab_search(state->ss->sidtab, sid); >>> if (unlikely(!ctxt)) { >>> WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n", >>> sid); >>> @@ -3568,7 +3580,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state, >>> u32 *sid) >>> { >>> struct policydb *policydb = &state->ss->policydb; >>> - struct sidtab *sidtab = &state->ss->sidtab; >>> + struct sidtab *sidtab = state->ss->sidtab; >>> int rc; >>> struct context *ctx; >>> struct context ctx_new; >>> @@ -3646,7 +3658,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, >>> read_lock(&state->ss->policy_rwlock); >>> >>> rc = -ENOENT; >>> - ctx = sidtab_search(&state->ss->sidtab, sid); >>> + ctx = sidtab_search(state->ss->sidtab, sid); >>> if (ctx == NULL) >>> goto out; >>> >>> diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h >>> index 24c7bdcc8075..9a36de860368 100644 >>> --- a/security/selinux/ss/services.h >>> +++ b/security/selinux/ss/services.h >>> @@ -24,7 +24,7 @@ struct selinux_map { >>> }; >>> >>> struct selinux_ss { >>> - struct sidtab sidtab; >>> + struct sidtab *sidtab; >>> struct policydb policydb; >>> rwlock_t policy_rwlock; >>> u32 latest_granting; >>> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c >>> index e66a2ab3d1c2..049ac1e6fd06 100644 >>> --- a/security/selinux/ss/sidtab.c >>> +++ b/security/selinux/ss/sidtab.c >>> @@ -22,16 +22,24 @@ int sidtab_init(struct sidtab *s) >>> s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC); >>> if (!s->htable) >>> return -ENOMEM; >>> + >>> + for (i = 0; i <= SECINITSID_NUM; i++) >>> + s->isids[i].set = 0; >>> + >>> for (i = 0; i < SIDTAB_SIZE; i++) >>> s->htable[i] = NULL; >>> + >>> + for (i = 0; i < SIDTAB_CACHE_LEN; i++) >>> + s->cache[i] = NULL; >>> + >>> s->nel = 0; >>> - s->next_sid = 1; >>> + s->next_sid = 0; >>> s->shutdown = 0; >>> spin_lock_init(&s->lock); >>> return 0; >>> } >>> >>> -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) >>> +static int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) >>> { >>> int hvalue; >>> struct sidtab_node *prev, *cur, *newnode; >>> @@ -76,34 +84,53 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) >>> return 0; >>> } >>> >>> -static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) >>> +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context) >>> +{ >>> + struct sidtab_isid_entry *entry = &s->isids[sid]; >>> + int rc = context_cpy(&entry->context, context); >>> + if (rc) >>> + return rc; >>> + >>> + entry->set = 1; >>> + return 0; >>> +} >>> + >>> +static struct context *sidtab_lookup(struct sidtab *s, u32 sid) >>> { >>> int hvalue; >>> struct sidtab_node *cur; >>> >>> - if (!s) >>> - return NULL; >>> - >>> hvalue = SIDTAB_HASH(sid); >>> cur = s->htable[hvalue]; >>> while (cur && sid > cur->sid) >>> cur = cur->next; >>> >>> - if (force && cur && sid == cur->sid && cur->context.len) >>> - return &cur->context; >>> + if (!cur || sid != cur->sid) >>> + return NULL; >>> >>> - if (!cur || sid != cur->sid || cur->context.len) { >>> - /* Remap invalid SIDs to the unlabeled SID. */ >>> - sid = SECINITSID_UNLABELED; >>> - hvalue = SIDTAB_HASH(sid); >>> - cur = s->htable[hvalue]; >>> - while (cur && sid > cur->sid) >>> - cur = cur->next; >>> - if (!cur || sid != cur->sid) >>> - return NULL; >>> + return &cur->context; >>> +} >>> + >>> +static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) >>> +{ >>> + struct context *context; >>> + struct sidtab_isid_entry *entry; >>> + >>> + if (!s) >>> + return NULL; >>> + >>> + if (sid > SECINITSID_NUM) { >>> + u32 index = sid - (SECINITSID_NUM + 1); >>> + context = sidtab_lookup(s, index); >>> + } else { >>> + entry = &s->isids[sid]; >>> + context = entry->set ? &entry->context : NULL; >>> } >>> + if (context && (!context->len || force)) >>> + return context; >>> >>> - return &cur->context; >>> + entry = &s->isids[SECINITSID_UNLABELED]; >>> + return entry->set ? &entry->context : NULL; >>> } >>> >>> struct context *sidtab_search(struct sidtab *s, u32 sid) >>> @@ -145,11 +172,7 @@ out: >>> static int clone_sid(u32 sid, struct context *context, void *arg) >>> { >>> struct sidtab *s = arg; >>> - >>> - if (sid > SECINITSID_NUM) >>> - return sidtab_insert(s, sid, context); >>> - else >>> - return 0; >>> + return sidtab_insert(s, sid, context); >>> } >>> >>> int sidtab_convert(struct sidtab *s, struct sidtab *news, >>> @@ -183,8 +206,8 @@ static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc >>> s->cache[0] = n; >>> } >>> >>> -static inline u32 sidtab_search_context(struct sidtab *s, >>> - struct context *context) >>> +static inline int sidtab_search_context(struct sidtab *s, >>> + struct context *context, u32 *sid) >>> { >>> int i; >>> struct sidtab_node *cur; >>> @@ -194,15 +217,17 @@ static inline u32 sidtab_search_context(struct sidtab *s, >>> while (cur) { >>> if (context_cmp(&cur->context, context)) { >>> sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); >>> - return cur->sid; >>> + *sid = cur->sid; >>> + return 0; >>> } >>> cur = cur->next; >>> } >>> } >>> - return 0; >>> + return -ENOENT; >>> } >>> >>> -static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) >>> +static inline int sidtab_search_cache(struct sidtab *s, struct context *context, >>> + u32 *sid) >>> { >>> int i; >>> struct sidtab_node *node; >>> @@ -210,54 +235,67 @@ static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) >>> for (i = 0; i < SIDTAB_CACHE_LEN; i++) { >>> node = s->cache[i]; >>> if (unlikely(!node)) >>> - return 0; >>> + return -ENOENT; >>> if (context_cmp(&node->context, context)) { >>> sidtab_update_cache(s, node, i); >>> - return node->sid; >>> + *sid = node->sid; >>> + return 0; >>> } >>> } >>> - return 0; >>> + return -ENOENT; >>> } >>> >>> -int sidtab_context_to_sid(struct sidtab *s, >>> - struct context *context, >>> - u32 *out_sid) >>> +static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, >>> + u32 *sid) >>> { >>> - u32 sid; >>> - int ret = 0; >>> + int ret; >>> unsigned long flags; >>> >>> - *out_sid = SECSID_NULL; >>> - >>> - sid = sidtab_search_cache(s, context); >>> - if (!sid) >>> - sid = sidtab_search_context(s, context); >>> - if (!sid) { >>> + ret = sidtab_search_cache(s, context, sid); >>> + if (ret) >>> + ret = sidtab_search_context(s, context, sid); >>> + if (ret) { >>> spin_lock_irqsave(&s->lock, flags); >>> /* Rescan now that we hold the lock. */ >>> - sid = sidtab_search_context(s, context); >>> - if (sid) >>> + ret = sidtab_search_context(s, context, sid); >>> + if (!ret) >>> goto unlock_out; >>> /* No SID exists for the context. Allocate a new one. */ >>> - if (s->next_sid == UINT_MAX || s->shutdown) { >>> + if (s->next_sid == (UINT_MAX - SECINITSID_NUM - 1) || s->shutdown) { >>> ret = -ENOMEM; >>> goto unlock_out; >>> } >>> - sid = s->next_sid++; >>> + *sid = s->next_sid++; >>> if (context->len) >>> pr_info("SELinux: Context %s is not valid (left unmapped).\n", >>> context->str); >>> - ret = sidtab_insert(s, sid, context); >>> + ret = sidtab_insert(s, *sid, context); >>> if (ret) >>> s->next_sid--; >>> unlock_out: >>> spin_unlock_irqrestore(&s->lock, flags); >>> } >>> >>> - if (ret) >>> - return ret; >>> + return ret; >>> +} >>> + >>> +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid) >>> +{ >>> + int rc; >>> + u32 i; >>> + >>> + for (i = 0; i <= SECINITSID_NUM; i++) { >>> + struct sidtab_isid_entry *entry = &s->isids[i]; >>> + if (entry->set && context_cmp(context, &entry->context)) { >>> + *sid = i; >>> + return 0; >>> + } >>> + } >>> >>> - *out_sid = sid; >>> + rc = sidtab_reverse_lookup(s, context, sid); >>> + if (rc) >>> + return rc; >>> + *sid += SECINITSID_NUM + 1; >>> return 0; >>> } >>> >>> @@ -296,6 +334,11 @@ void sidtab_destroy(struct sidtab *s) >>> if (!s) >>> return; >>> >>> + for (i = 0; i <= SECINITSID_NUM; i++) >>> + if (s->isids[i].set) >>> + context_destroy(&s->isids[i].context); >>> + >>> + >>> for (i = 0; i < SIDTAB_SIZE; i++) { >>> cur = s->htable[i]; >>> while (cur) { >>> @@ -311,18 +354,3 @@ void sidtab_destroy(struct sidtab *s) >>> s->nel = 0; >>> s->next_sid = 1; >>> } >>> - >>> -void sidtab_set(struct sidtab *dst, struct sidtab *src) >>> -{ >>> - unsigned long flags; >>> - int i; >>> - >>> - spin_lock_irqsave(&src->lock, flags); >>> - dst->htable = src->htable; >>> - dst->nel = src->nel; >>> - dst->next_sid = src->next_sid; >>> - dst->shutdown = 0; >>> - for (i = 0; i < SIDTAB_CACHE_LEN; i++) >>> - dst->cache[i] = NULL; >>> - spin_unlock_irqrestore(&src->lock, flags); >>> -} >>> diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h >>> index 26c74fe7afc0..e181db3589bc 100644 >>> --- a/security/selinux/ss/sidtab.h >>> +++ b/security/selinux/ss/sidtab.h >>> @@ -22,6 +22,11 @@ struct sidtab_node { >>> >>> #define SIDTAB_SIZE SIDTAB_HASH_BUCKETS >>> >>> +struct sidtab_isid_entry { >>> + int set; >>> + struct context context; >>> +}; >>> + >>> struct sidtab { >>> struct sidtab_node **htable; >>> unsigned int nel; /* number of elements */ >>> @@ -30,10 +35,12 @@ struct sidtab { >>> #define SIDTAB_CACHE_LEN 3 >>> struct sidtab_node *cache[SIDTAB_CACHE_LEN]; >>> spinlock_t lock; >>> + >>> + struct sidtab_isid_entry isids[SECINITSID_NUM + 1]; >>> }; >>> >>> int sidtab_init(struct sidtab *s); >>> -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context); >>> +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context); >>> struct context *sidtab_search(struct sidtab *s, u32 sid); >>> struct context *sidtab_search_force(struct sidtab *s, u32 sid); >>> >>> @@ -43,13 +50,10 @@ int sidtab_convert(struct sidtab *s, struct sidtab *news, >>> void *args), >>> void *args); >>> >>> -int sidtab_context_to_sid(struct sidtab *s, >>> - struct context *context, >>> - u32 *sid); >>> +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); >>> >>> void sidtab_hash_eval(struct sidtab *h, char *tag); >>> void sidtab_destroy(struct sidtab *s); >>> -void sidtab_set(struct sidtab *dst, struct sidtab *src); >>> >>> #endif /* _SS_SIDTAB_H_ */ >>> >>> >> > > -- > Ondrej Mosnacek <omosnace at redhat dot com> > Associate Software Engineer, Security Technologies > Red Hat, Inc. >
On Wed, Nov 14, 2018 at 3:08 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 11/14/18 4:45 AM, Ondrej Mosnacek wrote: > > On Tue, Nov 13, 2018 at 10:35 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > >> On 11/13/18 8:52 AM, Ondrej Mosnacek wrote: > >>> This patch is non-functional and moves handling of initial SIDs into a > >>> separate table. Note that the SIDs stored in the main table are now > >>> shifted by SECINITSID_NUM and converted to/from the actual SIDs > >>> transparently by helper functions. > >> > >> When you say "non-functional", you mean it doesn't make any functional > >> changes, right? As opposed to leaving the kernel in a broken > >> intermediate state until your 3rd patch? > > > > I mean it *should* be non-functional on its own, unless I made a > > mistake. I admit I didn't do very much testing on this patch, but I at > > least checked that I can boot the kernel, load a policy and that > > reported file contexts make sense. > > Commonly non-functional means "not working". I think you mean "this > patch makes no functional changes". Ah, I see, so it was just a language issue :) Indeed I meant the latter. > > > > >> > >> I think you need to double check that all references to > >> state->ss->sidtab are preceded by a check of state->initialized since it > >> could be NULL before first policy load and a system could come up with > >> SELinux enabled but never load a policy. > > > > Well, the original code does not initialize the sidtab field with > > sidtab_init() either so state->ss->sidtab.htable would be NULL (or > > some garbage pointer) after boot and any use of sidtab would cause a > > GPF anyway. > > Prior to the patch, the sidtab is directly embedded in the selinux_ss, > which is static and thus all fields are initialized to 0/NULL. But you > could access the sidtab itself without any problem since it was not a > pointer. Likewise for the policydb. When you change the sidtab to be a > pointer, then you have to deal with the possibility that it will be > NULL. So you might be introducing new situations where we would trigger > a GPF. My point was that even when the sidtab is embedded in selinux_ss, its htable field is a pointer and any sidtab_search[_force]()/sidtab_context_to_sid() calls on an uninitialized sidtab would still trigger a NULL pointer dereference. And outside the policy load code, these are the only functions that are ever used to access the sidtab. > > > > > Looking at the places that reference the sidtab (which are all > > highlighted in the patch because of the switch to pointer type in the > > struct selinux_ss definition), these don't seem to check for > > state->initialized: > > - security_port_sid > > In this case, policydb->ocontexts[OCON_PORT] will be NULL at > initialization, so c will be NULL and we will just set *out_sid to > SECINITSID_PORT and return without ever accessing the sidtab. > > > - security_ib_pkey_sid > > - security_ib_endport_sid > > - security_netif_sid > > - security_node_sid > > These are all similar. > > > - security_sid_mls_copy, called from: > > This one checks state->initialized and returns if not set. > > > - selinux_socket_unix_stream_connect (avc_has_perm is called before) > > - selinux_conn_sid, called from: > > - selinux_sctp_assoc_request (selinux_policycap_extsockclass is > > called before) > > - selinux_inet_conn_request > > - selinux_ip_postroute (selinux_policycap_netpeer is called before) > > - security_net_peersid_resolve, called from: > > This one should always return before taking the policy rwlock when > !state->initialized because you can't have configured network labels > without a policy IIUC (so xfrm_sid and/or nlbl_sid should be NULL), or > regardless, policydb->mls_enabled will be 0 at this point. > > > - selinux_skb_peerlbl_sid, called from: > > - selinux_socket_sock_rcv_skb (selinux_policycap_netpeer is called before) > > - selinux_socket_getpeersec_dgram > > - selinux_sctp_assoc_request (again) > > - selinux_inet_conn_request (again) > > - selinux_inet_conn_established > > - selinux_ip_forward (selinux_policycap_netpeer is called before) > > - selinux_ip_postroute (again) > > - selinux_audit_rule_match > > This one can't be called without a prior selinux_audit_rule_init, which > checks state->initialized. > > > - security_netlbl_secattr_to_sid, called from: > > This one checks state->initialized. > > > - selinux_netlbl_sidlookup_cached, called from: > > - selinux_netlbl_skbuff_getsid (netlbl_enabled is called before) > > - selinux_netlbl_sock_rcv_skb (netlbl_enabled is called before) > > - security_netlbl_sid_to_secattr, called from: > > Ditto. > > > - selinux_netlbl_sock_genattr, called from: > > - selinux_netlbl_socket_post_create, called from: > > - selinux_socket_post_create > > - selinux_netlbl_skbuff_setsid, called from: > > - selinux_ip_forward (again) > > - selinux_netlbl_sctp_assoc_request, called from: > > - selinux_sctp_assoc_request (again) > > - selinux_netlbl_inet_conn_request, called from: > > - selinux_inet_conn_request (again) > > > > I suppose in some (or all?) of these cases state->initialized might be > > implicitly always 1, but at least in these it wasn't immediately > > obvious to me. > > > > Either way, such cases would already be buggy before this patch, since > > they would happily access the policy structure (likely hitting some > > NULL/invalid pointers) and most likely also dereference the invalid > > htable pointer in sidtab. > > > >> > >> Aren't you still wasting a slot in the initial SIDs array, or did I miss > >> something? > > > > Yes, I am, because the original code doesn't seem to guard against > > SECSID_NULL being loaded as initial SID into sidtab. I asked in the > > other thread whether this is considered a bug, but I didn't get a > > reply, so I left it sort of bug-to-bug compatible for now... Anyway, > > it doesn't seem to make much sense to keep that behavior so I will > > probably just go ahead and add the check to policydb_load_isids() and > > shrink the table. You can expect it to be resolved in the final > > patchset. > > Yes, that would be a bug. > > > > >> > >>> > >>> This change doesn't make much sense on its own, but it simplifies > >>> further sidtab overhaul in a succeeding patch. > >>> > >>> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > >>> --- > >>> security/selinux/ss/policydb.c | 10 ++- > >>> security/selinux/ss/services.c | 88 ++++++++++-------- > >>> security/selinux/ss/services.h | 2 +- > >>> security/selinux/ss/sidtab.c | 158 +++++++++++++++++++-------------- > >>> security/selinux/ss/sidtab.h | 14 +-- > >>> 5 files changed, 162 insertions(+), 110 deletions(-) > >>> > >>> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c > >>> index f4eadd3f7350..21e4bdbcf994 100644 > >>> --- a/security/selinux/ss/policydb.c > >>> +++ b/security/selinux/ss/policydb.c > >>> @@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) > >>> if (!c->context[0].user) { > >>> pr_err("SELinux: SID %s was never defined.\n", > >>> c->u.name); > >>> + sidtab_destroy(s); > >>> + goto out; > >>> + } > >>> + if (c->sid[0] > SECINITSID_NUM) { > >>> + pr_err("SELinux: Initial SID %s out of range.\n", > >>> + c->u.name); > >>> + sidtab_destroy(s); > >>> goto out; > >>> } > >>> > >>> - rc = sidtab_insert(s, c->sid[0], &c->context[0]); > >>> + rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); > >>> if (rc) { > >>> pr_err("SELinux: unable to load initial SID %s.\n", > >>> c->u.name); > >>> + sidtab_destroy(s); > >>> goto out; > >>> } > >>> } > >>> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > >>> index 7337db24a6a8..30170d4c567a 100644 > >>> --- a/security/selinux/ss/services.c > >>> +++ b/security/selinux/ss/services.c > >>> @@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> if (!user) > >>> tclass = unmap_class(&state->ss->map, orig_tclass); > >>> @@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> rc = -EINVAL; > >>> old_context = sidtab_search(sidtab, old_sid); > >>> @@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct selinux_state *state, > >>> goto allow; > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> scontext = sidtab_search(sidtab, ssid); > >>> if (!scontext) { > >>> @@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state, > >>> goto allow; > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> scontext = sidtab_search(sidtab, ssid); > >>> if (!scontext) { > >>> @@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state, > >>> goto allow; > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> scontext = sidtab_search(sidtab, ssid); > >>> if (!scontext) { > >>> @@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct selinux_state *state, > >>> } > >>> read_lock(&state->ss->policy_rwlock); > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> if (force) > >>> context = sidtab_search_force(sidtab, sid); > >>> else > >>> @@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct selinux_state *state, > >>> } > >>> read_lock(&state->ss->policy_rwlock); > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> rc = string_to_context_struct(policydb, sidtab, scontext2, > >>> &context, def_sid); > >>> if (rc == -EINVAL && force) { > >>> @@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state *state, > >>> } > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> scontext = sidtab_search(sidtab, ssid); > >>> if (!scontext) { > >>> @@ -1925,10 +1925,7 @@ static int convert_context(u32 key, > >>> struct user_datum *usrdatum; > >>> char *s; > >>> u32 len; > >>> - int rc = 0; > >>> - > >>> - if (key <= SECINITSID_NUM) > >>> - goto out; > >>> + int rc; > >>> > >>> args = p; > >>> > >>> @@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state *state, > >>> int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> { > >>> struct policydb *policydb; > >>> - struct sidtab *sidtab; > >>> + struct sidtab *oldsidtab, *newsidtab; > >>> struct policydb *oldpolicydb, *newpolicydb; > >>> - struct sidtab oldsidtab, newsidtab; > >>> struct selinux_mapping *oldmapping; > >>> struct selinux_map newmap; > >>> struct convert_context_args args; > >>> @@ -2108,27 +2104,37 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> newpolicydb = oldpolicydb + 1; > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + > >>> + newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL); > >>> + if (!newsidtab) { > >>> + rc = -ENOMEM; > >>> + goto out; > >>> + } > >>> > >>> if (!state->initialized) { > >>> rc = policydb_read(policydb, fp); > >>> - if (rc) > >>> + if (rc) { > >>> + kfree(newsidtab); > >>> goto out; > >>> + } > >>> > >>> policydb->len = len; > >>> rc = selinux_set_mapping(policydb, secclass_map, > >>> &state->ss->map); > >>> if (rc) { > >>> + kfree(newsidtab); > >>> policydb_destroy(policydb); > >>> goto out; > >>> } > >>> > >>> - rc = policydb_load_isids(policydb, sidtab); > >>> + rc = policydb_load_isids(policydb, newsidtab); > >>> if (rc) { > >>> + kfree(newsidtab); > >>> policydb_destroy(policydb); > >>> goto out; > >>> } > >>> > >>> + state->ss->sidtab = newsidtab; > >>> security_load_policycaps(state); > >>> state->initialized = 1; > >>> seqno = ++state->ss->latest_granting; > >>> @@ -2141,13 +2147,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> goto out; > >>> } > >>> > >>> + oldsidtab = state->ss->sidtab; > >>> + > >>> #if 0 > >>> - sidtab_hash_eval(sidtab, "sids"); > >>> + sidtab_hash_eval(oldsidtab, "sids"); > >>> #endif > >>> > >>> rc = policydb_read(newpolicydb, fp); > >>> - if (rc) > >>> + if (rc) { > >>> + kfree(newsidtab); > >>> goto out; > >>> + } > >>> > >>> newpolicydb->len = len; > >>> /* If switching between different policy types, log MLS status */ > >>> @@ -2156,10 +2166,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> else if (!policydb->mls_enabled && newpolicydb->mls_enabled) > >>> pr_info("SELinux: Enabling MLS support...\n"); > >>> > >>> - rc = policydb_load_isids(newpolicydb, &newsidtab); > >>> + rc = policydb_load_isids(newpolicydb, newsidtab); > >>> if (rc) { > >>> pr_err("SELinux: unable to load the initial SIDs\n"); > >>> policydb_destroy(newpolicydb); > >>> + kfree(newsidtab); > >>> goto out; > >>> } > >>> > >>> @@ -2180,7 +2191,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> args.state = state; > >>> args.oldp = policydb; > >>> args.newp = newpolicydb; > >>> - rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args); > >>> + rc = sidtab_convert(oldsidtab, newsidtab, convert_context, &args); > >>> if (rc) { > >>> pr_err("SELinux: unable to convert the internal" > >>> " representation of contexts in the new SID" > >>> @@ -2190,12 +2201,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> > >>> /* Save the old policydb and SID table to free later. */ > >>> memcpy(oldpolicydb, policydb, sizeof(*policydb)); > >>> - sidtab_set(&oldsidtab, sidtab); > >>> > >>> /* Install the new policydb and SID table. */ > >>> write_lock_irq(&state->ss->policy_rwlock); > >>> memcpy(policydb, newpolicydb, sizeof(*policydb)); > >>> - sidtab_set(sidtab, &newsidtab); > >>> + state->ss->sidtab = newsidtab; > >>> security_load_policycaps(state); > >>> oldmapping = state->ss->map.mapping; > >>> state->ss->map.mapping = newmap.mapping; > >>> @@ -2205,7 +2215,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> > >>> /* Free the old policydb and SID table. */ > >>> policydb_destroy(oldpolicydb); > >>> - sidtab_destroy(&oldsidtab); > >>> + sidtab_destroy(oldsidtab); > >>> + kfree(oldsidtab); > >>> kfree(oldmapping); > >>> > >>> avc_ss_reset(state->avc, seqno); > >>> @@ -2219,7 +2230,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > >>> > >>> err: > >>> kfree(newmap.mapping); > >>> - sidtab_destroy(&newsidtab); > >>> + sidtab_destroy(newsidtab); > >>> + kfree(newsidtab); > >>> policydb_destroy(newpolicydb); > >>> > >>> out: > >>> @@ -2256,7 +2268,7 @@ int security_port_sid(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> c = policydb->ocontexts[OCON_PORT]; > >>> while (c) { > >>> @@ -2302,7 +2314,7 @@ int security_ib_pkey_sid(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> c = policydb->ocontexts[OCON_IBPKEY]; > >>> while (c) { > >>> @@ -2348,7 +2360,7 @@ int security_ib_endport_sid(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> c = policydb->ocontexts[OCON_IBENDPORT]; > >>> while (c) { > >>> @@ -2394,7 +2406,7 @@ int security_netif_sid(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> c = policydb->ocontexts[OCON_NETIF]; > >>> while (c) { > >>> @@ -2459,7 +2471,7 @@ int security_node_sid(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> switch (domain) { > >>> case AF_INET: { > >>> @@ -2559,7 +2571,7 @@ int security_get_user_sids(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> context_init(&usercon); > >>> > >>> @@ -2661,7 +2673,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, > >>> u32 *sid) > >>> { > >>> struct policydb *policydb = &state->ss->policydb; > >>> - struct sidtab *sidtab = &state->ss->sidtab; > >>> + struct sidtab *sidtab = state->ss->sidtab; > >>> int len; > >>> u16 sclass; > >>> struct genfs *genfs; > >>> @@ -2747,7 +2759,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> policydb = &state->ss->policydb; > >>> - sidtab = &state->ss->sidtab; > >>> + sidtab = state->ss->sidtab; > >>> > >>> c = policydb->ocontexts[OCON_FSUSE]; > >>> while (c) { > >>> @@ -2953,7 +2965,7 @@ int security_sid_mls_copy(struct selinux_state *state, > >>> u32 sid, u32 mls_sid, u32 *new_sid) > >>> { > >>> struct policydb *policydb = &state->ss->policydb; > >>> - struct sidtab *sidtab = &state->ss->sidtab; > >>> + struct sidtab *sidtab = state->ss->sidtab; > >>> struct context *context1; > >>> struct context *context2; > >>> struct context newcon; > >>> @@ -3044,7 +3056,7 @@ int security_net_peersid_resolve(struct selinux_state *state, > >>> u32 *peer_sid) > >>> { > >>> struct policydb *policydb = &state->ss->policydb; > >>> - struct sidtab *sidtab = &state->ss->sidtab; > >>> + struct sidtab *sidtab = state->ss->sidtab; > >>> int rc; > >>> struct context *nlbl_ctx; > >>> struct context *xfrm_ctx; > >>> @@ -3405,7 +3417,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, > >>> goto out; > >>> } > >>> > >>> - ctxt = sidtab_search(&state->ss->sidtab, sid); > >>> + ctxt = sidtab_search(state->ss->sidtab, sid); > >>> if (unlikely(!ctxt)) { > >>> WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n", > >>> sid); > >>> @@ -3568,7 +3580,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state, > >>> u32 *sid) > >>> { > >>> struct policydb *policydb = &state->ss->policydb; > >>> - struct sidtab *sidtab = &state->ss->sidtab; > >>> + struct sidtab *sidtab = state->ss->sidtab; > >>> int rc; > >>> struct context *ctx; > >>> struct context ctx_new; > >>> @@ -3646,7 +3658,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, > >>> read_lock(&state->ss->policy_rwlock); > >>> > >>> rc = -ENOENT; > >>> - ctx = sidtab_search(&state->ss->sidtab, sid); > >>> + ctx = sidtab_search(state->ss->sidtab, sid); > >>> if (ctx == NULL) > >>> goto out; > >>> > >>> diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h > >>> index 24c7bdcc8075..9a36de860368 100644 > >>> --- a/security/selinux/ss/services.h > >>> +++ b/security/selinux/ss/services.h > >>> @@ -24,7 +24,7 @@ struct selinux_map { > >>> }; > >>> > >>> struct selinux_ss { > >>> - struct sidtab sidtab; > >>> + struct sidtab *sidtab; > >>> struct policydb policydb; > >>> rwlock_t policy_rwlock; > >>> u32 latest_granting; > >>> diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c > >>> index e66a2ab3d1c2..049ac1e6fd06 100644 > >>> --- a/security/selinux/ss/sidtab.c > >>> +++ b/security/selinux/ss/sidtab.c > >>> @@ -22,16 +22,24 @@ int sidtab_init(struct sidtab *s) > >>> s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC); > >>> if (!s->htable) > >>> return -ENOMEM; > >>> + > >>> + for (i = 0; i <= SECINITSID_NUM; i++) > >>> + s->isids[i].set = 0; > >>> + > >>> for (i = 0; i < SIDTAB_SIZE; i++) > >>> s->htable[i] = NULL; > >>> + > >>> + for (i = 0; i < SIDTAB_CACHE_LEN; i++) > >>> + s->cache[i] = NULL; > >>> + > >>> s->nel = 0; > >>> - s->next_sid = 1; > >>> + s->next_sid = 0; > >>> s->shutdown = 0; > >>> spin_lock_init(&s->lock); > >>> return 0; > >>> } > >>> > >>> -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > >>> +static int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > >>> { > >>> int hvalue; > >>> struct sidtab_node *prev, *cur, *newnode; > >>> @@ -76,34 +84,53 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) > >>> return 0; > >>> } > >>> > >>> -static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > >>> +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context) > >>> +{ > >>> + struct sidtab_isid_entry *entry = &s->isids[sid]; > >>> + int rc = context_cpy(&entry->context, context); > >>> + if (rc) > >>> + return rc; > >>> + > >>> + entry->set = 1; > >>> + return 0; > >>> +} > >>> + > >>> +static struct context *sidtab_lookup(struct sidtab *s, u32 sid) > >>> { > >>> int hvalue; > >>> struct sidtab_node *cur; > >>> > >>> - if (!s) > >>> - return NULL; > >>> - > >>> hvalue = SIDTAB_HASH(sid); > >>> cur = s->htable[hvalue]; > >>> while (cur && sid > cur->sid) > >>> cur = cur->next; > >>> > >>> - if (force && cur && sid == cur->sid && cur->context.len) > >>> - return &cur->context; > >>> + if (!cur || sid != cur->sid) > >>> + return NULL; > >>> > >>> - if (!cur || sid != cur->sid || cur->context.len) { > >>> - /* Remap invalid SIDs to the unlabeled SID. */ > >>> - sid = SECINITSID_UNLABELED; > >>> - hvalue = SIDTAB_HASH(sid); > >>> - cur = s->htable[hvalue]; > >>> - while (cur && sid > cur->sid) > >>> - cur = cur->next; > >>> - if (!cur || sid != cur->sid) > >>> - return NULL; > >>> + return &cur->context; > >>> +} > >>> + > >>> +static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) > >>> +{ > >>> + struct context *context; > >>> + struct sidtab_isid_entry *entry; > >>> + > >>> + if (!s) > >>> + return NULL; > >>> + > >>> + if (sid > SECINITSID_NUM) { > >>> + u32 index = sid - (SECINITSID_NUM + 1); > >>> + context = sidtab_lookup(s, index); > >>> + } else { > >>> + entry = &s->isids[sid]; > >>> + context = entry->set ? &entry->context : NULL; > >>> } > >>> + if (context && (!context->len || force)) > >>> + return context; > >>> > >>> - return &cur->context; > >>> + entry = &s->isids[SECINITSID_UNLABELED]; > >>> + return entry->set ? &entry->context : NULL; > >>> } > >>> > >>> struct context *sidtab_search(struct sidtab *s, u32 sid) > >>> @@ -145,11 +172,7 @@ out: > >>> static int clone_sid(u32 sid, struct context *context, void *arg) > >>> { > >>> struct sidtab *s = arg; > >>> - > >>> - if (sid > SECINITSID_NUM) > >>> - return sidtab_insert(s, sid, context); > >>> - else > >>> - return 0; > >>> + return sidtab_insert(s, sid, context); > >>> } > >>> > >>> int sidtab_convert(struct sidtab *s, struct sidtab *news, > >>> @@ -183,8 +206,8 @@ static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc > >>> s->cache[0] = n; > >>> } > >>> > >>> -static inline u32 sidtab_search_context(struct sidtab *s, > >>> - struct context *context) > >>> +static inline int sidtab_search_context(struct sidtab *s, > >>> + struct context *context, u32 *sid) > >>> { > >>> int i; > >>> struct sidtab_node *cur; > >>> @@ -194,15 +217,17 @@ static inline u32 sidtab_search_context(struct sidtab *s, > >>> while (cur) { > >>> if (context_cmp(&cur->context, context)) { > >>> sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); > >>> - return cur->sid; > >>> + *sid = cur->sid; > >>> + return 0; > >>> } > >>> cur = cur->next; > >>> } > >>> } > >>> - return 0; > >>> + return -ENOENT; > >>> } > >>> > >>> -static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > >>> +static inline int sidtab_search_cache(struct sidtab *s, struct context *context, > >>> + u32 *sid) > >>> { > >>> int i; > >>> struct sidtab_node *node; > >>> @@ -210,54 +235,67 @@ static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) > >>> for (i = 0; i < SIDTAB_CACHE_LEN; i++) { > >>> node = s->cache[i]; > >>> if (unlikely(!node)) > >>> - return 0; > >>> + return -ENOENT; > >>> if (context_cmp(&node->context, context)) { > >>> sidtab_update_cache(s, node, i); > >>> - return node->sid; > >>> + *sid = node->sid; > >>> + return 0; > >>> } > >>> } > >>> - return 0; > >>> + return -ENOENT; > >>> } > >>> > >>> -int sidtab_context_to_sid(struct sidtab *s, > >>> - struct context *context, > >>> - u32 *out_sid) > >>> +static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, > >>> + u32 *sid) > >>> { > >>> - u32 sid; > >>> - int ret = 0; > >>> + int ret; > >>> unsigned long flags; > >>> > >>> - *out_sid = SECSID_NULL; > >>> - > >>> - sid = sidtab_search_cache(s, context); > >>> - if (!sid) > >>> - sid = sidtab_search_context(s, context); > >>> - if (!sid) { > >>> + ret = sidtab_search_cache(s, context, sid); > >>> + if (ret) > >>> + ret = sidtab_search_context(s, context, sid); > >>> + if (ret) { > >>> spin_lock_irqsave(&s->lock, flags); > >>> /* Rescan now that we hold the lock. */ > >>> - sid = sidtab_search_context(s, context); > >>> - if (sid) > >>> + ret = sidtab_search_context(s, context, sid); > >>> + if (!ret) > >>> goto unlock_out; > >>> /* No SID exists for the context. Allocate a new one. */ > >>> - if (s->next_sid == UINT_MAX || s->shutdown) { > >>> + if (s->next_sid == (UINT_MAX - SECINITSID_NUM - 1) || s->shutdown) { > >>> ret = -ENOMEM; > >>> goto unlock_out; > >>> } > >>> - sid = s->next_sid++; > >>> + *sid = s->next_sid++; > >>> if (context->len) > >>> pr_info("SELinux: Context %s is not valid (left unmapped).\n", > >>> context->str); > >>> - ret = sidtab_insert(s, sid, context); > >>> + ret = sidtab_insert(s, *sid, context); > >>> if (ret) > >>> s->next_sid--; > >>> unlock_out: > >>> spin_unlock_irqrestore(&s->lock, flags); > >>> } > >>> > >>> - if (ret) > >>> - return ret; > >>> + return ret; > >>> +} > >>> + > >>> +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid) > >>> +{ > >>> + int rc; > >>> + u32 i; > >>> + > >>> + for (i = 0; i <= SECINITSID_NUM; i++) { > >>> + struct sidtab_isid_entry *entry = &s->isids[i]; > >>> + if (entry->set && context_cmp(context, &entry->context)) { > >>> + *sid = i; > >>> + return 0; > >>> + } > >>> + } > >>> > >>> - *out_sid = sid; > >>> + rc = sidtab_reverse_lookup(s, context, sid); > >>> + if (rc) > >>> + return rc; > >>> + *sid += SECINITSID_NUM + 1; > >>> return 0; > >>> } > >>> > >>> @@ -296,6 +334,11 @@ void sidtab_destroy(struct sidtab *s) > >>> if (!s) > >>> return; > >>> > >>> + for (i = 0; i <= SECINITSID_NUM; i++) > >>> + if (s->isids[i].set) > >>> + context_destroy(&s->isids[i].context); > >>> + > >>> + > >>> for (i = 0; i < SIDTAB_SIZE; i++) { > >>> cur = s->htable[i]; > >>> while (cur) { > >>> @@ -311,18 +354,3 @@ void sidtab_destroy(struct sidtab *s) > >>> s->nel = 0; > >>> s->next_sid = 1; > >>> } > >>> - > >>> -void sidtab_set(struct sidtab *dst, struct sidtab *src) > >>> -{ > >>> - unsigned long flags; > >>> - int i; > >>> - > >>> - spin_lock_irqsave(&src->lock, flags); > >>> - dst->htable = src->htable; > >>> - dst->nel = src->nel; > >>> - dst->next_sid = src->next_sid; > >>> - dst->shutdown = 0; > >>> - for (i = 0; i < SIDTAB_CACHE_LEN; i++) > >>> - dst->cache[i] = NULL; > >>> - spin_unlock_irqrestore(&src->lock, flags); > >>> -} > >>> diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h > >>> index 26c74fe7afc0..e181db3589bc 100644 > >>> --- a/security/selinux/ss/sidtab.h > >>> +++ b/security/selinux/ss/sidtab.h > >>> @@ -22,6 +22,11 @@ struct sidtab_node { > >>> > >>> #define SIDTAB_SIZE SIDTAB_HASH_BUCKETS > >>> > >>> +struct sidtab_isid_entry { > >>> + int set; > >>> + struct context context; > >>> +}; > >>> + > >>> struct sidtab { > >>> struct sidtab_node **htable; > >>> unsigned int nel; /* number of elements */ > >>> @@ -30,10 +35,12 @@ struct sidtab { > >>> #define SIDTAB_CACHE_LEN 3 > >>> struct sidtab_node *cache[SIDTAB_CACHE_LEN]; > >>> spinlock_t lock; > >>> + > >>> + struct sidtab_isid_entry isids[SECINITSID_NUM + 1]; > >>> }; > >>> > >>> int sidtab_init(struct sidtab *s); > >>> -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context); > >>> +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context); > >>> struct context *sidtab_search(struct sidtab *s, u32 sid); > >>> struct context *sidtab_search_force(struct sidtab *s, u32 sid); > >>> > >>> @@ -43,13 +50,10 @@ int sidtab_convert(struct sidtab *s, struct sidtab *news, > >>> void *args), > >>> void *args); > >>> > >>> -int sidtab_context_to_sid(struct sidtab *s, > >>> - struct context *context, > >>> - u32 *sid); > >>> +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); > >>> > >>> void sidtab_hash_eval(struct sidtab *h, char *tag); > >>> void sidtab_destroy(struct sidtab *s); > >>> -void sidtab_set(struct sidtab *dst, struct sidtab *src); > >>> > >>> #endif /* _SS_SIDTAB_H_ */ > >>> > >>> > >> > > > > -- > > Ondrej Mosnacek <omosnace at redhat dot com> > > Associate Software Engineer, Security Technologies > > Red Hat, Inc. > > > -- Ondrej Mosnacek <omosnace at redhat dot com> Associate Software Engineer, Security Technologies Red Hat, Inc.
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index f4eadd3f7350..21e4bdbcf994 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -909,13 +909,21 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) if (!c->context[0].user) { pr_err("SELinux: SID %s was never defined.\n", c->u.name); + sidtab_destroy(s); + goto out; + } + if (c->sid[0] > SECINITSID_NUM) { + pr_err("SELinux: Initial SID %s out of range.\n", + c->u.name); + sidtab_destroy(s); goto out; } - rc = sidtab_insert(s, c->sid[0], &c->context[0]); + rc = sidtab_set_initial(s, c->sid[0], &c->context[0]); if (rc) { pr_err("SELinux: unable to load initial SID %s.\n", c->u.name); + sidtab_destroy(s); goto out; } } diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 7337db24a6a8..30170d4c567a 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -776,7 +776,7 @@ static int security_compute_validatetrans(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; if (!user) tclass = unmap_class(&state->ss->map, orig_tclass); @@ -876,7 +876,7 @@ int security_bounded_transition(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; rc = -EINVAL; old_context = sidtab_search(sidtab, old_sid); @@ -1034,7 +1034,7 @@ void security_compute_xperms_decision(struct selinux_state *state, goto allow; policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; scontext = sidtab_search(sidtab, ssid); if (!scontext) { @@ -1123,7 +1123,7 @@ void security_compute_av(struct selinux_state *state, goto allow; policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; scontext = sidtab_search(sidtab, ssid); if (!scontext) { @@ -1177,7 +1177,7 @@ void security_compute_av_user(struct selinux_state *state, goto allow; policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; scontext = sidtab_search(sidtab, ssid); if (!scontext) { @@ -1315,7 +1315,7 @@ static int security_sid_to_context_core(struct selinux_state *state, } read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; if (force) context = sidtab_search_force(sidtab, sid); else @@ -1483,7 +1483,7 @@ static int security_context_to_sid_core(struct selinux_state *state, } read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; rc = string_to_context_struct(policydb, sidtab, scontext2, &context, def_sid); if (rc == -EINVAL && force) { @@ -1668,7 +1668,7 @@ static int security_compute_sid(struct selinux_state *state, } policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; scontext = sidtab_search(sidtab, ssid); if (!scontext) { @@ -1925,10 +1925,7 @@ static int convert_context(u32 key, struct user_datum *usrdatum; char *s; u32 len; - int rc = 0; - - if (key <= SECINITSID_NUM) - goto out; + int rc; args = p; @@ -2090,9 +2087,8 @@ static int security_preserve_bools(struct selinux_state *state, int security_load_policy(struct selinux_state *state, void *data, size_t len) { struct policydb *policydb; - struct sidtab *sidtab; + struct sidtab *oldsidtab, *newsidtab; struct policydb *oldpolicydb, *newpolicydb; - struct sidtab oldsidtab, newsidtab; struct selinux_mapping *oldmapping; struct selinux_map newmap; struct convert_context_args args; @@ -2108,27 +2104,37 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) newpolicydb = oldpolicydb + 1; policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + + newsidtab = kmalloc(sizeof(*newsidtab), GFP_KERNEL); + if (!newsidtab) { + rc = -ENOMEM; + goto out; + } if (!state->initialized) { rc = policydb_read(policydb, fp); - if (rc) + if (rc) { + kfree(newsidtab); goto out; + } policydb->len = len; rc = selinux_set_mapping(policydb, secclass_map, &state->ss->map); if (rc) { + kfree(newsidtab); policydb_destroy(policydb); goto out; } - rc = policydb_load_isids(policydb, sidtab); + rc = policydb_load_isids(policydb, newsidtab); if (rc) { + kfree(newsidtab); policydb_destroy(policydb); goto out; } + state->ss->sidtab = newsidtab; security_load_policycaps(state); state->initialized = 1; seqno = ++state->ss->latest_granting; @@ -2141,13 +2147,17 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) goto out; } + oldsidtab = state->ss->sidtab; + #if 0 - sidtab_hash_eval(sidtab, "sids"); + sidtab_hash_eval(oldsidtab, "sids"); #endif rc = policydb_read(newpolicydb, fp); - if (rc) + if (rc) { + kfree(newsidtab); goto out; + } newpolicydb->len = len; /* If switching between different policy types, log MLS status */ @@ -2156,10 +2166,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) else if (!policydb->mls_enabled && newpolicydb->mls_enabled) pr_info("SELinux: Enabling MLS support...\n"); - rc = policydb_load_isids(newpolicydb, &newsidtab); + rc = policydb_load_isids(newpolicydb, newsidtab); if (rc) { pr_err("SELinux: unable to load the initial SIDs\n"); policydb_destroy(newpolicydb); + kfree(newsidtab); goto out; } @@ -2180,7 +2191,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) args.state = state; args.oldp = policydb; args.newp = newpolicydb; - rc = sidtab_convert(sidtab, &newsidtab, convert_context, &args); + rc = sidtab_convert(oldsidtab, newsidtab, convert_context, &args); if (rc) { pr_err("SELinux: unable to convert the internal" " representation of contexts in the new SID" @@ -2190,12 +2201,11 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) /* Save the old policydb and SID table to free later. */ memcpy(oldpolicydb, policydb, sizeof(*policydb)); - sidtab_set(&oldsidtab, sidtab); /* Install the new policydb and SID table. */ write_lock_irq(&state->ss->policy_rwlock); memcpy(policydb, newpolicydb, sizeof(*policydb)); - sidtab_set(sidtab, &newsidtab); + state->ss->sidtab = newsidtab; security_load_policycaps(state); oldmapping = state->ss->map.mapping; state->ss->map.mapping = newmap.mapping; @@ -2205,7 +2215,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) /* Free the old policydb and SID table. */ policydb_destroy(oldpolicydb); - sidtab_destroy(&oldsidtab); + sidtab_destroy(oldsidtab); + kfree(oldsidtab); kfree(oldmapping); avc_ss_reset(state->avc, seqno); @@ -2219,7 +2230,8 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) err: kfree(newmap.mapping); - sidtab_destroy(&newsidtab); + sidtab_destroy(newsidtab); + kfree(newsidtab); policydb_destroy(newpolicydb); out: @@ -2256,7 +2268,7 @@ int security_port_sid(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; c = policydb->ocontexts[OCON_PORT]; while (c) { @@ -2302,7 +2314,7 @@ int security_ib_pkey_sid(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; c = policydb->ocontexts[OCON_IBPKEY]; while (c) { @@ -2348,7 +2360,7 @@ int security_ib_endport_sid(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; c = policydb->ocontexts[OCON_IBENDPORT]; while (c) { @@ -2394,7 +2406,7 @@ int security_netif_sid(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; c = policydb->ocontexts[OCON_NETIF]; while (c) { @@ -2459,7 +2471,7 @@ int security_node_sid(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; switch (domain) { case AF_INET: { @@ -2559,7 +2571,7 @@ int security_get_user_sids(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; context_init(&usercon); @@ -2661,7 +2673,7 @@ static inline int __security_genfs_sid(struct selinux_state *state, u32 *sid) { struct policydb *policydb = &state->ss->policydb; - struct sidtab *sidtab = &state->ss->sidtab; + struct sidtab *sidtab = state->ss->sidtab; int len; u16 sclass; struct genfs *genfs; @@ -2747,7 +2759,7 @@ int security_fs_use(struct selinux_state *state, struct super_block *sb) read_lock(&state->ss->policy_rwlock); policydb = &state->ss->policydb; - sidtab = &state->ss->sidtab; + sidtab = state->ss->sidtab; c = policydb->ocontexts[OCON_FSUSE]; while (c) { @@ -2953,7 +2965,7 @@ int security_sid_mls_copy(struct selinux_state *state, u32 sid, u32 mls_sid, u32 *new_sid) { struct policydb *policydb = &state->ss->policydb; - struct sidtab *sidtab = &state->ss->sidtab; + struct sidtab *sidtab = state->ss->sidtab; struct context *context1; struct context *context2; struct context newcon; @@ -3044,7 +3056,7 @@ int security_net_peersid_resolve(struct selinux_state *state, u32 *peer_sid) { struct policydb *policydb = &state->ss->policydb; - struct sidtab *sidtab = &state->ss->sidtab; + struct sidtab *sidtab = state->ss->sidtab; int rc; struct context *nlbl_ctx; struct context *xfrm_ctx; @@ -3405,7 +3417,7 @@ int selinux_audit_rule_match(u32 sid, u32 field, u32 op, void *vrule, goto out; } - ctxt = sidtab_search(&state->ss->sidtab, sid); + ctxt = sidtab_search(state->ss->sidtab, sid); if (unlikely(!ctxt)) { WARN_ONCE(1, "selinux_audit_rule_match: unrecognized SID %d\n", sid); @@ -3568,7 +3580,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state, u32 *sid) { struct policydb *policydb = &state->ss->policydb; - struct sidtab *sidtab = &state->ss->sidtab; + struct sidtab *sidtab = state->ss->sidtab; int rc; struct context *ctx; struct context ctx_new; @@ -3646,7 +3658,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state, read_lock(&state->ss->policy_rwlock); rc = -ENOENT; - ctx = sidtab_search(&state->ss->sidtab, sid); + ctx = sidtab_search(state->ss->sidtab, sid); if (ctx == NULL) goto out; diff --git a/security/selinux/ss/services.h b/security/selinux/ss/services.h index 24c7bdcc8075..9a36de860368 100644 --- a/security/selinux/ss/services.h +++ b/security/selinux/ss/services.h @@ -24,7 +24,7 @@ struct selinux_map { }; struct selinux_ss { - struct sidtab sidtab; + struct sidtab *sidtab; struct policydb policydb; rwlock_t policy_rwlock; u32 latest_granting; diff --git a/security/selinux/ss/sidtab.c b/security/selinux/ss/sidtab.c index e66a2ab3d1c2..049ac1e6fd06 100644 --- a/security/selinux/ss/sidtab.c +++ b/security/selinux/ss/sidtab.c @@ -22,16 +22,24 @@ int sidtab_init(struct sidtab *s) s->htable = kmalloc_array(SIDTAB_SIZE, sizeof(*s->htable), GFP_ATOMIC); if (!s->htable) return -ENOMEM; + + for (i = 0; i <= SECINITSID_NUM; i++) + s->isids[i].set = 0; + for (i = 0; i < SIDTAB_SIZE; i++) s->htable[i] = NULL; + + for (i = 0; i < SIDTAB_CACHE_LEN; i++) + s->cache[i] = NULL; + s->nel = 0; - s->next_sid = 1; + s->next_sid = 0; s->shutdown = 0; spin_lock_init(&s->lock); return 0; } -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) +static int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) { int hvalue; struct sidtab_node *prev, *cur, *newnode; @@ -76,34 +84,53 @@ int sidtab_insert(struct sidtab *s, u32 sid, struct context *context) return 0; } -static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context) +{ + struct sidtab_isid_entry *entry = &s->isids[sid]; + int rc = context_cpy(&entry->context, context); + if (rc) + return rc; + + entry->set = 1; + return 0; +} + +static struct context *sidtab_lookup(struct sidtab *s, u32 sid) { int hvalue; struct sidtab_node *cur; - if (!s) - return NULL; - hvalue = SIDTAB_HASH(sid); cur = s->htable[hvalue]; while (cur && sid > cur->sid) cur = cur->next; - if (force && cur && sid == cur->sid && cur->context.len) - return &cur->context; + if (!cur || sid != cur->sid) + return NULL; - if (!cur || sid != cur->sid || cur->context.len) { - /* Remap invalid SIDs to the unlabeled SID. */ - sid = SECINITSID_UNLABELED; - hvalue = SIDTAB_HASH(sid); - cur = s->htable[hvalue]; - while (cur && sid > cur->sid) - cur = cur->next; - if (!cur || sid != cur->sid) - return NULL; + return &cur->context; +} + +static struct context *sidtab_search_core(struct sidtab *s, u32 sid, int force) +{ + struct context *context; + struct sidtab_isid_entry *entry; + + if (!s) + return NULL; + + if (sid > SECINITSID_NUM) { + u32 index = sid - (SECINITSID_NUM + 1); + context = sidtab_lookup(s, index); + } else { + entry = &s->isids[sid]; + context = entry->set ? &entry->context : NULL; } + if (context && (!context->len || force)) + return context; - return &cur->context; + entry = &s->isids[SECINITSID_UNLABELED]; + return entry->set ? &entry->context : NULL; } struct context *sidtab_search(struct sidtab *s, u32 sid) @@ -145,11 +172,7 @@ out: static int clone_sid(u32 sid, struct context *context, void *arg) { struct sidtab *s = arg; - - if (sid > SECINITSID_NUM) - return sidtab_insert(s, sid, context); - else - return 0; + return sidtab_insert(s, sid, context); } int sidtab_convert(struct sidtab *s, struct sidtab *news, @@ -183,8 +206,8 @@ static void sidtab_update_cache(struct sidtab *s, struct sidtab_node *n, int loc s->cache[0] = n; } -static inline u32 sidtab_search_context(struct sidtab *s, - struct context *context) +static inline int sidtab_search_context(struct sidtab *s, + struct context *context, u32 *sid) { int i; struct sidtab_node *cur; @@ -194,15 +217,17 @@ static inline u32 sidtab_search_context(struct sidtab *s, while (cur) { if (context_cmp(&cur->context, context)) { sidtab_update_cache(s, cur, SIDTAB_CACHE_LEN - 1); - return cur->sid; + *sid = cur->sid; + return 0; } cur = cur->next; } } - return 0; + return -ENOENT; } -static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) +static inline int sidtab_search_cache(struct sidtab *s, struct context *context, + u32 *sid) { int i; struct sidtab_node *node; @@ -210,54 +235,67 @@ static inline u32 sidtab_search_cache(struct sidtab *s, struct context *context) for (i = 0; i < SIDTAB_CACHE_LEN; i++) { node = s->cache[i]; if (unlikely(!node)) - return 0; + return -ENOENT; if (context_cmp(&node->context, context)) { sidtab_update_cache(s, node, i); - return node->sid; + *sid = node->sid; + return 0; } } - return 0; + return -ENOENT; } -int sidtab_context_to_sid(struct sidtab *s, - struct context *context, - u32 *out_sid) +static int sidtab_reverse_lookup(struct sidtab *s, struct context *context, + u32 *sid) { - u32 sid; - int ret = 0; + int ret; unsigned long flags; - *out_sid = SECSID_NULL; - - sid = sidtab_search_cache(s, context); - if (!sid) - sid = sidtab_search_context(s, context); - if (!sid) { + ret = sidtab_search_cache(s, context, sid); + if (ret) + ret = sidtab_search_context(s, context, sid); + if (ret) { spin_lock_irqsave(&s->lock, flags); /* Rescan now that we hold the lock. */ - sid = sidtab_search_context(s, context); - if (sid) + ret = sidtab_search_context(s, context, sid); + if (!ret) goto unlock_out; /* No SID exists for the context. Allocate a new one. */ - if (s->next_sid == UINT_MAX || s->shutdown) { + if (s->next_sid == (UINT_MAX - SECINITSID_NUM - 1) || s->shutdown) { ret = -ENOMEM; goto unlock_out; } - sid = s->next_sid++; + *sid = s->next_sid++; if (context->len) pr_info("SELinux: Context %s is not valid (left unmapped).\n", context->str); - ret = sidtab_insert(s, sid, context); + ret = sidtab_insert(s, *sid, context); if (ret) s->next_sid--; unlock_out: spin_unlock_irqrestore(&s->lock, flags); } - if (ret) - return ret; + return ret; +} + +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid) +{ + int rc; + u32 i; + + for (i = 0; i <= SECINITSID_NUM; i++) { + struct sidtab_isid_entry *entry = &s->isids[i]; + if (entry->set && context_cmp(context, &entry->context)) { + *sid = i; + return 0; + } + } - *out_sid = sid; + rc = sidtab_reverse_lookup(s, context, sid); + if (rc) + return rc; + *sid += SECINITSID_NUM + 1; return 0; } @@ -296,6 +334,11 @@ void sidtab_destroy(struct sidtab *s) if (!s) return; + for (i = 0; i <= SECINITSID_NUM; i++) + if (s->isids[i].set) + context_destroy(&s->isids[i].context); + + for (i = 0; i < SIDTAB_SIZE; i++) { cur = s->htable[i]; while (cur) { @@ -311,18 +354,3 @@ void sidtab_destroy(struct sidtab *s) s->nel = 0; s->next_sid = 1; } - -void sidtab_set(struct sidtab *dst, struct sidtab *src) -{ - unsigned long flags; - int i; - - spin_lock_irqsave(&src->lock, flags); - dst->htable = src->htable; - dst->nel = src->nel; - dst->next_sid = src->next_sid; - dst->shutdown = 0; - for (i = 0; i < SIDTAB_CACHE_LEN; i++) - dst->cache[i] = NULL; - spin_unlock_irqrestore(&src->lock, flags); -} diff --git a/security/selinux/ss/sidtab.h b/security/selinux/ss/sidtab.h index 26c74fe7afc0..e181db3589bc 100644 --- a/security/selinux/ss/sidtab.h +++ b/security/selinux/ss/sidtab.h @@ -22,6 +22,11 @@ struct sidtab_node { #define SIDTAB_SIZE SIDTAB_HASH_BUCKETS +struct sidtab_isid_entry { + int set; + struct context context; +}; + struct sidtab { struct sidtab_node **htable; unsigned int nel; /* number of elements */ @@ -30,10 +35,12 @@ struct sidtab { #define SIDTAB_CACHE_LEN 3 struct sidtab_node *cache[SIDTAB_CACHE_LEN]; spinlock_t lock; + + struct sidtab_isid_entry isids[SECINITSID_NUM + 1]; }; int sidtab_init(struct sidtab *s); -int sidtab_insert(struct sidtab *s, u32 sid, struct context *context); +int sidtab_set_initial(struct sidtab *s, u32 sid, struct context *context); struct context *sidtab_search(struct sidtab *s, u32 sid); struct context *sidtab_search_force(struct sidtab *s, u32 sid); @@ -43,13 +50,10 @@ int sidtab_convert(struct sidtab *s, struct sidtab *news, void *args), void *args); -int sidtab_context_to_sid(struct sidtab *s, - struct context *context, - u32 *sid); +int sidtab_context_to_sid(struct sidtab *s, struct context *context, u32 *sid); void sidtab_hash_eval(struct sidtab *h, char *tag); void sidtab_destroy(struct sidtab *s); -void sidtab_set(struct sidtab *dst, struct sidtab *src); #endif /* _SS_SIDTAB_H_ */
This patch is non-functional and moves handling of initial SIDs into a separate table. Note that the SIDs stored in the main table are now shifted by SECINITSID_NUM and converted to/from the actual SIDs transparently by helper functions. This change doesn't make much sense on its own, but it simplifies further sidtab overhaul in a succeeding patch. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/ss/policydb.c | 10 ++- security/selinux/ss/services.c | 88 ++++++++++-------- security/selinux/ss/services.h | 2 +- security/selinux/ss/sidtab.c | 158 +++++++++++++++++++-------------- security/selinux/ss/sidtab.h | 14 +-- 5 files changed, 162 insertions(+), 110 deletions(-)