[PATCH V3 2/5 selinux-next] selinux: Introduce selinux_ruleset struct
J Freyensee
why2jjj.linux at gmail.com
Wed May 30 21:15:15 UTC 2018
(snip)
.
.
.
>
> -static void security_load_policycaps(struct selinux_state *state)
> +static void security_load_policycaps(struct selinux_state *state,
> + struct policydb *p)
> {
> - struct policydb *p = &state->ss->policydb;
> unsigned int i;
> struct ebitmap_node *node;
>
> @@ -2107,47 +2112,47 @@ 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 policydb *oldpolicydb, *newpolicydb;
> - struct sidtab oldsidtab, newsidtab;
> - struct selinux_mapping *oldmapping;
> struct selinux_map newmap;
> struct convert_context_args args;
> u32 seqno;
> int rc = 0;
> + struct selinux_ruleset *next_set, *old_set;
> struct policy_file file = { data, len }, *fp = &file;
>
> - oldpolicydb = kzalloc(2 * sizeof(*oldpolicydb), GFP_KERNEL);
> - if (!oldpolicydb) {
> + next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
> + if (!next_set) {
> rc = -ENOMEM;
> goto out;
> }
> - newpolicydb = oldpolicydb + 1;
> -
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + next_set->sidtab = kzalloc(sizeof(struct sidtab), GFP_KERNEL);
> + if (!next_set->sidtab) {
> + rc = -ENOMEM;
> + kfree(next_set);
How about moving these kfree(next_set) into the 'goto out' cleanup? The
effort has already been made to have a goto cleanup section in
security_load_policy). There is a lot of diff changes in here which is
hard to follow, and my worry is a kfree(next_set); could get missed, or
is not as easily maintainable as if the code was restructured to have a
single kfree(next_set); call for all 'goto out;' cleanup situations.
> + goto out;
> + }
>
> if (!state->initialized) {
> - rc = policydb_read(policydb, fp);
> + old_set = state->ss->active_set;
> + rc = policydb_read(&next_set->policydb, fp);
> if (rc)
> goto out;
>
> - policydb->len = len;
> - rc = selinux_set_mapping(policydb, secclass_map,
> - &state->ss->map);
> + next_set->policydb.len = len;
> + rc = selinux_set_mapping(&next_set->policydb, secclass_map,
> + &next_set->map);
> if (rc) {
> - policydb_destroy(policydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - rc = policydb_load_isids(policydb, sidtab);
> + rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
> if (rc) {
> - policydb_destroy(policydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - security_load_policycaps(state);
> + security_load_policycaps(state, &next_set->policydb);
> + state->ss->active_set = next_set;
> state->initialized = 1;
> seqno = ++state->ss->latest_granting;
> selinux_complete_init();
> @@ -2156,45 +2161,48 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
> + kfree(old_set->sidtab);
> + kfree(old_set);
> goto out;
> }
> -
> + old_set = state->ss->active_set;
> #if 0
> sidtab_hash_eval(sidtab, "sids");
> #endif
>
> - rc = policydb_read(newpolicydb, fp);
> + rc = policydb_read(&next_set->policydb, fp);
> if (rc)
> goto out;
>
> - newpolicydb->len = len;
> + next_set->policydb.len = len;
> +
> /* If switching between different policy types, log MLS status */
> - if (policydb->mls_enabled && !newpolicydb->mls_enabled)
> + if (old_set->policydb.mls_enabled && !next_set->policydb.mls_enabled)
> printk(KERN_INFO "SELinux: Disabling MLS support...\n");
> - else if (!policydb->mls_enabled && newpolicydb->mls_enabled)
> + else if (!old_set->policydb.mls_enabled
> + && next_set->policydb.mls_enabled)
> printk(KERN_INFO "SELinux: Enabling MLS support...\n");
> -
> - rc = policydb_load_isids(newpolicydb, &newsidtab);
> + rc = policydb_load_isids(&next_set->policydb, next_set->sidtab);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to load the initial SIDs\n");
> - policydb_destroy(newpolicydb);
> + policydb_destroy(&next_set->policydb);
> goto out;
> }
>
> - rc = selinux_set_mapping(newpolicydb, secclass_map, &newmap);
> + rc = selinux_set_mapping(&next_set->policydb, secclass_map, &newmap);
> if (rc)
> goto err;
>
> - rc = security_preserve_bools(state, newpolicydb);
> + rc = security_preserve_bools(state, &next_set->policydb);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to preserve booleans\n");
> goto err;
> }
>
> /* Clone the SID table. */
> - sidtab_shutdown(sidtab);
> + sidtab_shutdown(old_set->sidtab);
>
> - rc = sidtab_map(sidtab, clone_sid, &newsidtab);
> + rc = sidtab_map(old_set->sidtab, clone_sid, next_set->sidtab);
> if (rc)
> goto err;
>
> @@ -2203,9 +2211,9 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> * in the new SID table.
> */
> args.state = state;
> - args.oldp = policydb;
> - args.newp = newpolicydb;
> - rc = sidtab_map(&newsidtab, convert_context, &args);
> + args.oldp = &old_set->policydb;
> + args.newp = &next_set->policydb;
> + rc = sidtab_map(next_set->sidtab, convert_context, &args);
> if (rc) {
> printk(KERN_ERR "SELinux: unable to convert the internal"
> " representation of contexts in the new SID"
> @@ -2213,48 +2221,43 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len)
> goto err;
> }
>
> - /* Save the old policydb and SID table to free later. */
> - memcpy(oldpolicydb, policydb, sizeof(*policydb));
> - sidtab_set(&oldsidtab, sidtab);
> + next_set->map.mapping = newmap.mapping;
> + next_set->map.size = newmap.size;
>
> /* Install the new policydb and SID table. */
> write_lock_irq(&state->ss->policy_rwlock);
> - memcpy(policydb, newpolicydb, sizeof(*policydb));
> - sidtab_set(sidtab, &newsidtab);
> - security_load_policycaps(state);
> - oldmapping = state->ss->map.mapping;
> - state->ss->map.mapping = newmap.mapping;
> - state->ss->map.size = newmap.size;
> + security_load_policycaps(state, &next_set->policydb);
> seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> write_unlock_irq(&state->ss->policy_rwlock);
>
> - /* Free the old policydb and SID table. */
> - policydb_destroy(oldpolicydb);
> - sidtab_destroy(&oldsidtab);
> - kfree(oldmapping);
> -
> avc_ss_reset(state->avc, seqno);
> selnl_notify_policyload(seqno);
> selinux_status_update_policyload(state, seqno);
> selinux_netlbl_cache_invalidate();
> selinux_xfrm_notify_policyload();
>
> + /* Free the old policydb and SID table. */
> + policydb_destroy(&old_set->policydb);
> + sidtab_destroy(old_set->sidtab);
> + kfree(old_set->sidtab);
> + kfree(old_set->map.mapping);
> + kfree(old_set);
> rc = 0;
> goto out;
>
> err:
> kfree(newmap.mapping);
> - sidtab_destroy(&newsidtab);
> - policydb_destroy(newpolicydb);
> -
> + sidtab_destroy(next_set->sidtab);
> + policydb_destroy(&next_set->policydb);
> + kfree(next_set);
> out:
> - kfree(oldpolicydb);
> return rc;
> }
>
> size_t security_policydb_len(struct selinux_state *state)
> {
> - struct policydb *p = &state->ss->policydb;
> + struct policydb *p = &state->ss->active_set->policydb;
> size_t len;
>
> read_lock(&state->ss->policy_rwlock);
> @@ -2280,8 +2283,8 @@ int security_port_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_PORT];
> while (c) {
> @@ -2326,8 +2329,8 @@ int security_ib_pkey_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_IBPKEY];
> while (c) {
> @@ -2372,8 +2375,8 @@ int security_ib_endport_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_IBENDPORT];
> while (c) {
> @@ -2418,8 +2421,8 @@ int security_netif_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_NETIF];
> while (c) {
> @@ -2483,8 +2486,8 @@ int security_node_sid(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> switch (domain) {
> case AF_INET: {
> @@ -2583,8 +2586,8 @@ int security_get_user_sids(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> - sidtab = &state->ss->sidtab;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> context_init(&usercon);
>
> @@ -2685,8 +2688,8 @@ static inline int __security_genfs_sid(struct selinux_state *state,
> u16 orig_sclass,
> u32 *sid)
> {
> - struct policydb *policydb = &state->ss->policydb;
> - struct sidtab *sidtab = &state->ss->sidtab;
> + struct policydb *policydb = &state->ss->active_set->policydb;
> + struct sidtab *sidtab = state->ss->active_set->sidtab;
> int len;
> u16 sclass;
> struct genfs *genfs;
> @@ -2696,7 +2699,7 @@ static inline int __security_genfs_sid(struct selinux_state *state,
> while (path[0] == '/' && path[1] == '/')
> path++;
>
> - sclass = unmap_class(&state->ss->map, orig_sclass);
> + sclass = unmap_class(&state->ss->active_set->map, orig_sclass);
> *sid = SECINITSID_UNLABELED;
>
> for (genfs = policydb->genfs; genfs; genfs = genfs->next) {
> @@ -2771,8 +2774,8 @@ 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;
> + policydb = &state->ss->active_set->policydb;
> + sidtab = state->ss->active_set->sidtab;
>
> c = policydb->ocontexts[OCON_FSUSE];
> while (c) {
> @@ -2821,7 +2824,7 @@ int security_get_bools(struct selinux_state *state,
>
> read_lock(&state->ss->policy_rwlock);
>
> - policydb = &state->ss->policydb;
> + policydb = &state->ss->active_set->policydb;
>
> *names = NULL;
> *values = NULL;
> @@ -2866,53 +2869,86 @@ int security_get_bools(struct selinux_state *state,
>
> int security_set_bools(struct selinux_state *state, int len, int *values)
> {
> - struct policydb *policydb;
> int i, rc;
> int lenp, seqno = 0;
> struct cond_node *cur;
> + struct selinux_ruleset *next_set, *old_set = NULL;
> + void *storage;
> + size_t size;
>
> - write_lock_irq(&state->ss->policy_rwlock);
> + next_set = kzalloc(sizeof(struct selinux_ruleset), GFP_KERNEL);
> + if (!next_set) {
> + rc = -ENOMEM;
> + goto errout;
> + }
> +
> + rc = policydb_flattened_alloc(&state->ss->active_set->policydb,
> + &storage, &size);
> + if (rc) {
> + kfree(next_set);
I think this function, security_set_bools(), is another case where it
would be good to restructure the code where there is a single location
where kfree(next_set); is called for code readability and
maintainability. Or at the very least keeping the goto cleanup strategy
consistent; here, for normal 'goto out;' routines kfree(next_set); is
there in the 'out' block, but for 'goto errout;' routines it is not
there and kfree(next_set); must be called before-hand.
> + goto errout;
> + }
>
> - policydb = &state->ss->policydb;
> + write_lock_irq(&state->ss->policy_rwlock);
> + old_set = state->ss->active_set;
> + memcpy(next_set, old_set, sizeof(struct selinux_ruleset));
> + rc = policydb_copy(&old_set->policydb, &next_set->policydb,
> + &storage, size);
> + if (rc)
> + goto out;
>
> rc = -EFAULT;
> - lenp = policydb->p_bools.nprim;
> + lenp = next_set->policydb.p_bools.nprim;
> if (len != lenp)
> goto out;
>
> for (i = 0; i < len; i++) {
> - if (!!values[i] != policydb->bool_val_to_struct[i]->state) {
> + if (!!values[i] !=
> + next_set->policydb.bool_val_to_struct[i]->state) {
> audit_log(current->audit_context, GFP_ATOMIC,
> AUDIT_MAC_CONFIG_CHANGE,
> "bool=%s val=%d old_val=%d auid=%u ses=%u",
> - sym_name(policydb, SYM_BOOLS, i),
> + sym_name(&next_set->policydb, SYM_BOOLS, i),
> !!values[i],
> - policydb->bool_val_to_struct[i]->state,
> + next_set->policydb.bool_val_to_struct[i]->state,
> from_kuid(&init_user_ns, audit_get_loginuid(current)),
> audit_get_sessionid(current));
> }
> if (values[i])
> - policydb->bool_val_to_struct[i]->state = 1;
> + next_set->policydb.bool_val_to_struct[i]->state = 1;
> else
> - policydb->bool_val_to_struct[i]->state = 0;
> + next_set->policydb.bool_val_to_struct[i]->state = 0;
> }
>
> - for (cur = policydb->cond_list; cur; cur = cur->next) {
> - rc = evaluate_cond_node(policydb, cur);
> + for (cur = next_set->policydb.cond_list; cur; cur = cur->next) {
> + rc = evaluate_cond_node(&next_set->policydb, cur);
> if (rc)
> goto out;
> }
>
> seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> rc = 0;
> out:
> - write_unlock_irq(&state->ss->policy_rwlock);
> if (!rc) {
> + seqno = ++state->ss->latest_granting;
> + state->ss->active_set = next_set;
> + rc = 0;
Why would we want to set rc to 0 here and have the return value be 0
instead of the original rc value evaluated in the if() statement above?
> + write_unlock_irq(&state->ss->policy_rwlock);
> avc_ss_reset(state->avc, seqno);
> selnl_notify_policyload(seqno);
> selinux_status_update_policyload(state, seqno);
> selinux_xfrm_notify_policyload();
> + policydb_destroy(&old_set->policydb);
> + kfree(old_set);
> + } else {
> + printk(KERN_ERR "SELinux: %s failed %d\n", __func__, rc);
> + write_unlock_irq(&state->ss->policy_rwlock);
> + kfree(next_set);
> }
> + policydb_flattened_free(storage);
> +
> + errout:
> return rc;
> }
>
> @@
Thanks,
Jay
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
More information about the Linux-security-module-archive
mailing list