* [PATCH v2 1/5] selinux: simplify evaluate_cond_node()
2020-01-17 8:58 [PATCH v2 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
@ 2020-01-17 8:58 ` Ondrej Mosnacek
2020-01-17 19:37 ` Stephen Smalley
2020-01-31 3:47 ` Paul Moore
2020-01-17 8:58 ` [PATCH v2 2/5] selinux: convert cond_list to array Ondrej Mosnacek
` (3 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-01-17 8:58 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
It never fails, so it can just return 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 | 14 ++++----------
3 files changed, 6 insertions(+), 13 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 216ce602a2b5..ff43a35bb874 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2958,11 +2958,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;
@@ -3015,11 +3012,8 @@ static int security_preserve_bools(struct selinux_state *state,
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 (cur = policydb->cond_list; cur; cur = cur->next)
+ evaluate_cond_node(policydb, cur);
out:
if (bnames) {
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] selinux: simplify evaluate_cond_node()
2020-01-17 8:58 ` [PATCH v2 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
@ 2020-01-17 19:37 ` Stephen Smalley
2020-01-31 3:47 ` Paul Moore
1 sibling, 0 replies; 16+ messages in thread
From: Stephen Smalley @ 2020-01-17 19:37 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/17/20 3:58 AM, Ondrej Mosnacek wrote:
> It never fails, so it can just return void.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
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 | 14 ++++----------
> 3 files changed, 6 insertions(+), 13 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 216ce602a2b5..ff43a35bb874 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2958,11 +2958,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;
> @@ -3015,11 +3012,8 @@ static int security_preserve_bools(struct selinux_state *state,
> 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 (cur = policydb->cond_list; cur; cur = cur->next)
> + evaluate_cond_node(policydb, cur);
>
> out:
> if (bnames) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 1/5] selinux: simplify evaluate_cond_node()
2020-01-17 8:58 ` [PATCH v2 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
2020-01-17 19:37 ` Stephen Smalley
@ 2020-01-31 3:47 ` Paul Moore
1 sibling, 0 replies; 16+ messages in thread
From: Paul Moore @ 2020-01-31 3:47 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> It never fails, so it can just return 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 | 14 ++++----------
> 3 files changed, 6 insertions(+), 13 deletions(-)
Thanks, I've queued this into selinux/next, you'll see if after the
merge window closes.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/5] selinux: convert cond_list to array
2020-01-17 8:58 [PATCH v2 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-01-17 8:58 ` [PATCH v2 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
@ 2020-01-17 8:58 ` Ondrej Mosnacek
2020-01-17 19:38 ` Stephen Smalley
2020-01-31 4:05 ` Paul Moore
2020-01-17 8:58 ` [PATCH v2 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
` (2 subsequent siblings)
4 siblings, 2 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-01-17 8:58 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
While there, also fix signedness of some related variables/parameters.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/include/conditional.h | 6 +--
security/selinux/selinuxfs.c | 4 +-
security/selinux/ss/conditional.c | 54 ++++++++++----------------
security/selinux/ss/conditional.h | 3 +-
security/selinux/ss/policydb.c | 2 +-
security/selinux/ss/policydb.h | 3 +-
security/selinux/ss/services.c | 28 ++++++-------
7 files changed, 43 insertions(+), 57 deletions(-)
diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index 0ab316f61da0..ffb9a33341f8 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -14,12 +14,12 @@
#include "security.h"
int security_get_bools(struct selinux_state *state,
- int *len, char ***names, int **values);
+ u32 *len, char ***names, int **values);
int security_set_bools(struct selinux_state *state,
- int len, int *values);
+ u32 len, int *values);
int security_get_bool_value(struct selinux_state *state,
- int index);
+ u32 index);
#endif
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 79c710911a3c..296ce86e8b1f 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -1327,14 +1327,14 @@ static void sel_remove_entries(struct dentry *de)
static int sel_make_bools(struct selinux_fs_info *fsi)
{
- int i, ret;
+ int ret;
ssize_t len;
struct dentry *dentry = NULL;
struct dentry *dir = fsi->bool_dir;
struct inode *inode = NULL;
struct inode_security_struct *isec;
char **names = NULL, *page;
- int num;
+ u32 i, num;
int *values = NULL;
u32 sid;
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 04593062008d..c8a02c9b23ee 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
p->bool_val_to_struct = NULL;
p->cond_list = NULL;
+ p->cond_list_len = 0;
rc = avtab_init(&p->te_cond_avtab);
if (rc)
@@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
}
cond_av_list_destroy(node->true_list);
cond_av_list_destroy(node->false_list);
- kfree(node);
}
-static void cond_list_destroy(struct cond_node *list)
+static void cond_list_destroy(struct policydb *p)
{
- struct cond_node *next, *cur;
+ u32 i;
- if (list == NULL)
- return;
-
- for (cur = list; cur; cur = next) {
- next = cur->next;
- cond_node_destroy(cur);
- }
+ for (i = 0; i < p->cond_list_len; i++)
+ cond_node_destroy(&p->cond_list[i]);
+ kfree(p->cond_list);
}
void cond_policydb_destroy(struct policydb *p)
{
kfree(p->bool_val_to_struct);
avtab_destroy(&p->te_cond_avtab);
- cond_list_destroy(p->cond_list);
+ cond_list_destroy(p);
}
int cond_init_bool_indexes(struct policydb *p)
@@ -447,7 +443,6 @@ err:
int cond_read_list(struct policydb *p, void *fp)
{
- struct cond_node *node, *last = NULL;
__le32 buf[1];
u32 i, len;
int rc;
@@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
len = le32_to_cpu(buf[0]);
+ p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
+ if (!p->cond_list)
+ return rc;
+
rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
if (rc)
goto err;
for (i = 0; i < len; i++) {
- rc = -ENOMEM;
- node = kzalloc(sizeof(*node), GFP_KERNEL);
- if (!node)
- goto err;
-
- rc = cond_read_node(p, node, fp);
+ rc = cond_read_node(p, &p->cond_list[i], fp);
if (rc)
goto err;
-
- if (i == 0)
- p->cond_list = node;
- else
- last->next = node;
- last = node;
}
+
+ p->cond_list_len = len;
return 0;
err:
- cond_list_destroy(p->cond_list);
+ cond_list_destroy(p);
p->cond_list = NULL;
return rc;
}
@@ -585,23 +575,19 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
return 0;
}
-int cond_write_list(struct policydb *p, struct cond_node *list, void *fp)
+int cond_write_list(struct policydb *p, void *fp)
{
- struct cond_node *cur;
- u32 len;
+ u32 i;
__le32 buf[1];
int rc;
- len = 0;
- for (cur = list; cur != NULL; cur = cur->next)
- len++;
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(p->cond_list_len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur = list; cur != NULL; cur = cur->next) {
- rc = cond_write_node(p, cur, fp);
+ for (i = 0; i < p->cond_list_len; i++) {
+ rc = cond_write_node(p, &p->cond_list[i], fp);
if (rc)
return rc;
}
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index d86ef286ca84..e474bdd3a0ed 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -55,7 +55,6 @@ struct cond_node {
struct cond_expr *expr;
struct cond_av_list *true_list;
struct cond_av_list *false_list;
- struct cond_node *next;
};
int cond_policydb_init(struct policydb *p);
@@ -69,7 +68,7 @@ int cond_index_bool(void *key, void *datum, void *datap);
int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp);
int cond_read_list(struct policydb *p, void *fp);
int cond_write_bool(void *key, void *datum, void *ptr);
-int cond_write_list(struct policydb *p, struct cond_node *list, void *fp);
+int cond_write_list(struct policydb *p, void *fp);
void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
struct av_decision *avd, struct extended_perms *xperms);
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 2aa7f2e1a8e7..8ac9b9ffc83c 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -3483,7 +3483,7 @@ int policydb_write(struct policydb *p, void *fp)
if (rc)
return rc;
- rc = cond_write_list(p, p->cond_list, fp);
+ rc = cond_write_list(p, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index 69b24191fa38..6459616f8487 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -272,8 +272,9 @@ struct policydb {
struct cond_bool_datum **bool_val_to_struct;
/* type enforcement conditional access vectors and transitions */
struct avtab te_cond_avtab;
- /* linked list indexing te_cond_avtab by conditional */
+ /* array indexing te_cond_avtab by conditional */
struct cond_node *cond_list;
+ u32 cond_list_len;
/* role allows */
struct role_allow *role_allow;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index ff43a35bb874..8fc8ec317bb6 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2868,10 +2868,11 @@ out:
}
int security_get_bools(struct selinux_state *state,
- int *len, char ***names, int **values)
+ u32 *len, char ***names, int **values)
{
struct policydb *policydb;
- int i, rc;
+ u32 i;
+ int rc;
if (!selinux_initialized(state)) {
*len = 0;
@@ -2925,12 +2926,11 @@ err:
}
-int security_set_bools(struct selinux_state *state, int len, int *values)
+int security_set_bools(struct selinux_state *state, u32 len, int *values)
{
struct policydb *policydb;
- int i, rc;
- int lenp, seqno = 0;
- struct cond_node *cur;
+ int rc;
+ u32 i, lenp, seqno = 0;
write_lock_irq(&state->ss->policy_rwlock);
@@ -2958,8 +2958,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)
- evaluate_cond_node(policydb, cur);
+ for (i = 0; i < policydb->cond_list_len; i++)
+ evaluate_cond_node(policydb, &policydb->cond_list[i]);
seqno = ++state->ss->latest_granting;
rc = 0;
@@ -2975,11 +2975,11 @@ out:
}
int security_get_bool_value(struct selinux_state *state,
- int index)
+ u32 index)
{
struct policydb *policydb;
int rc;
- int len;
+ u32 len;
read_lock(&state->ss->policy_rwlock);
@@ -2999,10 +2999,10 @@ out:
static int security_preserve_bools(struct selinux_state *state,
struct policydb *policydb)
{
- int rc, nbools = 0, *bvalues = NULL, i;
+ int rc, *bvalues = NULL;
char **bnames = NULL;
struct cond_bool_datum *booldatum;
- struct cond_node *cur;
+ u32 i, nbools = 0;
rc = security_get_bools(state, &nbools, &bnames, &bvalues);
if (rc)
@@ -3012,8 +3012,8 @@ static int security_preserve_bools(struct selinux_state *state,
if (booldatum)
booldatum->state = bvalues[i];
}
- for (cur = policydb->cond_list; cur; cur = cur->next)
- evaluate_cond_node(policydb, cur);
+ for (i = 0; i < policydb->cond_list_len; i++)
+ evaluate_cond_node(policydb, &policydb->cond_list[i]);
out:
if (bnames) {
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 2/5] selinux: convert cond_list to array
2020-01-17 8:58 ` [PATCH v2 2/5] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-01-17 19:38 ` Stephen Smalley
2020-01-31 4:05 ` Paul Moore
1 sibling, 0 replies; 16+ messages in thread
From: Stephen Smalley @ 2020-01-17 19:38 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/17/20 3:58 AM, Ondrej Mosnacek wrote:
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> While there, also fix signedness of some related variables/parameters.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/include/conditional.h | 6 +--
> security/selinux/selinuxfs.c | 4 +-
> security/selinux/ss/conditional.c | 54 ++++++++++----------------
> security/selinux/ss/conditional.h | 3 +-
> security/selinux/ss/policydb.c | 2 +-
> security/selinux/ss/policydb.h | 3 +-
> security/selinux/ss/services.c | 28 ++++++-------
> 7 files changed, 43 insertions(+), 57 deletions(-)
>
> diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
> index 0ab316f61da0..ffb9a33341f8 100644
> --- a/security/selinux/include/conditional.h
> +++ b/security/selinux/include/conditional.h
> @@ -14,12 +14,12 @@
> #include "security.h"
>
> int security_get_bools(struct selinux_state *state,
> - int *len, char ***names, int **values);
> + u32 *len, char ***names, int **values);
>
> int security_set_bools(struct selinux_state *state,
> - int len, int *values);
> + u32 len, int *values);
>
> int security_get_bool_value(struct selinux_state *state,
> - int index);
> + u32 index);
>
> #endif
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 79c710911a3c..296ce86e8b1f 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1327,14 +1327,14 @@ static void sel_remove_entries(struct dentry *de)
>
> static int sel_make_bools(struct selinux_fs_info *fsi)
> {
> - int i, ret;
> + int ret;
> ssize_t len;
> struct dentry *dentry = NULL;
> struct dentry *dir = fsi->bool_dir;
> struct inode *inode = NULL;
> struct inode_security_struct *isec;
> char **names = NULL, *page;
> - int num;
> + u32 i, num;
> int *values = NULL;
> u32 sid;
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 04593062008d..c8a02c9b23ee 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
>
> p->bool_val_to_struct = NULL;
> p->cond_list = NULL;
> + p->cond_list_len = 0;
>
> rc = avtab_init(&p->te_cond_avtab);
> if (rc)
> @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
> }
> cond_av_list_destroy(node->true_list);
> cond_av_list_destroy(node->false_list);
> - kfree(node);
> }
>
> -static void cond_list_destroy(struct cond_node *list)
> +static void cond_list_destroy(struct policydb *p)
> {
> - struct cond_node *next, *cur;
> + u32 i;
>
> - if (list == NULL)
> - return;
> -
> - for (cur = list; cur; cur = next) {
> - next = cur->next;
> - cond_node_destroy(cur);
> - }
> + for (i = 0; i < p->cond_list_len; i++)
> + cond_node_destroy(&p->cond_list[i]);
> + kfree(p->cond_list);
> }
>
> void cond_policydb_destroy(struct policydb *p)
> {
> kfree(p->bool_val_to_struct);
> avtab_destroy(&p->te_cond_avtab);
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> }
>
> int cond_init_bool_indexes(struct policydb *p)
> @@ -447,7 +443,6 @@ err:
>
> int cond_read_list(struct policydb *p, void *fp)
> {
> - struct cond_node *node, *last = NULL;
> __le32 buf[1];
> u32 i, len;
> int rc;
> @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
>
> len = le32_to_cpu(buf[0]);
>
> + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> + if (!p->cond_list)
> + return rc;
> +
> rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
> if (rc)
> goto err;
>
> for (i = 0; i < len; i++) {
> - rc = -ENOMEM;
> - node = kzalloc(sizeof(*node), GFP_KERNEL);
> - if (!node)
> - goto err;
> -
> - rc = cond_read_node(p, node, fp);
> + rc = cond_read_node(p, &p->cond_list[i], fp);
> if (rc)
> goto err;
> -
> - if (i == 0)
> - p->cond_list = node;
> - else
> - last->next = node;
> - last = node;
> }
> +
> + p->cond_list_len = len;
> return 0;
> err:
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> p->cond_list = NULL;
> return rc;
> }
> @@ -585,23 +575,19 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
> return 0;
> }
>
> -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp)
> +int cond_write_list(struct policydb *p, void *fp)
> {
> - struct cond_node *cur;
> - u32 len;
> + u32 i;
> __le32 buf[1];
> int rc;
>
> - len = 0;
> - for (cur = list; cur != NULL; cur = cur->next)
> - len++;
> - buf[0] = cpu_to_le32(len);
> + buf[0] = cpu_to_le32(p->cond_list_len);
> rc = put_entry(buf, sizeof(u32), 1, fp);
> if (rc)
> return rc;
>
> - for (cur = list; cur != NULL; cur = cur->next) {
> - rc = cond_write_node(p, cur, fp);
> + for (i = 0; i < p->cond_list_len; i++) {
> + rc = cond_write_node(p, &p->cond_list[i], fp);
> if (rc)
> return rc;
> }
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index d86ef286ca84..e474bdd3a0ed 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -55,7 +55,6 @@ struct cond_node {
> struct cond_expr *expr;
> struct cond_av_list *true_list;
> struct cond_av_list *false_list;
> - struct cond_node *next;
> };
>
> int cond_policydb_init(struct policydb *p);
> @@ -69,7 +68,7 @@ int cond_index_bool(void *key, void *datum, void *datap);
> int cond_read_bool(struct policydb *p, struct hashtab *h, void *fp);
> int cond_read_list(struct policydb *p, void *fp);
> int cond_write_bool(void *key, void *datum, void *ptr);
> -int cond_write_list(struct policydb *p, struct cond_node *list, void *fp);
> +int cond_write_list(struct policydb *p, void *fp);
>
> void cond_compute_av(struct avtab *ctab, struct avtab_key *key,
> struct av_decision *avd, struct extended_perms *xperms);
> diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> index 2aa7f2e1a8e7..8ac9b9ffc83c 100644
> --- a/security/selinux/ss/policydb.c
> +++ b/security/selinux/ss/policydb.c
> @@ -3483,7 +3483,7 @@ int policydb_write(struct policydb *p, void *fp)
> if (rc)
> return rc;
>
> - rc = cond_write_list(p, p->cond_list, fp);
> + rc = cond_write_list(p, fp);
> if (rc)
> return rc;
>
> diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
> index 69b24191fa38..6459616f8487 100644
> --- a/security/selinux/ss/policydb.h
> +++ b/security/selinux/ss/policydb.h
> @@ -272,8 +272,9 @@ struct policydb {
> struct cond_bool_datum **bool_val_to_struct;
> /* type enforcement conditional access vectors and transitions */
> struct avtab te_cond_avtab;
> - /* linked list indexing te_cond_avtab by conditional */
> + /* array indexing te_cond_avtab by conditional */
> struct cond_node *cond_list;
> + u32 cond_list_len;
>
> /* role allows */
> struct role_allow *role_allow;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index ff43a35bb874..8fc8ec317bb6 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2868,10 +2868,11 @@ out:
> }
>
> int security_get_bools(struct selinux_state *state,
> - int *len, char ***names, int **values)
> + u32 *len, char ***names, int **values)
> {
> struct policydb *policydb;
> - int i, rc;
> + u32 i;
> + int rc;
>
> if (!selinux_initialized(state)) {
> *len = 0;
> @@ -2925,12 +2926,11 @@ err:
> }
>
>
> -int security_set_bools(struct selinux_state *state, int len, int *values)
> +int security_set_bools(struct selinux_state *state, u32 len, int *values)
> {
> struct policydb *policydb;
> - int i, rc;
> - int lenp, seqno = 0;
> - struct cond_node *cur;
> + int rc;
> + u32 i, lenp, seqno = 0;
>
> write_lock_irq(&state->ss->policy_rwlock);
>
> @@ -2958,8 +2958,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)
> - evaluate_cond_node(policydb, cur);
> + for (i = 0; i < policydb->cond_list_len; i++)
> + evaluate_cond_node(policydb, &policydb->cond_list[i]);
>
> seqno = ++state->ss->latest_granting;
> rc = 0;
> @@ -2975,11 +2975,11 @@ out:
> }
>
> int security_get_bool_value(struct selinux_state *state,
> - int index)
> + u32 index)
> {
> struct policydb *policydb;
> int rc;
> - int len;
> + u32 len;
>
> read_lock(&state->ss->policy_rwlock);
>
> @@ -2999,10 +2999,10 @@ out:
> static int security_preserve_bools(struct selinux_state *state,
> struct policydb *policydb)
> {
> - int rc, nbools = 0, *bvalues = NULL, i;
> + int rc, *bvalues = NULL;
> char **bnames = NULL;
> struct cond_bool_datum *booldatum;
> - struct cond_node *cur;
> + u32 i, nbools = 0;
>
> rc = security_get_bools(state, &nbools, &bnames, &bvalues);
> if (rc)
> @@ -3012,8 +3012,8 @@ static int security_preserve_bools(struct selinux_state *state,
> if (booldatum)
> booldatum->state = bvalues[i];
> }
> - for (cur = policydb->cond_list; cur; cur = cur->next)
> - evaluate_cond_node(policydb, cur);
> + for (i = 0; i < policydb->cond_list_len; i++)
> + evaluate_cond_node(policydb, &policydb->cond_list[i]);
>
> out:
> if (bnames) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 2/5] selinux: convert cond_list to array
2020-01-17 8:58 ` [PATCH v2 2/5] selinux: convert cond_list to array Ondrej Mosnacek
2020-01-17 19:38 ` Stephen Smalley
@ 2020-01-31 4:05 ` Paul Moore
2020-02-03 9:05 ` Ondrej Mosnacek
1 sibling, 1 reply; 16+ messages in thread
From: Paul Moore @ 2020-01-31 4:05 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> While there, also fix signedness of some related variables/parameters.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/include/conditional.h | 6 +--
> security/selinux/selinuxfs.c | 4 +-
> security/selinux/ss/conditional.c | 54 ++++++++++----------------
> security/selinux/ss/conditional.h | 3 +-
> security/selinux/ss/policydb.c | 2 +-
> security/selinux/ss/policydb.h | 3 +-
> security/selinux/ss/services.c | 28 ++++++-------
> 7 files changed, 43 insertions(+), 57 deletions(-)
>
> diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
> index 0ab316f61da0..ffb9a33341f8 100644
> --- a/security/selinux/include/conditional.h
> +++ b/security/selinux/include/conditional.h
> @@ -14,12 +14,12 @@
> #include "security.h"
>
> int security_get_bools(struct selinux_state *state,
> - int *len, char ***names, int **values);
> + u32 *len, char ***names, int **values);
>
> int security_set_bools(struct selinux_state *state,
> - int len, int *values);
> + u32 len, int *values);
In the future, if you're making changes like this, you might as well
put it all on one line (it fits under 80 chars).
> int security_get_bool_value(struct selinux_state *state,
> - int index);
> + u32 index);
Same here.
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 04593062008d..c8a02c9b23ee 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
>
> p->bool_val_to_struct = NULL;
> p->cond_list = NULL;
> + p->cond_list_len = 0;
>
> rc = avtab_init(&p->te_cond_avtab);
> if (rc)
> @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
> }
> cond_av_list_destroy(node->true_list);
> cond_av_list_destroy(node->false_list);
> - kfree(node);
> }
>
> -static void cond_list_destroy(struct cond_node *list)
> +static void cond_list_destroy(struct policydb *p)
> {
> - struct cond_node *next, *cur;
> + u32 i;
>
> - if (list == NULL)
> - return;
> -
> - for (cur = list; cur; cur = next) {
> - next = cur->next;
> - cond_node_destroy(cur);
> - }
> + for (i = 0; i < p->cond_list_len; i++)
> + cond_node_destroy(&p->cond_list[i]);
> + kfree(p->cond_list);
> }
>
> void cond_policydb_destroy(struct policydb *p)
> {
> kfree(p->bool_val_to_struct);
> avtab_destroy(&p->te_cond_avtab);
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> }
>
> int cond_init_bool_indexes(struct policydb *p)
> @@ -447,7 +443,6 @@ err:
>
> int cond_read_list(struct policydb *p, void *fp)
> {
> - struct cond_node *node, *last = NULL;
> __le32 buf[1];
> u32 i, len;
> int rc;
> @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
>
> len = le32_to_cpu(buf[0]);
>
> + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> + if (!p->cond_list)
> + return rc;
> +
> rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
> if (rc)
> goto err;
>
> for (i = 0; i < len; i++) {
> - rc = -ENOMEM;
> - node = kzalloc(sizeof(*node), GFP_KERNEL);
> - if (!node)
> - goto err;
> -
> - rc = cond_read_node(p, node, fp);
> + rc = cond_read_node(p, &p->cond_list[i], fp);
> if (rc)
> goto err;
> -
> - if (i == 0)
> - p->cond_list = node;
> - else
> - last->next = node;
> - last = node;
> }
> +
> + p->cond_list_len = len;
Hmmm. Since we don't update p->cond_list_len until here, if we fail
in the for-loop above and end up jumping to "err" we might not cleanup
all the nodes, right?
> return 0;
> err:
> - cond_list_destroy(p->cond_list);
> + cond_list_destroy(p);
> p->cond_list = NULL;
> return rc;
> }
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 2/5] selinux: convert cond_list to array
2020-01-31 4:05 ` Paul Moore
@ 2020-02-03 9:05 ` Ondrej Mosnacek
0 siblings, 0 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 9:05 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Stephen Smalley
On Fri, Jan 31, 2020 at 5:05 AM Paul Moore <paul@paul-moore.com> wrote:
> On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Since it is fixed-size after allocation and we know the size beforehand,
> > using a plain old array is simpler and more efficient.
> >
> > While there, also fix signedness of some related variables/parameters.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > security/selinux/include/conditional.h | 6 +--
> > security/selinux/selinuxfs.c | 4 +-
> > security/selinux/ss/conditional.c | 54 ++++++++++----------------
> > security/selinux/ss/conditional.h | 3 +-
> > security/selinux/ss/policydb.c | 2 +-
> > security/selinux/ss/policydb.h | 3 +-
> > security/selinux/ss/services.c | 28 ++++++-------
> > 7 files changed, 43 insertions(+), 57 deletions(-)
> >
> > diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
> > index 0ab316f61da0..ffb9a33341f8 100644
> > --- a/security/selinux/include/conditional.h
> > +++ b/security/selinux/include/conditional.h
> > @@ -14,12 +14,12 @@
> > #include "security.h"
> >
> > int security_get_bools(struct selinux_state *state,
> > - int *len, char ***names, int **values);
> > + u32 *len, char ***names, int **values);
> >
> > int security_set_bools(struct selinux_state *state,
> > - int len, int *values);
> > + u32 len, int *values);
>
> In the future, if you're making changes like this, you might as well
> put it all on one line (it fits under 80 chars).
Good point, will do that in v3.
>
> > int security_get_bool_value(struct selinux_state *state,
> > - int index);
> > + u32 index);
>
> Same here.
>
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index 04593062008d..c8a02c9b23ee 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -119,6 +119,7 @@ int cond_policydb_init(struct policydb *p)
> >
> > p->bool_val_to_struct = NULL;
> > p->cond_list = NULL;
> > + p->cond_list_len = 0;
> >
> > rc = avtab_init(&p->te_cond_avtab);
> > if (rc)
> > @@ -147,27 +148,22 @@ static void cond_node_destroy(struct cond_node *node)
> > }
> > cond_av_list_destroy(node->true_list);
> > cond_av_list_destroy(node->false_list);
> > - kfree(node);
> > }
> >
> > -static void cond_list_destroy(struct cond_node *list)
> > +static void cond_list_destroy(struct policydb *p)
> > {
> > - struct cond_node *next, *cur;
> > + u32 i;
> >
> > - if (list == NULL)
> > - return;
> > -
> > - for (cur = list; cur; cur = next) {
> > - next = cur->next;
> > - cond_node_destroy(cur);
> > - }
> > + for (i = 0; i < p->cond_list_len; i++)
> > + cond_node_destroy(&p->cond_list[i]);
> > + kfree(p->cond_list);
> > }
> >
> > void cond_policydb_destroy(struct policydb *p)
> > {
> > kfree(p->bool_val_to_struct);
> > avtab_destroy(&p->te_cond_avtab);
> > - cond_list_destroy(p->cond_list);
> > + cond_list_destroy(p);
> > }
> >
> > int cond_init_bool_indexes(struct policydb *p)
> > @@ -447,7 +443,6 @@ err:
> >
> > int cond_read_list(struct policydb *p, void *fp)
> > {
> > - struct cond_node *node, *last = NULL;
> > __le32 buf[1];
> > u32 i, len;
> > int rc;
> > @@ -458,29 +453,24 @@ int cond_read_list(struct policydb *p, void *fp)
> >
> > len = le32_to_cpu(buf[0]);
> >
> > + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL);
> > + if (!p->cond_list)
> > + return rc;
> > +
> > rc = avtab_alloc(&(p->te_cond_avtab), p->te_avtab.nel);
> > if (rc)
> > goto err;
> >
> > for (i = 0; i < len; i++) {
> > - rc = -ENOMEM;
> > - node = kzalloc(sizeof(*node), GFP_KERNEL);
> > - if (!node)
> > - goto err;
> > -
> > - rc = cond_read_node(p, node, fp);
> > + rc = cond_read_node(p, &p->cond_list[i], fp);
> > if (rc)
> > goto err;
> > -
> > - if (i == 0)
> > - p->cond_list = node;
> > - else
> > - last->next = node;
> > - last = node;
> > }
> > +
> > + p->cond_list_len = len;
>
> Hmmm. Since we don't update p->cond_list_len until here, if we fail
> in the for-loop above and end up jumping to "err" we might not cleanup
> all the nodes, right?
You're right. I'll fix it by moving the p->cond_list_len assignment
before the for-loop - since the array is zero-initialized by
kcalloc(), cond_node_destroy() can be called safely also on unread
items. (That is a bit inefficient, but I see little value in
micro-optimizing the error path.)
>
> > return 0;
> > err:
> > - cond_list_destroy(p->cond_list);
> > + cond_list_destroy(p);
> > p->cond_list = NULL;
> > return rc;
> > }
>
> --
> paul moore
> www.paul-moore.com
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/5] selinux: convert cond_av_list to array
2020-01-17 8:58 [PATCH v2 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-01-17 8:58 ` [PATCH v2 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
2020-01-17 8:58 ` [PATCH v2 2/5] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-01-17 8:58 ` Ondrej Mosnacek
2020-01-31 4:17 ` Paul Moore
2020-01-17 8:58 ` [PATCH v2 4/5] selinux: convert cond_expr " Ondrej Mosnacek
2020-01-17 8:58 ` [PATCH v2 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
4 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-01-17 8:58 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/ss/conditional.c | 125 ++++++++++++------------------
security/selinux/ss/conditional.h | 8 +-
2 files changed, 53 insertions(+), 80 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index c8a02c9b23ee..b847fd2a6a51 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -87,8 +87,9 @@ static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
*/
void evaluate_cond_node(struct policydb *p, struct cond_node *node)
{
+ struct avtab_node *avnode;
int new_state;
- struct cond_av_list *cur;
+ u32 i;
new_state = cond_evaluate_expr(p, node->expr);
if (new_state != node->cur_state) {
@@ -96,19 +97,21 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
if (new_state == -1)
pr_err("SELinux: expression result was undefined - disabling all rules.\n");
/* turn the rules on or off */
- for (cur = node->true_list; cur; cur = cur->next) {
+ for (i = 0; i < node->true_list.len; i++) {
+ avnode = node->true_list.nodes[i];
if (new_state <= 0)
- cur->node->key.specified &= ~AVTAB_ENABLED;
+ avnode->key.specified &= ~AVTAB_ENABLED;
else
- cur->node->key.specified |= AVTAB_ENABLED;
+ avnode->key.specified |= AVTAB_ENABLED;
}
- for (cur = node->false_list; cur; cur = cur->next) {
+ for (i = 0; i < node->false_list.len; i++) {
+ avnode = node->false_list.nodes[i];
/* -1 or 1 */
if (new_state)
- cur->node->key.specified &= ~AVTAB_ENABLED;
+ avnode->key.specified &= ~AVTAB_ENABLED;
else
- cur->node->key.specified |= AVTAB_ENABLED;
+ avnode->key.specified |= AVTAB_ENABLED;
}
}
}
@@ -128,16 +131,6 @@ int cond_policydb_init(struct policydb *p)
return 0;
}
-static void cond_av_list_destroy(struct cond_av_list *list)
-{
- struct cond_av_list *cur, *next;
- for (cur = list; cur; cur = next) {
- next = cur->next;
- /* the avtab_ptr_t node is destroy by the avtab */
- kfree(cur);
- }
-}
-
static void cond_node_destroy(struct cond_node *node)
{
struct cond_expr *cur_expr, *next_expr;
@@ -146,8 +139,9 @@ static void cond_node_destroy(struct cond_node *node)
next_expr = cur_expr->next;
kfree(cur_expr);
}
- cond_av_list_destroy(node->true_list);
- cond_av_list_destroy(node->false_list);
+ /* the avtab_ptr_t nodes are destroyed by the avtab */
+ kfree(node->true_list.nodes);
+ kfree(node->false_list.nodes);
}
static void cond_list_destroy(struct policydb *p)
@@ -255,19 +249,17 @@ err:
struct cond_insertf_data {
struct policydb *p;
+ struct avtab_node **dst;
struct cond_av_list *other;
- struct cond_av_list *head;
- struct cond_av_list *tail;
};
static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
{
struct cond_insertf_data *data = ptr;
struct policydb *p = data->p;
- struct cond_av_list *other = data->other, *list, *cur;
struct avtab_node *node_ptr;
- u8 found;
- int rc = -EINVAL;
+ u32 i;
+ bool found;
/*
* For type rules we have to make certain there aren't any
@@ -277,7 +269,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
if (k->specified & AVTAB_TYPE) {
if (avtab_search(&p->te_avtab, k)) {
pr_err("SELinux: type rule already exists outside of a conditional.\n");
- goto err;
+ return -EINVAL;
}
/*
* If we are reading the false list other will be a pointer to
@@ -287,29 +279,29 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
* If we are reading the true list (other == NULL) there shouldn't
* be any other entries.
*/
- if (other) {
+ if (data->other) {
node_ptr = avtab_search_node(&p->te_cond_avtab, k);
if (node_ptr) {
if (avtab_search_node_next(node_ptr, k->specified)) {
pr_err("SELinux: too many conflicting type rules.\n");
- goto err;
+ return -EINVAL;
}
- found = 0;
- for (cur = other; cur; cur = cur->next) {
- if (cur->node == node_ptr) {
- found = 1;
+ found = false;
+ for (i = 0; i < data->other->len; i++) {
+ if (data->other->nodes[i] == node_ptr) {
+ found = true;
break;
}
}
if (!found) {
pr_err("SELinux: conflicting type rules.\n");
- goto err;
+ return -EINVAL;
}
}
} else {
if (avtab_search(&p->te_cond_avtab, k)) {
pr_err("SELinux: conflicting type rules when adding type rule for true.\n");
- goto err;
+ return -EINVAL;
}
}
}
@@ -317,39 +309,22 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
node_ptr = avtab_insert_nonunique(&p->te_cond_avtab, k, d);
if (!node_ptr) {
pr_err("SELinux: could not insert rule.\n");
- rc = -ENOMEM;
- goto err;
- }
-
- list = kzalloc(sizeof(*list), GFP_KERNEL);
- if (!list) {
- rc = -ENOMEM;
- goto err;
+ return -ENOMEM;
}
- list->node = node_ptr;
- if (!data->head)
- data->head = list;
- else
- data->tail->next = list;
- data->tail = list;
+ *data->dst = node_ptr;
return 0;
-
-err:
- cond_av_list_destroy(data->head);
- data->head = NULL;
- return rc;
}
-static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list **ret_list, struct cond_av_list *other)
+static int cond_read_av_list(struct policydb *p, void *fp,
+ struct cond_av_list *list,
+ struct cond_av_list *other)
{
- int i, rc;
+ int rc;
__le32 buf[1];
- u32 len;
+ u32 i, len;
struct cond_insertf_data data;
- *ret_list = NULL;
-
rc = next_entry(buf, fp, sizeof(u32));
if (rc)
return rc;
@@ -358,18 +333,24 @@ static int cond_read_av_list(struct policydb *p, void *fp, struct cond_av_list *
if (len == 0)
return 0;
+ list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL);
+ if (!list->nodes)
+ return -ENOMEM;
+
data.p = p;
data.other = other;
- data.head = NULL;
- data.tail = NULL;
for (i = 0; i < len; i++) {
+ data.dst = &list->nodes[i];
rc = avtab_read_item(&p->te_cond_avtab, fp, p, cond_insertf,
&data);
- if (rc)
+ if (rc) {
+ kfree(list->nodes);
+ list->nodes = NULL;
return rc;
+ }
}
- *ret_list = data.head;
+ list->len = len;
return 0;
}
@@ -432,7 +413,7 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
rc = cond_read_av_list(p, fp, &node->true_list, NULL);
if (rc)
goto err;
- rc = cond_read_av_list(p, fp, &node->false_list, node->true_list);
+ rc = cond_read_av_list(p, fp, &node->false_list, &node->true_list);
if (rc)
goto err;
return 0;
@@ -511,24 +492,16 @@ static int cond_write_av_list(struct policydb *p,
struct cond_av_list *list, struct policy_file *fp)
{
__le32 buf[1];
- struct cond_av_list *cur_list;
- u32 len;
+ u32 i;
int rc;
- len = 0;
- for (cur_list = list; cur_list != NULL; cur_list = cur_list->next)
- len++;
-
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(list->len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- if (len == 0)
- return 0;
-
- for (cur_list = list; cur_list != NULL; cur_list = cur_list->next) {
- rc = avtab_write_item(p, cur_list->node, fp);
+ for (i = 0; i < list->len; i++) {
+ rc = avtab_write_item(p, list->nodes[i], fp);
if (rc)
return rc;
}
@@ -565,10 +538,10 @@ static int cond_write_node(struct policydb *p, struct cond_node *node,
return rc;
}
- rc = cond_write_av_list(p, node->true_list, fp);
+ rc = cond_write_av_list(p, &node->true_list, fp);
if (rc)
return rc;
- rc = cond_write_av_list(p, node->false_list, fp);
+ rc = cond_write_av_list(p, &node->false_list, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index e474bdd3a0ed..5f97f678440e 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -39,8 +39,8 @@ struct cond_expr {
* struct is for that list.
*/
struct cond_av_list {
- struct avtab_node *node;
- struct cond_av_list *next;
+ struct avtab_node **nodes;
+ u32 len;
};
/*
@@ -53,8 +53,8 @@ struct cond_av_list {
struct cond_node {
int cur_state;
struct cond_expr *expr;
- struct cond_av_list *true_list;
- struct cond_av_list *false_list;
+ struct cond_av_list true_list;
+ struct cond_av_list false_list;
};
int cond_policydb_init(struct policydb *p);
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 3/5] selinux: convert cond_av_list to array
2020-01-17 8:58 ` [PATCH v2 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-01-31 4:17 ` Paul Moore
2020-02-03 9:13 ` Ondrej Mosnacek
0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2020-01-31 4:17 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 125 ++++++++++++------------------
> security/selinux/ss/conditional.h | 8 +-
> 2 files changed, 53 insertions(+), 80 deletions(-)
...
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index c8a02c9b23ee..b847fd2a6a51 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -255,19 +249,17 @@ err:
>
> struct cond_insertf_data {
> struct policydb *p;
> + struct avtab_node **dst;
> struct cond_av_list *other;
> - struct cond_av_list *head;
> - struct cond_av_list *tail;
> };
>
> static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
> {
> struct cond_insertf_data *data = ptr;
> struct policydb *p = data->p;
> - struct cond_av_list *other = data->other, *list, *cur;
> struct avtab_node *node_ptr;
> - u8 found;
> - int rc = -EINVAL;
> + u32 i;
> + bool found;
>
> /*
> * For type rules we have to make certain there aren't any
> @@ -277,7 +269,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> if (k->specified & AVTAB_TYPE) {
> if (avtab_search(&p->te_avtab, k)) {
> pr_err("SELinux: type rule already exists outside of a conditional.\n");
> - goto err;
> + return -EINVAL;
> }
> /*
> * If we are reading the false list other will be a pointer to
> @@ -287,29 +279,29 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> * If we are reading the true list (other == NULL) there shouldn't
> * be any other entries.
> */
> - if (other) {
> + if (data->other) {
> node_ptr = avtab_search_node(&p->te_cond_avtab, k);
> if (node_ptr) {
> if (avtab_search_node_next(node_ptr, k->specified)) {
> pr_err("SELinux: too many conflicting type rules.\n");
> - goto err;
> + return -EINVAL;
> }
> - found = 0;
> - for (cur = other; cur; cur = cur->next) {
> - if (cur->node == node_ptr) {
> - found = 1;
> + found = false;
> + for (i = 0; i < data->other->len; i++) {
> + if (data->other->nodes[i] == node_ptr) {
> + found = true;
> break;
> }
> }
Can you explain why you got rid of the "other" variable in this
function? It seems like it would be nice in the loop above.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 3/5] selinux: convert cond_av_list to array
2020-01-31 4:17 ` Paul Moore
@ 2020-02-03 9:13 ` Ondrej Mosnacek
0 siblings, 0 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 9:13 UTC (permalink / raw)
To: Paul Moore; +Cc: SElinux list, Stephen Smalley
On Fri, Jan 31, 2020 at 5:17 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > Since it is fixed-size after allocation and we know the size beforehand,
> > using a plain old array is simpler and more efficient.
> >
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> > ---
> > security/selinux/ss/conditional.c | 125 ++++++++++++------------------
> > security/selinux/ss/conditional.h | 8 +-
> > 2 files changed, 53 insertions(+), 80 deletions(-)
>
> ...
>
> > diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> > index c8a02c9b23ee..b847fd2a6a51 100644
> > --- a/security/selinux/ss/conditional.c
> > +++ b/security/selinux/ss/conditional.c
> > @@ -255,19 +249,17 @@ err:
> >
> > struct cond_insertf_data {
> > struct policydb *p;
> > + struct avtab_node **dst;
> > struct cond_av_list *other;
> > - struct cond_av_list *head;
> > - struct cond_av_list *tail;
> > };
> >
> > static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
> > {
> > struct cond_insertf_data *data = ptr;
> > struct policydb *p = data->p;
> > - struct cond_av_list *other = data->other, *list, *cur;
> > struct avtab_node *node_ptr;
> > - u8 found;
> > - int rc = -EINVAL;
> > + u32 i;
> > + bool found;
> >
> > /*
> > * For type rules we have to make certain there aren't any
> > @@ -277,7 +269,7 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> > if (k->specified & AVTAB_TYPE) {
> > if (avtab_search(&p->te_avtab, k)) {
> > pr_err("SELinux: type rule already exists outside of a conditional.\n");
> > - goto err;
> > + return -EINVAL;
> > }
> > /*
> > * If we are reading the false list other will be a pointer to
> > @@ -287,29 +279,29 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
> > * If we are reading the true list (other == NULL) there shouldn't
> > * be any other entries.
> > */
> > - if (other) {
> > + if (data->other) {
> > node_ptr = avtab_search_node(&p->te_cond_avtab, k);
> > if (node_ptr) {
> > if (avtab_search_node_next(node_ptr, k->specified)) {
> > pr_err("SELinux: too many conflicting type rules.\n");
> > - goto err;
> > + return -EINVAL;
> > }
> > - found = 0;
> > - for (cur = other; cur; cur = cur->next) {
> > - if (cur->node == node_ptr) {
> > - found = 1;
> > + found = false;
> > + for (i = 0; i < data->other->len; i++) {
> > + if (data->other->nodes[i] == node_ptr) {
> > + found = true;
> > break;
> > }
> > }
>
> Can you explain why you got rid of the "other" variable in this
> function? It seems like it would be nice in the loop above.
It's probably a result of incremental edits... I agree it looks a bit
nicer with it and it also makes the diff simpler. I'll put it back in
v3.
--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 4/5] selinux: convert cond_expr to array
2020-01-17 8:58 [PATCH v2 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (2 preceding siblings ...)
2020-01-17 8:58 ` [PATCH v2 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-01-17 8:58 ` Ondrej Mosnacek
2020-01-31 4:22 ` Paul Moore
2020-01-17 8:58 ` [PATCH v2 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
4 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-01-17 8:58 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Since it is fixed-size after allocation and we know the size beforehand,
using a plain old array is simpler and more efficient.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
---
security/selinux/ss/conditional.c | 62 ++++++++++++-------------------
security/selinux/ss/conditional.h | 14 ++++---
2 files changed, 33 insertions(+), 43 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index b847fd2a6a51..8f9f2f3c86a0 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -23,18 +23,19 @@
*/
static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr)
{
-
- struct cond_expr *cur;
+ u32 i;
int s[COND_EXPR_MAXDEPTH];
int sp = -1;
- for (cur = expr; cur; cur = cur->next) {
- switch (cur->expr_type) {
+ for (i = 0; i < expr->len; i++) {
+ struct cond_expr_node *node = &expr->nodes[i];
+
+ switch (node->expr_type) {
case COND_BOOL:
if (sp == (COND_EXPR_MAXDEPTH - 1))
return -1;
sp++;
- s[sp] = p->bool_val_to_struct[cur->bool - 1]->state;
+ s[sp] = p->bool_val_to_struct[node->bool - 1]->state;
break;
case COND_NOT:
if (sp < 0)
@@ -91,7 +92,7 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
int new_state;
u32 i;
- new_state = cond_evaluate_expr(p, node->expr);
+ new_state = cond_evaluate_expr(p, &node->expr);
if (new_state != node->cur_state) {
node->cur_state = new_state;
if (new_state == -1)
@@ -133,12 +134,7 @@ int cond_policydb_init(struct policydb *p)
static void cond_node_destroy(struct cond_node *node)
{
- struct cond_expr *cur_expr, *next_expr;
-
- for (cur_expr = node->expr; cur_expr; cur_expr = next_expr) {
- next_expr = cur_expr->next;
- kfree(cur_expr);
- }
+ kfree(node->expr.nodes);
/* the avtab_ptr_t nodes are destroyed by the avtab */
kfree(node->true_list.nodes);
kfree(node->false_list.nodes);
@@ -354,7 +350,7 @@ static int cond_read_av_list(struct policydb *p, void *fp,
return 0;
}
-static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
+static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr)
{
if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) {
pr_err("SELinux: conditional expressions uses unknown operator.\n");
@@ -371,43 +367,37 @@ static int expr_isvalid(struct policydb *p, struct cond_expr *expr)
static int cond_read_node(struct policydb *p, struct cond_node *node, void *fp)
{
__le32 buf[2];
- u32 len, i;
+ u32 i, len;
int rc;
- struct cond_expr *expr = NULL, *last = NULL;
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
- goto err;
+ return rc;
node->cur_state = le32_to_cpu(buf[0]);
/* expr */
len = le32_to_cpu(buf[1]);
+ node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL);
+ if (!node->expr.nodes)
+ return -ENOMEM;
+
+ node->expr.len = len;
for (i = 0; i < len; i++) {
+ struct cond_expr_node *expr = &node->expr.nodes[i];
+
rc = next_entry(buf, fp, sizeof(u32) * 2);
if (rc)
goto err;
- rc = -ENOMEM;
- expr = kzalloc(sizeof(*expr), GFP_KERNEL);
- if (!expr)
- goto err;
-
expr->expr_type = le32_to_cpu(buf[0]);
expr->bool = le32_to_cpu(buf[1]);
- if (!expr_isvalid(p, expr)) {
+ if (!expr_node_isvalid(p, expr)) {
rc = -EINVAL;
- kfree(expr);
goto err;
}
-
- if (i == 0)
- node->expr = expr;
- else
- last->next = expr;
- last = expr;
}
rc = cond_read_av_list(p, fp, &node->true_list, NULL);
@@ -512,27 +502,23 @@ static int cond_write_av_list(struct policydb *p,
static int cond_write_node(struct policydb *p, struct cond_node *node,
struct policy_file *fp)
{
- struct cond_expr *cur_expr;
__le32 buf[2];
int rc;
- u32 len = 0;
+ u32 i;
buf[0] = cpu_to_le32(node->cur_state);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next)
- len++;
-
- buf[0] = cpu_to_le32(len);
+ buf[0] = cpu_to_le32(node->expr.len);
rc = put_entry(buf, sizeof(u32), 1, fp);
if (rc)
return rc;
- for (cur_expr = node->expr; cur_expr != NULL; cur_expr = cur_expr->next) {
- buf[0] = cpu_to_le32(cur_expr->expr_type);
- buf[1] = cpu_to_le32(cur_expr->bool);
+ for (i = 0; i < node->expr.len; i++) {
+ buf[0] = cpu_to_le32(node->expr.nodes[i].expr_type);
+ buf[1] = cpu_to_le32(node->expr.nodes[i].bool);
rc = put_entry(buf, sizeof(u32), 2, fp);
if (rc)
return rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 5f97f678440e..4677c6ff7450 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -19,7 +19,7 @@
* A conditional expression is a list of operators and operands
* in reverse polish notation.
*/
-struct cond_expr {
+struct cond_expr_node {
#define COND_BOOL 1 /* plain bool */
#define COND_NOT 2 /* !bool */
#define COND_OR 3 /* bool || bool */
@@ -28,9 +28,13 @@ struct cond_expr {
#define COND_EQ 6 /* bool == bool */
#define COND_NEQ 7 /* bool != bool */
#define COND_LAST COND_NEQ
- __u32 expr_type;
- __u32 bool;
- struct cond_expr *next;
+ u32 expr_type;
+ u32 bool;
+};
+
+struct cond_expr {
+ struct cond_expr_node *nodes;
+ u32 len;
};
/*
@@ -52,7 +56,7 @@ struct cond_av_list {
*/
struct cond_node {
int cur_state;
- struct cond_expr *expr;
+ struct cond_expr expr;
struct cond_av_list true_list;
struct cond_av_list false_list;
};
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 4/5] selinux: convert cond_expr to array
2020-01-17 8:58 ` [PATCH v2 4/5] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-01-31 4:22 ` Paul Moore
0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2020-01-31 4:22 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Since it is fixed-size after allocation and we know the size beforehand,
> using a plain old array is simpler and more efficient.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 62 ++++++++++++-------------------
> security/selinux/ss/conditional.h | 14 ++++---
> 2 files changed, 33 insertions(+), 43 deletions(-)
This patch looks fine, but since I think you may need to respin at
least one of the earlier patches I'm going to hold off on merging this
for now.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 5/5] selinux: generalize evaluate_cond_node()
2020-01-17 8:58 [PATCH v2 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (3 preceding siblings ...)
2020-01-17 8:58 ` [PATCH v2 4/5] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-01-17 8:58 ` Ondrej Mosnacek
2020-01-17 19:39 ` Stephen Smalley
2020-01-31 4:24 ` Paul Moore
4 siblings, 2 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2020-01-17 8:58 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
Both callers iterate the cond_list and call it for each node - turn it
into evaluate_cond_nodes(), which does the iteration for them.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/conditional.c | 10 +++++++++-
security/selinux/ss/conditional.h | 2 +-
security/selinux/ss/services.c | 6 ++----
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 8f9f2f3c86a0..ad709ccea036 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -86,7 +86,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.
*/
-void evaluate_cond_node(struct policydb *p, struct cond_node *node)
+static void evaluate_cond_node(struct policydb *p, struct cond_node *node)
{
struct avtab_node *avnode;
int new_state;
@@ -117,6 +117,14 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
}
}
+void evaluate_cond_nodes(struct policydb *p)
+{
+ u32 i;
+
+ for (i = 0; i < p->cond_list_len; i++)
+ evaluate_cond_node(p, &p->cond_list[i]);
+}
+
int cond_policydb_init(struct policydb *p)
{
int rc;
diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
index 4677c6ff7450..b9eb888ffa76 100644
--- a/security/selinux/ss/conditional.h
+++ b/security/selinux/ss/conditional.h
@@ -78,6 +78,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);
-void evaluate_cond_node(struct policydb *p, struct cond_node *node);
+void evaluate_cond_nodes(struct policydb *p);
#endif /* _CONDITIONAL_H_ */
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 8fc8ec317bb6..7fb7f2efe566 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2958,8 +2958,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
policydb->bool_val_to_struct[i]->state = 0;
}
- for (i = 0; i < policydb->cond_list_len; i++)
- evaluate_cond_node(policydb, &policydb->cond_list[i]);
+ evaluate_cond_nodes(policydb);
seqno = ++state->ss->latest_granting;
rc = 0;
@@ -3012,8 +3011,7 @@ static int security_preserve_bools(struct selinux_state *state,
if (booldatum)
booldatum->state = bvalues[i];
}
- for (i = 0; i < policydb->cond_list_len; i++)
- evaluate_cond_node(policydb, &policydb->cond_list[i]);
+ evaluate_cond_nodes(policydb);
out:
if (bnames) {
--
2.24.1
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH v2 5/5] selinux: generalize evaluate_cond_node()
2020-01-17 8:58 ` [PATCH v2 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
@ 2020-01-17 19:39 ` Stephen Smalley
2020-01-31 4:24 ` Paul Moore
1 sibling, 0 replies; 16+ messages in thread
From: Stephen Smalley @ 2020-01-17 19:39 UTC (permalink / raw)
To: Ondrej Mosnacek, selinux, Paul Moore
On 1/17/20 3:58 AM, Ondrej Mosnacek wrote:
> Both callers iterate the cond_list and call it for each node - turn it
> into evaluate_cond_nodes(), which does the iteration for them.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
Reviewed-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/ss/conditional.c | 10 +++++++++-
> security/selinux/ss/conditional.h | 2 +-
> security/selinux/ss/services.c | 6 ++----
> 3 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
> index 8f9f2f3c86a0..ad709ccea036 100644
> --- a/security/selinux/ss/conditional.c
> +++ b/security/selinux/ss/conditional.c
> @@ -86,7 +86,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.
> */
> -void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> +static void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> {
> struct avtab_node *avnode;
> int new_state;
> @@ -117,6 +117,14 @@ void evaluate_cond_node(struct policydb *p, struct cond_node *node)
> }
> }
>
> +void evaluate_cond_nodes(struct policydb *p)
> +{
> + u32 i;
> +
> + for (i = 0; i < p->cond_list_len; i++)
> + evaluate_cond_node(p, &p->cond_list[i]);
> +}
> +
> int cond_policydb_init(struct policydb *p)
> {
> int rc;
> diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h
> index 4677c6ff7450..b9eb888ffa76 100644
> --- a/security/selinux/ss/conditional.h
> +++ b/security/selinux/ss/conditional.h
> @@ -78,6 +78,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);
> -void evaluate_cond_node(struct policydb *p, struct cond_node *node);
> +void evaluate_cond_nodes(struct policydb *p);
>
> #endif /* _CONDITIONAL_H_ */
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index 8fc8ec317bb6..7fb7f2efe566 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -2958,8 +2958,7 @@ int security_set_bools(struct selinux_state *state, u32 len, int *values)
> policydb->bool_val_to_struct[i]->state = 0;
> }
>
> - for (i = 0; i < policydb->cond_list_len; i++)
> - evaluate_cond_node(policydb, &policydb->cond_list[i]);
> + evaluate_cond_nodes(policydb);
>
> seqno = ++state->ss->latest_granting;
> rc = 0;
> @@ -3012,8 +3011,7 @@ static int security_preserve_bools(struct selinux_state *state,
> if (booldatum)
> booldatum->state = bvalues[i];
> }
> - for (i = 0; i < policydb->cond_list_len; i++)
> - evaluate_cond_node(policydb, &policydb->cond_list[i]);
> + evaluate_cond_nodes(policydb);
>
> out:
> if (bnames) {
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH v2 5/5] selinux: generalize evaluate_cond_node()
2020-01-17 8:58 ` [PATCH v2 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
2020-01-17 19:39 ` Stephen Smalley
@ 2020-01-31 4:24 ` Paul Moore
1 sibling, 0 replies; 16+ messages in thread
From: Paul Moore @ 2020-01-31 4:24 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Jan 17, 2020 at 3:58 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Both callers iterate the cond_list and call it for each node - turn it
> into evaluate_cond_nodes(), which does the iteration for them.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/ss/conditional.c | 10 +++++++++-
> security/selinux/ss/conditional.h | 2 +-
> security/selinux/ss/services.c | 6 ++----
> 3 files changed, 12 insertions(+), 6 deletions(-)
Also looks good, but I'm holding off pending a potential respin.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 16+ messages in thread