| Message ID | 20200116120439.303034-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: Assorted simplifications and cleanups | expand |
On 1/16/20 7:04 AM, Ondrej Mosnacek wrote: > First, evaluate_cond_node() never returns an error. Make it just return > void. > > Second, drop the use of security_get_bools() from > security_preserve_bools() and read from the old policydb directly. This > saves some useless allocations and together with the first change makes > security_preserve_bools() no longer possibly return an error. Again the > return type is changed to void. > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Dropping use of security_get_bools() means we are no longer reading the boolean values with the policy read-lock held so they could in theory change underneath us. However, this is presently prevented via the fsi->mutex taken by selinuxfs so I believe this is safe. Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov> > --- > security/selinux/ss/conditional.c | 3 +- > security/selinux/ss/conditional.h | 2 +- > security/selinux/ss/services.c | 52 ++++++++++--------------------- > 3 files changed, 18 insertions(+), 39 deletions(-) > > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c > index 70c378ee1a2f..04593062008d 100644 > --- a/security/selinux/ss/conditional.c > +++ b/security/selinux/ss/conditional.c > @@ -85,7 +85,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) > * list appropriately. If the result of the expression is undefined > * all of the rules are disabled for safety. > */ > -int evaluate_cond_node(struct policydb *p, struct cond_node *node) > +void evaluate_cond_node(struct policydb *p, struct cond_node *node) > { > int new_state; > struct cond_av_list *cur; > @@ -111,7 +111,6 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) > cur->node->key.specified |= AVTAB_ENABLED; > } > } > - return 0; > } > > int cond_policydb_init(struct policydb *p) > diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h > index ec846e45904c..d86ef286ca84 100644 > --- a/security/selinux/ss/conditional.h > +++ b/security/selinux/ss/conditional.h > @@ -75,6 +75,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, > struct av_decision *avd, struct extended_perms *xperms); > void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key, > struct extended_perms_decision *xpermd); > -int evaluate_cond_node(struct policydb *p, struct cond_node *node); > +void evaluate_cond_node(struct policydb *p, struct cond_node *node); > > #endif /* _CONDITIONAL_H_ */ > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c > index 42ca9f6dbbf4..b9eda7d89e22 100644 > --- a/security/selinux/ss/services.c > +++ b/security/selinux/ss/services.c > @@ -2157,8 +2157,8 @@ static void security_load_policycaps(struct selinux_state *state) > } > } > > -static int security_preserve_bools(struct selinux_state *state, > - struct policydb *newpolicydb); > +static void security_preserve_bools(struct policydb *oldpolicydb, > + struct policydb *newpolicydb); > > /** > * security_load_policy - Load a security policy configuration. > @@ -2257,11 +2257,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) > if (rc) > goto err; > > - rc = security_preserve_bools(state, newpolicydb); > - if (rc) { > - pr_err("SELinux: unable to preserve booleans\n"); > - goto err; > - } > + security_preserve_bools(policydb, newpolicydb); > > oldsidtab = state->ss->sidtab; > > @@ -2958,11 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values) > policydb->bool_val_to_struct[i]->state = 0; > } > > - for (cur = policydb->cond_list; cur; cur = cur->next) { > - rc = evaluate_cond_node(policydb, cur); > - if (rc) > - goto out; > - } > + for (cur = policydb->cond_list; cur; cur = cur->next) > + evaluate_cond_node(policydb, cur); > > seqno = ++state->ss->latest_granting; > rc = 0; > @@ -2999,36 +2992,23 @@ out: > return rc; > } > > -static int security_preserve_bools(struct selinux_state *state, > - struct policydb *policydb) > +static void security_preserve_bools(struct policydb *oldpolicydb, > + struct policydb *newpolicydb) > { > - int rc, nbools = 0, *bvalues = NULL, i; > - char **bnames = NULL; > struct cond_bool_datum *booldatum; > struct cond_node *cur; > + int i; > > - rc = security_get_bools(state, &nbools, &bnames, &bvalues); > - if (rc) > - goto out; > - for (i = 0; i < nbools; i++) { > - booldatum = hashtab_search(policydb->p_bools.table, bnames[i]); > - if (booldatum) > - booldatum->state = bvalues[i]; > - } > - for (cur = policydb->cond_list; cur; cur = cur->next) { > - rc = evaluate_cond_node(policydb, cur); > - if (rc) > - goto out; > - } > + for (i = 0; i < oldpolicydb->p_bools.nprim; i++) { > + const char *name = sym_name(oldpolicydb, SYM_BOOLS, i); > + int value = oldpolicydb->bool_val_to_struct[i]->state; > > -out: > - if (bnames) { > - for (i = 0; i < nbools; i++) > - kfree(bnames[i]); > + booldatum = hashtab_search(newpolicydb->p_bools.table, name); > + if (booldatum) > + booldatum->state = value; > } > - kfree(bnames); > - kfree(bvalues); > - return rc; > + for (cur = newpolicydb->cond_list; cur; cur = cur->next) > + evaluate_cond_node(newpolicydb, cur); > } > > /* >
On Thu, Jan 16, 2020 at 11:41 AM Stephen Smalley <sds@tycho.nsa.gov> wrote: > On 1/16/20 7:04 AM, Ondrej Mosnacek wrote: > > First, evaluate_cond_node() never returns an error. Make it just return > > void. > > > > Second, drop the use of security_get_bools() from > > security_preserve_bools() and read from the old policydb directly. This > > saves some useless allocations and together with the first change makes > > security_preserve_bools() no longer possibly return an error. Again the > > return type is changed to void. > > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > Dropping use of security_get_bools() means we are no longer reading the > boolean values with the policy read-lock held so they could in theory > change underneath us. However, this is presently prevented via the > fsi->mutex taken by selinuxfs so I believe this is safe. Since this code shouldn't be run very often, I think I would prefer the added abstraction and safety of preserving the call to security_get_bools(). In an effort to make sure expectations are set correctly, patches 2 through 6 are something that should probably wait until after the upcoming merge window, so no rush on a respin.
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 70c378ee1a2f..04593062008d 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -85,7 +85,7 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) * list appropriately. If the result of the expression is undefined * all of the rules are disabled for safety. */ -int evaluate_cond_node(struct policydb *p, struct cond_node *node) +void evaluate_cond_node(struct policydb *p, struct cond_node *node) { int new_state; struct cond_av_list *cur; @@ -111,7 +111,6 @@ int evaluate_cond_node(struct policydb *p, struct cond_node *node) cur->node->key.specified |= AVTAB_ENABLED; } } - return 0; } int cond_policydb_init(struct policydb *p) diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index ec846e45904c..d86ef286ca84 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -75,6 +75,6 @@ void cond_compute_av(struct avtab *ctab, struct avtab_key *key, struct av_decision *avd, struct extended_perms *xperms); void cond_compute_xperms(struct avtab *ctab, struct avtab_key *key, struct extended_perms_decision *xpermd); -int evaluate_cond_node(struct policydb *p, struct cond_node *node); +void evaluate_cond_node(struct policydb *p, struct cond_node *node); #endif /* _CONDITIONAL_H_ */ diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 42ca9f6dbbf4..b9eda7d89e22 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -2157,8 +2157,8 @@ static void security_load_policycaps(struct selinux_state *state) } } -static int security_preserve_bools(struct selinux_state *state, - struct policydb *newpolicydb); +static void security_preserve_bools(struct policydb *oldpolicydb, + struct policydb *newpolicydb); /** * security_load_policy - Load a security policy configuration. @@ -2257,11 +2257,7 @@ int security_load_policy(struct selinux_state *state, void *data, size_t len) if (rc) goto err; - rc = security_preserve_bools(state, newpolicydb); - if (rc) { - pr_err("SELinux: unable to preserve booleans\n"); - goto err; - } + security_preserve_bools(policydb, newpolicydb); oldsidtab = state->ss->sidtab; @@ -2958,11 +2954,8 @@ int security_set_bools(struct selinux_state *state, int len, int *values) policydb->bool_val_to_struct[i]->state = 0; } - for (cur = policydb->cond_list; cur; cur = cur->next) { - rc = evaluate_cond_node(policydb, cur); - if (rc) - goto out; - } + for (cur = policydb->cond_list; cur; cur = cur->next) + evaluate_cond_node(policydb, cur); seqno = ++state->ss->latest_granting; rc = 0; @@ -2999,36 +2992,23 @@ out: return rc; } -static int security_preserve_bools(struct selinux_state *state, - struct policydb *policydb) +static void security_preserve_bools(struct policydb *oldpolicydb, + struct policydb *newpolicydb) { - int rc, nbools = 0, *bvalues = NULL, i; - char **bnames = NULL; struct cond_bool_datum *booldatum; struct cond_node *cur; + int i; - rc = security_get_bools(state, &nbools, &bnames, &bvalues); - if (rc) - goto out; - for (i = 0; i < nbools; i++) { - booldatum = hashtab_search(policydb->p_bools.table, bnames[i]); - if (booldatum) - booldatum->state = bvalues[i]; - } - for (cur = policydb->cond_list; cur; cur = cur->next) { - rc = evaluate_cond_node(policydb, cur); - if (rc) - goto out; - } + for (i = 0; i < oldpolicydb->p_bools.nprim; i++) { + const char *name = sym_name(oldpolicydb, SYM_BOOLS, i); + int value = oldpolicydb->bool_val_to_struct[i]->state; -out: - if (bnames) { - for (i = 0; i < nbools; i++) - kfree(bnames[i]); + booldatum = hashtab_search(newpolicydb->p_bools.table, name); + if (booldatum) + booldatum->state = value; } - kfree(bnames); - kfree(bvalues); - return rc; + for (cur = newpolicydb->cond_list; cur; cur = cur->next) + evaluate_cond_node(newpolicydb, cur); } /*
First, evaluate_cond_node() never returns an error. Make it just return void. Second, drop the use of security_get_bools() from security_preserve_bools() and read from the old policydb directly. This saves some useless allocations and together with the first change makes security_preserve_bools() no longer possibly return an error. Again the return type is changed to void. Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- security/selinux/ss/conditional.c | 3 +- security/selinux/ss/conditional.h | 2 +- security/selinux/ss/services.c | 52 ++++++++++--------------------- 3 files changed, 18 insertions(+), 39 deletions(-)