* [PATCH v3 0/5] selinux: Assorted simplifications and cleanups
@ 2020-02-03 11:27 Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
` (4 more replies)
0 siblings, 5 replies; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
This series contains some boolean code simplifications that I discovered
while working on another patch. I believe they also save some run time
(although not in any perf-critical paths) and some memory overhead.
Changes in v3:
- properly destroy all cond nodes in error path (bug found by Paul)
- additional style fixes suggested by Paul
Changes in v2:
- drop already merged 1st patch
- drop the rewrite of security_preserve_bools(), keep only the
evaluate_cond_node() return type change (requested by Paul)
Ondrej Mosnacek (5):
selinux: simplify evaluate_cond_node()
selinux: convert cond_list to array
selinux: convert cond_av_list to array
selinux: convert cond_expr to array
selinux: generalize evaluate_cond_node()
security/selinux/include/conditional.h | 8 +-
security/selinux/selinuxfs.c | 4 +-
security/selinux/ss/conditional.c | 251 ++++++++++---------------
security/selinux/ss/conditional.h | 27 +--
security/selinux/ss/policydb.c | 2 +-
security/selinux/ss/policydb.h | 3 +-
security/selinux/ss/services.c | 32 ++--
7 files changed, 137 insertions(+), 190 deletions(-)
--
2.24.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/5] selinux: simplify evaluate_cond_node()
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
@ 2020-02-03 11:27 ` Ondrej Mosnacek
2020-02-12 2:29 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 2/5] selinux: convert cond_list to array Ondrej Mosnacek
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 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] 11+ messages in thread
* [PATCH v3 2/5] selinux: convert cond_list to array
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
@ 2020-02-03 11:27 ` Ondrej Mosnacek
2020-02-12 2:41 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 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 | 8 ++--
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(+), 59 deletions(-)
diff --git a/security/selinux/include/conditional.h b/security/selinux/include/conditional.h
index 0ab316f61da0..539ab357707d 100644
--- a/security/selinux/include/conditional.h
+++ b/security/selinux/include/conditional.h
@@ -14,12 +14,10 @@
#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);
+int security_set_bools(struct selinux_state *state, u32 len, int *values);
-int security_get_bool_value(struct selinux_state *state,
- int index);
+int security_get_bool_value(struct selinux_state *state, 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..e6d203b76545 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;
+ p->cond_list_len = len;
- rc = cond_read_node(p, node, fp);
+ for (i = 0; i < len; i++) {
+ 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;
}
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] 11+ messages in thread
* [PATCH v3 3/5] selinux: convert cond_av_list to array
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 2/5] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-02-03 11:27 ` Ondrej Mosnacek
2020-02-12 2:47 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 4/5] selinux: convert cond_expr " Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
4 siblings, 1 reply; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 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 | 124 ++++++++++++------------------
security/selinux/ss/conditional.h | 8 +-
2 files changed, 53 insertions(+), 79 deletions(-)
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index e6d203b76545..82002b90809c 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,18 @@ 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 cond_av_list *other = data->other;
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 +270,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
@@ -292,24 +285,24 @@ static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum
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 < other->len; i++) {
+ if (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 +310,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 +334,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 +414,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 +493,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 +539,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] 11+ messages in thread
* [PATCH v3 4/5] selinux: convert cond_expr to array
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (2 preceding siblings ...)
2020-02-03 11:27 ` [PATCH v3 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-02-03 11:27 ` Ondrej Mosnacek
2020-02-12 2:49 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
4 siblings, 1 reply; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 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 82002b90809c..669b766c260b 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);
@@ -355,7 +351,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");
@@ -372,43 +368,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);
@@ -513,27 +503,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] 11+ messages in thread
* [PATCH v3 5/5] selinux: generalize evaluate_cond_node()
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
` (3 preceding siblings ...)
2020-02-03 11:27 ` [PATCH v3 4/5] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-02-03 11:27 ` Ondrej Mosnacek
2020-02-12 2:52 ` Paul Moore
4 siblings, 1 reply; 11+ messages in thread
From: Ondrej Mosnacek @ 2020-02-03 11:27 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 669b766c260b..cce4a75fb3e7 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] 11+ messages in thread
* Re: [PATCH v3 1/5] selinux: simplify evaluate_cond_node()
2020-02-03 11:27 ` [PATCH v3 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
@ 2020-02-12 2:29 ` Paul Moore
0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2020-02-12 2:29 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Mon, Feb 3, 2020 at 6:27 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(-)
This patch was merged from the v2 patchset and is currently in selinux/next.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/5] selinux: convert cond_list to array
2020-02-03 11:27 ` [PATCH v3 2/5] selinux: convert cond_list to array Ondrej Mosnacek
@ 2020-02-12 2:41 ` Paul Moore
0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2020-02-12 2:41 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Mon, Feb 3, 2020 at 6:27 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 | 8 ++--
> 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(+), 59 deletions(-)
Merged into selinux/next, thanks Ondrej.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 3/5] selinux: convert cond_av_list to array
2020-02-03 11:27 ` [PATCH v3 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
@ 2020-02-12 2:47 ` Paul Moore
0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2020-02-12 2:47 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Mon, Feb 3, 2020 at 6:27 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 | 124 ++++++++++++------------------
> security/selinux/ss/conditional.h | 8 +-
> 2 files changed, 53 insertions(+), 79 deletions(-)
Also merged into selinux/next, thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/5] selinux: convert cond_expr to array
2020-02-03 11:27 ` [PATCH v3 4/5] selinux: convert cond_expr " Ondrej Mosnacek
@ 2020-02-12 2:49 ` Paul Moore
0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2020-02-12 2:49 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Mon, Feb 3, 2020 at 6:27 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 one too ... merged.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] selinux: generalize evaluate_cond_node()
2020-02-03 11:27 ` [PATCH v3 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
@ 2020-02-12 2:52 ` Paul Moore
0 siblings, 0 replies; 11+ messages in thread
From: Paul Moore @ 2020-02-12 2:52 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Mon, Feb 3, 2020 at 6:27 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(-)
Last one, merged. Thanks!
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-02-12 2:52 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-03 11:27 [PATCH v3 0/5] selinux: Assorted simplifications and cleanups Ondrej Mosnacek
2020-02-03 11:27 ` [PATCH v3 1/5] selinux: simplify evaluate_cond_node() Ondrej Mosnacek
2020-02-12 2:29 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 2/5] selinux: convert cond_list to array Ondrej Mosnacek
2020-02-12 2:41 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 3/5] selinux: convert cond_av_list " Ondrej Mosnacek
2020-02-12 2:47 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 4/5] selinux: convert cond_expr " Ondrej Mosnacek
2020-02-12 2:49 ` Paul Moore
2020-02-03 11:27 ` [PATCH v3 5/5] selinux: generalize evaluate_cond_node() Ondrej Mosnacek
2020-02-12 2:52 ` Paul Moore
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.