All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] selinux: fix changing booleans
@ 2021-03-30 13:16 Ondrej Mosnacek
  2021-03-30 13:16 ` [PATCH 1/3] selinux: fix cond_list corruption when " Ondrej Mosnacek
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-30 13:16 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Stephen Smalley

This series contains a patch that fixes broken conditional AV list
duplication introduced by c7c556f1e81b ("selinux: refactor changing
booleans") and a couple "and while I'm here..." cleanup patches on top.

Ondrej Mosnacek (3):
  selinux: fix cond_list corruption when changing booleans
  selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
  selinux: constify some avtab function arguments

 security/selinux/ss/avtab.c       | 118 +++++++++++-------------------
 security/selinux/ss/avtab.h       |  18 +++--
 security/selinux/ss/conditional.c |  26 ++++---
 3 files changed, 65 insertions(+), 97 deletions(-)

-- 
2.30.2


^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-03-30 13:16 [PATCH 0/3] selinux: fix changing booleans Ondrej Mosnacek
@ 2021-03-30 13:16 ` Ondrej Mosnacek
  2021-03-31  2:04   ` Paul Moore
  2021-03-30 13:16 ` [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup() Ondrej Mosnacek
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-30 13:16 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Stephen Smalley

Currently, duplicate_policydb_cond_list() first copies the whole
conditional avtab and then tries to link to the correct entries in
cond_dup_av_list() using avtab_search(). However, since the conditional
avtab may contain multiple entries with the same key, this approach
often fails to find the right entry, potentially leading to wrong rules
being activated/deactivated when booleans are changed.

To fix this, instead start with an empty conditional avtab and add the
individual entries one-by-one while building the new av_lists. This
approach leads to the correct result, since each entry is present in the
av_lists exactly once.

The issue can be reproduced with Fedora policy as follows:

    # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
    allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
    allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
    # setsebool ftpd_anon_write=off ftpd_connect_all_unreserved=off ftpd_connect_db=off ftpd_full_access=off

On fixed kernels, the sesearch output is the same after the setsebool
command:

    # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
    allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
    allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True

While on the broken kernels, it will be different:

    # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
    allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
    allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
    allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True

Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/avtab.c       | 90 +++++++++----------------------
 security/selinux/ss/avtab.h       |  2 +-
 security/selinux/ss/conditional.c | 12 ++---
 3 files changed, 33 insertions(+), 71 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 6dcb6aa4db7f..11f8f524de98 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -305,86 +305,48 @@ void avtab_init(struct avtab *h)
 	h->nel = 0;
 }
 
-int avtab_alloc(struct avtab *h, u32 nrules)
+static int avtab_alloc_common(struct avtab *h, u32 nslot)
 {
-	u32 mask = 0;
-	u32 shift = 0;
-	u32 work = nrules;
-	u32 nslot = 0;
-
-	if (nrules == 0)
-		goto avtab_alloc_out;
-
-	while (work) {
-		work  = work >> 1;
-		shift++;
-	}
-	if (shift > 2)
-		shift = shift - 2;
-	nslot = 1 << shift;
-	if (nslot > MAX_AVTAB_HASH_BUCKETS)
-		nslot = MAX_AVTAB_HASH_BUCKETS;
-	mask = nslot - 1;
-
 	h->htable = kvcalloc(nslot, sizeof(void *), GFP_KERNEL);
 	if (!h->htable)
 		return -ENOMEM;
 
- avtab_alloc_out:
 	h->nel = 0;
 	h->nslot = nslot;
-	h->mask = mask;
-	pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
-	       h->nslot, nrules);
+	h->mask = nslot - 1;
 	return 0;
 }
 
-int avtab_duplicate(struct avtab *new, struct avtab *orig)
+int avtab_alloc(struct avtab *h, u32 nrules)
 {
-	int i;
-	struct avtab_node *node, *tmp, *tail;
-
-	memset(new, 0, sizeof(*new));
+	int rc;
+	u32 shift = 0;
+	u32 work = nrules;
+	u32 nslot = 0;
 
-	new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
-	if (!new->htable)
-		return -ENOMEM;
-	new->nslot = orig->nslot;
-	new->mask = orig->mask;
-
-	for (i = 0; i < orig->nslot; i++) {
-		tail = NULL;
-		for (node = orig->htable[i]; node; node = node->next) {
-			tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
-			if (!tmp)
-				goto error;
-			tmp->key = node->key;
-			if (tmp->key.specified & AVTAB_XPERMS) {
-				tmp->datum.u.xperms =
-					kmem_cache_zalloc(avtab_xperms_cachep,
-							GFP_KERNEL);
-				if (!tmp->datum.u.xperms) {
-					kmem_cache_free(avtab_node_cachep, tmp);
-					goto error;
-				}
-				tmp->datum.u.xperms = node->datum.u.xperms;
-			} else
-				tmp->datum.u.data = node->datum.u.data;
-
-			if (tail)
-				tail->next = tmp;
-			else
-				new->htable[i] = tmp;
-
-			tail = tmp;
-			new->nel++;
+	if (nrules != 0) {
+		while (work) {
+			work  = work >> 1;
+			shift++;
 		}
+		if (shift > 2)
+			shift = shift - 2;
+		nslot = 1 << shift;
+		if (nslot > MAX_AVTAB_HASH_BUCKETS)
+			nslot = MAX_AVTAB_HASH_BUCKETS;
 	}
 
+	rc = avtab_alloc_common(h, nslot);
+	if (rc)
+		return rc;
+
+	pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules);
 	return 0;
-error:
-	avtab_destroy(new);
-	return -ENOMEM;
+}
+
+int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
+{
+	return avtab_alloc_common(new, orig->nslot);
 }
 
 void avtab_hash_eval(struct avtab *h, char *tag)
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index 4c4445ca9118..f2eeb36265d1 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -89,7 +89,7 @@ struct avtab {
 
 void avtab_init(struct avtab *h);
 int avtab_alloc(struct avtab *, u32);
-int avtab_duplicate(struct avtab *new, struct avtab *orig);
+int avtab_alloc_dup(struct avtab *new, const struct avtab *orig);
 struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *k);
 void avtab_destroy(struct avtab *h);
 void avtab_hash_eval(struct avtab *h, char *tag);
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 0b32f3ab025e..1ef74c085f2b 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -605,7 +605,6 @@ static int cond_dup_av_list(struct cond_av_list *new,
 			struct cond_av_list *orig,
 			struct avtab *avtab)
 {
-	struct avtab_node *avnode;
 	u32 i;
 
 	memset(new, 0, sizeof(*new));
@@ -615,10 +614,11 @@ static int cond_dup_av_list(struct cond_av_list *new,
 		return -ENOMEM;
 
 	for (i = 0; i < orig->len; i++) {
-		avnode = avtab_search_node(avtab, &orig->nodes[i]->key);
-		if (WARN_ON(!avnode))
-			return -EINVAL;
-		new->nodes[i] = avnode;
+		new->nodes[i] = avtab_insert_nonunique(avtab,
+						       &orig->nodes[i]->key,
+						       &orig->nodes[i]->datum);
+		if (!new->nodes[i])
+			return -ENOMEM;
 		new->len++;
 	}
 
@@ -630,7 +630,7 @@ static int duplicate_policydb_cond_list(struct policydb *newp,
 {
 	int rc, i, j;
 
-	rc = avtab_duplicate(&newp->te_cond_avtab, &origp->te_cond_avtab);
+	rc = avtab_alloc_dup(&newp->te_cond_avtab, &origp->te_cond_avtab);
 	if (rc)
 		return rc;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
  2021-03-30 13:16 [PATCH 0/3] selinux: fix changing booleans Ondrej Mosnacek
  2021-03-30 13:16 ` [PATCH 1/3] selinux: fix cond_list corruption when " Ondrej Mosnacek
@ 2021-03-30 13:16 ` Ondrej Mosnacek
  2021-05-11  1:34   ` Paul Moore
  2021-03-30 13:16 ` [PATCH 3/3] selinux: constify some avtab function arguments Ondrej Mosnacek
  2021-03-31  1:13 ` [PATCH 0/3] selinux: fix changing booleans Paul Moore
  3 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-30 13:16 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Stephen Smalley

We can do the allocation + copying of expr.nodes in one go using
kmemdup().

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/conditional.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 1ef74c085f2b..f6dfa9c821d6 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -628,7 +628,8 @@ static int cond_dup_av_list(struct cond_av_list *new,
 static int duplicate_policydb_cond_list(struct policydb *newp,
 					struct policydb *origp)
 {
-	int rc, i, j;
+	int rc;
+	u32 i;
 
 	rc = avtab_alloc_dup(&newp->te_cond_avtab, &origp->te_cond_avtab);
 	if (rc)
@@ -648,12 +649,12 @@ static int duplicate_policydb_cond_list(struct policydb *newp,
 		newp->cond_list_len++;
 
 		newn->cur_state = orign->cur_state;
-		newn->expr.nodes = kcalloc(orign->expr.len,
-					sizeof(*newn->expr.nodes), GFP_KERNEL);
+		newn->expr.nodes = kmemdup(orign->expr.nodes,
+				orign->expr.len * sizeof(*orign->expr.nodes),
+				GFP_KERNEL);
 		if (!newn->expr.nodes)
 			goto error;
-		for (j = 0; j < orign->expr.len; j++)
-			newn->expr.nodes[j] = orign->expr.nodes[j];
+
 		newn->expr.len = orign->expr.len;
 
 		rc = cond_dup_av_list(&newn->true_list, &orign->true_list,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 3/3] selinux: constify some avtab function arguments
  2021-03-30 13:16 [PATCH 0/3] selinux: fix changing booleans Ondrej Mosnacek
  2021-03-30 13:16 ` [PATCH 1/3] selinux: fix cond_list corruption when " Ondrej Mosnacek
  2021-03-30 13:16 ` [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup() Ondrej Mosnacek
@ 2021-03-30 13:16 ` Ondrej Mosnacek
  2021-05-11  1:36   ` Paul Moore
  2021-03-31  1:13 ` [PATCH 0/3] selinux: fix changing booleans Paul Moore
  3 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-30 13:16 UTC (permalink / raw)
  To: selinux, Paul Moore; +Cc: Stephen Smalley

This makes the code a bit easier to reason about.

Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/ss/avtab.c       | 28 +++++++++++++++-------------
 security/selinux/ss/avtab.h       | 16 +++++++++-------
 security/selinux/ss/conditional.c |  3 ++-
 3 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 11f8f524de98..f6f09bb8116a 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -29,7 +29,7 @@ static struct kmem_cache *avtab_xperms_cachep __ro_after_init;
 /* Based on MurmurHash3, written by Austin Appleby and placed in the
  * public domain.
  */
-static inline int avtab_hash(struct avtab_key *keyp, u32 mask)
+static inline int avtab_hash(const struct avtab_key *keyp, u32 mask)
 {
 	static const u32 c1 = 0xcc9e2d51;
 	static const u32 c2 = 0x1b873593;
@@ -68,7 +68,7 @@ static inline int avtab_hash(struct avtab_key *keyp, u32 mask)
 static struct avtab_node*
 avtab_insert_node(struct avtab *h, int hvalue,
 		  struct avtab_node *prev, struct avtab_node *cur,
-		  struct avtab_key *key, struct avtab_datum *datum)
+		  const struct avtab_key *key, const struct avtab_datum *datum)
 {
 	struct avtab_node *newnode;
 	struct avtab_extended_perms *xperms;
@@ -103,7 +103,8 @@ avtab_insert_node(struct avtab *h, int hvalue,
 	return newnode;
 }
 
-static int avtab_insert(struct avtab *h, struct avtab_key *key, struct avtab_datum *datum)
+static int avtab_insert(struct avtab *h, const struct avtab_key *key,
+			const struct avtab_datum *datum)
 {
 	int hvalue;
 	struct avtab_node *prev, *cur, *newnode;
@@ -147,8 +148,9 @@ static int avtab_insert(struct avtab *h, struct avtab_key *key, struct avtab_dat
  * key/specified mask into the table, as needed by the conditional avtab.
  * It also returns a pointer to the node inserted.
  */
-struct avtab_node *
-avtab_insert_nonunique(struct avtab *h, struct avtab_key *key, struct avtab_datum *datum)
+struct avtab_node *avtab_insert_nonunique(struct avtab *h,
+					  const struct avtab_key *key,
+					  const struct avtab_datum *datum)
 {
 	int hvalue;
 	struct avtab_node *prev, *cur;
@@ -178,7 +180,7 @@ avtab_insert_nonunique(struct avtab *h, struct avtab_key *key, struct avtab_datu
 	return avtab_insert_node(h, hvalue, prev, cur, key, datum);
 }
 
-struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *key)
+struct avtab_datum *avtab_search(struct avtab *h, const struct avtab_key *key)
 {
 	int hvalue;
 	struct avtab_node *cur;
@@ -213,8 +215,8 @@ struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *key)
 /* This search function returns a node pointer, and can be used in
  * conjunction with avtab_search_next_node()
  */
-struct avtab_node*
-avtab_search_node(struct avtab *h, struct avtab_key *key)
+struct avtab_node *avtab_search_node(struct avtab *h,
+				     const struct avtab_key *key)
 {
 	int hvalue;
 	struct avtab_node *cur;
@@ -393,8 +395,8 @@ static uint16_t spec_order[] = {
 };
 
 int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
-		    int (*insertf)(struct avtab *a, struct avtab_key *k,
-				   struct avtab_datum *d, void *p),
+		    int (*insertf)(struct avtab *a, const struct avtab_key *k,
+				   const struct avtab_datum *d, void *p),
 		    void *p)
 {
 	__le16 buf16[4];
@@ -554,8 +556,8 @@ int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
 	return insertf(a, &key, &datum, p);
 }
 
-static int avtab_insertf(struct avtab *a, struct avtab_key *k,
-			 struct avtab_datum *d, void *p)
+static int avtab_insertf(struct avtab *a, const struct avtab_key *k,
+			 const struct avtab_datum *d, void *p)
 {
 	return avtab_insert(a, k, d);
 }
@@ -604,7 +606,7 @@ bad:
 	goto out;
 }
 
-int avtab_write_item(struct policydb *p, struct avtab_node *cur, void *fp)
+int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp)
 {
 	__le16 buf16[4];
 	__le32 buf32[ARRAY_SIZE(cur->datum.u.xperms->perms.p)];
diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h
index f2eeb36265d1..d3ebea8d146f 100644
--- a/security/selinux/ss/avtab.h
+++ b/security/selinux/ss/avtab.h
@@ -90,24 +90,26 @@ struct avtab {
 void avtab_init(struct avtab *h);
 int avtab_alloc(struct avtab *, u32);
 int avtab_alloc_dup(struct avtab *new, const struct avtab *orig);
-struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *k);
+struct avtab_datum *avtab_search(struct avtab *h, const struct avtab_key *k);
 void avtab_destroy(struct avtab *h);
 void avtab_hash_eval(struct avtab *h, char *tag);
 
 struct policydb;
 int avtab_read_item(struct avtab *a, void *fp, struct policydb *pol,
-		    int (*insert)(struct avtab *a, struct avtab_key *k,
-				  struct avtab_datum *d, void *p),
+		    int (*insert)(struct avtab *a, const struct avtab_key *k,
+				  const struct avtab_datum *d, void *p),
 		    void *p);
 
 int avtab_read(struct avtab *a, void *fp, struct policydb *pol);
-int avtab_write_item(struct policydb *p, struct avtab_node *cur, void *fp);
+int avtab_write_item(struct policydb *p, const struct avtab_node *cur, void *fp);
 int avtab_write(struct policydb *p, struct avtab *a, void *fp);
 
-struct avtab_node *avtab_insert_nonunique(struct avtab *h, struct avtab_key *key,
-					  struct avtab_datum *datum);
+struct avtab_node *avtab_insert_nonunique(struct avtab *h,
+					  const struct avtab_key *key,
+					  const struct avtab_datum *datum);
 
-struct avtab_node *avtab_search_node(struct avtab *h, struct avtab_key *key);
+struct avtab_node *avtab_search_node(struct avtab *h,
+				     const struct avtab_key *key);
 
 struct avtab_node *avtab_search_node_next(struct avtab_node *node, int specified);
 
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index f6dfa9c821d6..2ec6e5cd25d9 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -254,7 +254,8 @@ struct cond_insertf_data {
 	struct cond_av_list *other;
 };
 
-static int cond_insertf(struct avtab *a, struct avtab_key *k, struct avtab_datum *d, void *ptr)
+static int cond_insertf(struct avtab *a, const struct avtab_key *k,
+			const struct avtab_datum *d, void *ptr)
 {
 	struct cond_insertf_data *data = ptr;
 	struct policydb *p = data->p;
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] selinux: fix changing booleans
  2021-03-30 13:16 [PATCH 0/3] selinux: fix changing booleans Ondrej Mosnacek
                   ` (2 preceding siblings ...)
  2021-03-30 13:16 ` [PATCH 3/3] selinux: constify some avtab function arguments Ondrej Mosnacek
@ 2021-03-31  1:13 ` Paul Moore
  2021-03-31  8:23   ` Ondrej Mosnacek
  3 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2021-03-31  1:13 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley

On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This series contains a patch that fixes broken conditional AV list
> duplication introduced by c7c556f1e81b ("selinux: refactor changing
> booleans") and a couple "and while I'm here..." cleanup patches on top.
>
> Ondrej Mosnacek (3):
>   selinux: fix cond_list corruption when changing booleans
>   selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
>   selinux: constify some avtab function arguments

Please don't resubmit, but in the future if you are submitting a patch
(or two (or three ...)) which is potential -stable material (and so
far I think 1/3 qualifies) don't submit it in a patchset with trivial
cleanup patches.  Adding cleanup patches to a patchset that adds a
feature is okay, but fixes should generally stand by themselves.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-03-30 13:16 ` [PATCH 1/3] selinux: fix cond_list corruption when " Ondrej Mosnacek
@ 2021-03-31  2:04   ` Paul Moore
  2021-03-31  9:12     ` Ondrej Mosnacek
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2021-03-31  2:04 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley

On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> Currently, duplicate_policydb_cond_list() first copies the whole
> conditional avtab and then tries to link to the correct entries in
> cond_dup_av_list() using avtab_search(). However, since the conditional
> avtab may contain multiple entries with the same key, this approach
> often fails to find the right entry, potentially leading to wrong rules
> being activated/deactivated when booleans are changed.
>
> To fix this, instead start with an empty conditional avtab and add the
> individual entries one-by-one while building the new av_lists. This
> approach leads to the correct result, since each entry is present in the
> av_lists exactly once.
>
> The issue can be reproduced with Fedora policy as follows:
>
>     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
>     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
>     allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
>     # setsebool ftpd_anon_write=off ftpd_connect_all_unreserved=off ftpd_connect_db=off ftpd_full_access=off
>
> On fixed kernels, the sesearch output is the same after the setsebool
> command:
>
>     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
>     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
>     allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
>
> While on the broken kernels, it will be different:
>
>     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
>     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
>     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
>     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
>
> Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/avtab.c       | 90 +++++++++----------------------
>  security/selinux/ss/avtab.h       |  2 +-
>  security/selinux/ss/conditional.c | 12 ++---
>  3 files changed, 33 insertions(+), 71 deletions(-)
>
> diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> index 6dcb6aa4db7f..11f8f524de98 100644
> --- a/security/selinux/ss/avtab.c
> +++ b/security/selinux/ss/avtab.c
> @@ -305,86 +305,48 @@ void avtab_init(struct avtab *h)
>         h->nel = 0;
>  }
>
> -int avtab_alloc(struct avtab *h, u32 nrules)
> +static int avtab_alloc_common(struct avtab *h, u32 nslot)
>  {
> -       u32 mask = 0;
> -       u32 shift = 0;
> -       u32 work = nrules;
> -       u32 nslot = 0;
> -
> -       if (nrules == 0)
> -               goto avtab_alloc_out;
> -
> -       while (work) {
> -               work  = work >> 1;
> -               shift++;
> -       }
> -       if (shift > 2)
> -               shift = shift - 2;
> -       nslot = 1 << shift;
> -       if (nslot > MAX_AVTAB_HASH_BUCKETS)
> -               nslot = MAX_AVTAB_HASH_BUCKETS;
> -       mask = nslot - 1;
> -
>         h->htable = kvcalloc(nslot, sizeof(void *), GFP_KERNEL);
>         if (!h->htable)
>                 return -ENOMEM;

Hmmm, do we need to protect against 'nslot == 0'?  Unless I missed
something, a quick dive into kvcalloc() makes it look like it can
return non-NULL for zero length allocations, at least in the slab
case.

> - avtab_alloc_out:
>         h->nel = 0;
>         h->nslot = nslot;
> -       h->mask = mask;
> -       pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
> -              h->nslot, nrules);
> +       h->mask = nslot - 1;

This is definitely not good if 'nslot <= 1';

>         return 0;
>  }
>
> -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> +int avtab_alloc(struct avtab *h, u32 nrules)
>  {
> -       int i;
> -       struct avtab_node *node, *tmp, *tail;
> -
> -       memset(new, 0, sizeof(*new));
> +       int rc;
> +       u32 shift = 0;
> +       u32 work = nrules;
> +       u32 nslot = 0;
>
> -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> -       if (!new->htable)
> -               return -ENOMEM;
> -       new->nslot = orig->nslot;
> -       new->mask = orig->mask;
> -
> -       for (i = 0; i < orig->nslot; i++) {
> -               tail = NULL;
> -               for (node = orig->htable[i]; node; node = node->next) {
> -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> -                       if (!tmp)
> -                               goto error;
> -                       tmp->key = node->key;
> -                       if (tmp->key.specified & AVTAB_XPERMS) {
> -                               tmp->datum.u.xperms =
> -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> -                                                       GFP_KERNEL);
> -                               if (!tmp->datum.u.xperms) {
> -                                       kmem_cache_free(avtab_node_cachep, tmp);
> -                                       goto error;
> -                               }
> -                               tmp->datum.u.xperms = node->datum.u.xperms;
> -                       } else
> -                               tmp->datum.u.data = node->datum.u.data;
> -
> -                       if (tail)
> -                               tail->next = tmp;
> -                       else
> -                               new->htable[i] = tmp;
> -
> -                       tail = tmp;
> -                       new->nel++;
> +       if (nrules != 0) {
> +               while (work) {
> +                       work  = work >> 1;

Extra  horizontal  spaces  are  awkward  and  bad.

> +                       shift++;
>                 }
> +               if (shift > 2)
> +                       shift = shift - 2;

Since we are getting nit-picky with this code, why not make the
loop/if a bit more elegant?  How about something like below?

  u32 shift = 2;
  u32 work = nrules >> 4;
  while (work) {
    work >>= 1;
    shift++;
  }

> +               nslot = 1 << shift;
> +               if (nslot > MAX_AVTAB_HASH_BUCKETS)
> +                       nslot = MAX_AVTAB_HASH_BUCKETS;
>         }
>
> +       rc = avtab_alloc_common(h, nslot);
> +       if (rc)
> +               return rc;
> +
> +       pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules);
>         return 0;
> -error:
> -       avtab_destroy(new);
> -       return -ENOMEM;
> +}
> +
> +int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
> +{
> +       return avtab_alloc_common(new, orig->nslot);
>  }

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] selinux: fix changing booleans
  2021-03-31  1:13 ` [PATCH 0/3] selinux: fix changing booleans Paul Moore
@ 2021-03-31  8:23   ` Ondrej Mosnacek
  2021-03-31 23:26     ` Paul Moore
  0 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-31  8:23 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, Stephen Smalley

On Wed, Mar 31, 2021 at 3:13 AM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > This series contains a patch that fixes broken conditional AV list
> > duplication introduced by c7c556f1e81b ("selinux: refactor changing
> > booleans") and a couple "and while I'm here..." cleanup patches on top.
> >
> > Ondrej Mosnacek (3):
> >   selinux: fix cond_list corruption when changing booleans
> >   selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
> >   selinux: constify some avtab function arguments
>
> Please don't resubmit, but in the future if you are submitting a patch
> (or two (or three ...)) which is potential -stable material (and so
> far I think 1/3 qualifies) don't submit it in a patchset with trivial
> cleanup patches.  Adding cleanup patches to a patchset that adds a
> feature is okay, but fixes should generally stand by themselves.

Okay, but in this case the patches are sort of interdependent, so I
didn't want to mess with a rebase and sending conflicting patches... I
did move the bugfix patch to the bottom so that at worst you can pick
it out and ask me to resubmit the rest some other way.

Since I'll need to respin the bugfix patch, will it be better if I
repost just the one patch?

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-03-31  2:04   ` Paul Moore
@ 2021-03-31  9:12     ` Ondrej Mosnacek
  2021-03-31 23:19       ` Paul Moore
  0 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-03-31  9:12 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, Stephen Smalley

On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > Currently, duplicate_policydb_cond_list() first copies the whole
> > conditional avtab and then tries to link to the correct entries in
> > cond_dup_av_list() using avtab_search(). However, since the conditional
> > avtab may contain multiple entries with the same key, this approach
> > often fails to find the right entry, potentially leading to wrong rules
> > being activated/deactivated when booleans are changed.
> >
> > To fix this, instead start with an empty conditional avtab and add the
> > individual entries one-by-one while building the new av_lists. This
> > approach leads to the correct result, since each entry is present in the
> > av_lists exactly once.
> >
> > The issue can be reproduced with Fedora policy as follows:
> >
> >     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> >     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >     allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
> >     # setsebool ftpd_anon_write=off ftpd_connect_all_unreserved=off ftpd_connect_db=off ftpd_full_access=off
> >
> > On fixed kernels, the sesearch output is the same after the setsebool
> > command:
> >
> >     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> >     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >     allow ftpd_t public_content_rw_t:dir { add_name create link remove_name rename reparent rmdir setattr unlink watch watch_reads write }; [ ftpd_anon_write ]:True
> >
> > While on the broken kernels, it will be different:
> >
> >     # sesearch -s ftpd_t -t public_content_rw_t -c dir -p create -A
> >     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >     allow ftpd_t non_security_file_type:dir { add_name create getattr ioctl link lock open read remove_name rename reparent rmdir search setattr unlink watch watch_reads write }; [ ftpd_full_access ]:True
> >
> > Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/ss/avtab.c       | 90 +++++++++----------------------
> >  security/selinux/ss/avtab.h       |  2 +-
> >  security/selinux/ss/conditional.c | 12 ++---
> >  3 files changed, 33 insertions(+), 71 deletions(-)
> >
> > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
> > index 6dcb6aa4db7f..11f8f524de98 100644
> > --- a/security/selinux/ss/avtab.c
> > +++ b/security/selinux/ss/avtab.c
> > @@ -305,86 +305,48 @@ void avtab_init(struct avtab *h)
> >         h->nel = 0;
> >  }
> >
> > -int avtab_alloc(struct avtab *h, u32 nrules)
> > +static int avtab_alloc_common(struct avtab *h, u32 nslot)
> >  {
> > -       u32 mask = 0;
> > -       u32 shift = 0;
> > -       u32 work = nrules;
> > -       u32 nslot = 0;
> > -
> > -       if (nrules == 0)
> > -               goto avtab_alloc_out;
> > -
> > -       while (work) {
> > -               work  = work >> 1;
> > -               shift++;
> > -       }
> > -       if (shift > 2)
> > -               shift = shift - 2;
> > -       nslot = 1 << shift;
> > -       if (nslot > MAX_AVTAB_HASH_BUCKETS)
> > -               nslot = MAX_AVTAB_HASH_BUCKETS;
> > -       mask = nslot - 1;
> > -
> >         h->htable = kvcalloc(nslot, sizeof(void *), GFP_KERNEL);
> >         if (!h->htable)
> >                 return -ENOMEM;
>
> Hmmm, do we need to protect against 'nslot == 0'?  Unless I missed
> something, a quick dive into kvcalloc() makes it look like it can
> return non-NULL for zero length allocations, at least in the slab
> case.
>
> > - avtab_alloc_out:
> >         h->nel = 0;
> >         h->nslot = nslot;
> > -       h->mask = mask;
> > -       pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
> > -              h->nslot, nrules);
> > +       h->mask = nslot - 1;
>
> This is definitely not good if 'nslot <= 1';

Yeah, in fact the current code doesn't seem to be safe against nslot
== 0 either... I'll insert another patch that fixes this.

>
> >         return 0;
> >  }
> >
> > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > +int avtab_alloc(struct avtab *h, u32 nrules)
> >  {
> > -       int i;
> > -       struct avtab_node *node, *tmp, *tail;
> > -
> > -       memset(new, 0, sizeof(*new));
> > +       int rc;
> > +       u32 shift = 0;
> > +       u32 work = nrules;
> > +       u32 nslot = 0;
> >
> > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > -       if (!new->htable)
> > -               return -ENOMEM;
> > -       new->nslot = orig->nslot;
> > -       new->mask = orig->mask;
> > -
> > -       for (i = 0; i < orig->nslot; i++) {
> > -               tail = NULL;
> > -               for (node = orig->htable[i]; node; node = node->next) {
> > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > -                       if (!tmp)
> > -                               goto error;
> > -                       tmp->key = node->key;
> > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > -                               tmp->datum.u.xperms =
> > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > -                                                       GFP_KERNEL);
> > -                               if (!tmp->datum.u.xperms) {
> > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > -                                       goto error;
> > -                               }
> > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > -                       } else
> > -                               tmp->datum.u.data = node->datum.u.data;
> > -
> > -                       if (tail)
> > -                               tail->next = tmp;
> > -                       else
> > -                               new->htable[i] = tmp;
> > -
> > -                       tail = tmp;
> > -                       new->nel++;
> > +       if (nrules != 0) {
> > +               while (work) {
> > +                       work  = work >> 1;
>
> Extra  horizontal  spaces  are  awkward  and  bad.
>
> > +                       shift++;
> >                 }
> > +               if (shift > 2)
> > +                       shift = shift - 2;
>
> Since we are getting nit-picky with this code, why not make the
> loop/if a bit more elegant?  How about something like below?
>
>   u32 shift = 2;
>   u32 work = nrules >> 4;
>   while (work) {
>     work >>= 1;
>     shift++;
>   }

I think you meant:
u32 shift = 0;
u32 work = nrules >> 2;
while (work) {
    work >>= 1;
    shift++;
}

...which is equivalent to the current code and yes, I'll use that :)

>
> > +               nslot = 1 << shift;
> > +               if (nslot > MAX_AVTAB_HASH_BUCKETS)
> > +                       nslot = MAX_AVTAB_HASH_BUCKETS;
> >         }
> >
> > +       rc = avtab_alloc_common(h, nslot);
> > +       if (rc)
> > +               return rc;
> > +
> > +       pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules);
> >         return 0;
> > -error:
> > -       avtab_destroy(new);
> > -       return -ENOMEM;
> > +}
> > +
> > +int avtab_alloc_dup(struct avtab *new, const struct avtab *orig)
> > +{
> > +       return avtab_alloc_common(new, orig->nslot);
> >  }
>
> --
> paul moore
> www.paul-moore.com
>


-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-03-31  9:12     ` Ondrej Mosnacek
@ 2021-03-31 23:19       ` Paul Moore
  2021-04-01  7:59         ` Ondrej Mosnacek
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2021-03-31 23:19 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley

On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:

...

> > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > >  {
> > > -       int i;
> > > -       struct avtab_node *node, *tmp, *tail;
> > > -
> > > -       memset(new, 0, sizeof(*new));
> > > +       int rc;
> > > +       u32 shift = 0;
> > > +       u32 work = nrules;
> > > +       u32 nslot = 0;
> > >
> > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > -       if (!new->htable)
> > > -               return -ENOMEM;
> > > -       new->nslot = orig->nslot;
> > > -       new->mask = orig->mask;
> > > -
> > > -       for (i = 0; i < orig->nslot; i++) {
> > > -               tail = NULL;
> > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > -                       if (!tmp)
> > > -                               goto error;
> > > -                       tmp->key = node->key;
> > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > -                               tmp->datum.u.xperms =
> > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > -                                                       GFP_KERNEL);
> > > -                               if (!tmp->datum.u.xperms) {
> > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > -                                       goto error;
> > > -                               }
> > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > -                       } else
> > > -                               tmp->datum.u.data = node->datum.u.data;
> > > -
> > > -                       if (tail)
> > > -                               tail->next = tmp;
> > > -                       else
> > > -                               new->htable[i] = tmp;
> > > -
> > > -                       tail = tmp;
> > > -                       new->nel++;
> > > +       if (nrules != 0) {
> > > +               while (work) {
> > > +                       work  = work >> 1;
> >
> > Extra  horizontal  spaces  are  awkward  and  bad.
> >
> > > +                       shift++;
> > >                 }
> > > +               if (shift > 2)
> > > +                       shift = shift - 2;
> >
> > Since we are getting nit-picky with this code, why not make the
> > loop/if a bit more elegant?  How about something like below?
> >
> >   u32 shift = 2;
> >   u32 work = nrules >> 4;
> >   while (work) {
> >     work >>= 1;
> >     shift++;
> >   }
>
> I think you meant:
> u32 shift = 0;
> u32 work = nrules >> 2;
> while (work) {
>     work >>= 1;
>     shift++;
> }
>
> ...which is equivalent to the current code and yes, I'll use that :)

Well, no, not really, but that's okay as looking at it now we both got
it wrong :)

Basically I wanted to avoid the odd problem where the current code has
a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
or 7.  While the code I proposed yesterday did that, it inflated the
number of buckets beyond the current code; your suggestion had
problems with low numbers resulting in zero buckets.

I think what we really want is this:

  shift = 2;
  work = nrules >> 4;
  while (work) {
    work >>= 1;
    shift++;
  }

... it avoids any dips in the bucket count and it results in similar
bucket counts as the existing code for larger numbers of rules.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] selinux: fix changing booleans
  2021-03-31  8:23   ` Ondrej Mosnacek
@ 2021-03-31 23:26     ` Paul Moore
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2021-03-31 23:26 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley

On Wed, Mar 31, 2021 at 4:23 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Wed, Mar 31, 2021 at 3:13 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > This series contains a patch that fixes broken conditional AV list
> > > duplication introduced by c7c556f1e81b ("selinux: refactor changing
> > > booleans") and a couple "and while I'm here..." cleanup patches on top.
> > >
> > > Ondrej Mosnacek (3):
> > >   selinux: fix cond_list corruption when changing booleans
> > >   selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
> > >   selinux: constify some avtab function arguments
> >
> > Please don't resubmit, but in the future if you are submitting a patch
> > (or two (or three ...)) which is potential -stable material (and so
> > far I think 1/3 qualifies) don't submit it in a patchset with trivial
> > cleanup patches.  Adding cleanup patches to a patchset that adds a
> > feature is okay, but fixes should generally stand by themselves.
>
> Okay, but in this case the patches are sort of interdependent, so I
> didn't want to mess with a rebase and sending conflicting patches... I
> did move the bugfix patch to the bottom so that at worst you can pick
> it out and ask me to resubmit the rest some other way.

That's understandable, but it results in a patchset which is never
going to be applied as a whole (the bug fix(es) would go to stable-X,
the rest to next).  It's not the end of the world, but it's nice when
there is at least some hope that a patchset could be merged all at
once.  Moving forward I would suggest keeping the fixes separate and
submitting the cleanups in a separate patchset with a note that the
depend on the other patch(es).  Depending on the particular patches
I'll either just merge them and deal with the conflicts, or hold the
cleanups until they can be applied without conflict.

> Since I'll need to respin the bugfix patch, will it be better if I
> repost just the one patch?

Let's try the above suggestion and see how it works; if it turns out
to be a mess, we can work on "patching" the process :)

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-03-31 23:19       ` Paul Moore
@ 2021-04-01  7:59         ` Ondrej Mosnacek
  2021-04-01 15:36           ` Paul Moore
  0 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-04-01  7:59 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, Stephen Smalley

On Thu, Apr 1, 2021 at 1:20 AM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> ...
>
> > > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > > >  {
> > > > -       int i;
> > > > -       struct avtab_node *node, *tmp, *tail;
> > > > -
> > > > -       memset(new, 0, sizeof(*new));
> > > > +       int rc;
> > > > +       u32 shift = 0;
> > > > +       u32 work = nrules;
> > > > +       u32 nslot = 0;
> > > >
> > > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > > -       if (!new->htable)
> > > > -               return -ENOMEM;
> > > > -       new->nslot = orig->nslot;
> > > > -       new->mask = orig->mask;
> > > > -
> > > > -       for (i = 0; i < orig->nslot; i++) {
> > > > -               tail = NULL;
> > > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > > -                       if (!tmp)
> > > > -                               goto error;
> > > > -                       tmp->key = node->key;
> > > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > > -                               tmp->datum.u.xperms =
> > > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > > -                                                       GFP_KERNEL);
> > > > -                               if (!tmp->datum.u.xperms) {
> > > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > > -                                       goto error;
> > > > -                               }
> > > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > > -                       } else
> > > > -                               tmp->datum.u.data = node->datum.u.data;
> > > > -
> > > > -                       if (tail)
> > > > -                               tail->next = tmp;
> > > > -                       else
> > > > -                               new->htable[i] = tmp;
> > > > -
> > > > -                       tail = tmp;
> > > > -                       new->nel++;
> > > > +       if (nrules != 0) {
> > > > +               while (work) {
> > > > +                       work  = work >> 1;
> > >
> > > Extra  horizontal  spaces  are  awkward  and  bad.
> > >
> > > > +                       shift++;
> > > >                 }
> > > > +               if (shift > 2)
> > > > +                       shift = shift - 2;
> > >
> > > Since we are getting nit-picky with this code, why not make the
> > > loop/if a bit more elegant?  How about something like below?
> > >
> > >   u32 shift = 2;
> > >   u32 work = nrules >> 4;
> > >   while (work) {
> > >     work >>= 1;
> > >     shift++;
> > >   }
> >
> > I think you meant:
> > u32 shift = 0;
> > u32 work = nrules >> 2;
> > while (work) {
> >     work >>= 1;
> >     shift++;
> > }
> >
> > ...which is equivalent to the current code and yes, I'll use that :)
>
> Well, no, not really, but that's okay as looking at it now we both got
> it wrong :)
>
> Basically I wanted to avoid the odd problem where the current code has
> a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
> or 7.  While the code I proposed yesterday did that, it inflated the
> number of buckets beyond the current code; your suggestion had
> problems with low numbers resulting in zero buckets.

Aah, I wrongly parsed the "if (shift > 2) shift -= 2;" statement...
Yeah, I guess even the original intent was different than what the
code does.

Anyway, my code doesn't result in zero buckets, at worst in 1 bucket
(nslot = 1 << shift), which I think is reasonable given that the
intent of the code seems to be to simply squeeze the number of slots
down to approx. 1/4 the number of rules.

To make it a bit more concrete: your code allocates 4 buckets for
nrules 1..15; my code allocates 1 bucket for nrules 1..3, 2 buckets
for nrules 4..7, and 4 buckets for 8..15. Both our solutions are
equivalent to the old code at nrules >= 8.

So I'll argue that my proposed solution is actually slightly better
(and avoids adding a new magic number).

>
> I think what we really want is this:
>
>   shift = 2;
>   work = nrules >> 4;
>   while (work) {
>     work >>= 1;
>     shift++;
>   }
>
> ... it avoids any dips in the bucket count and it results in similar
> bucket counts as the existing code for larger numbers of rules.

But that's exactly the same as your original suggestion :D

--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-04-01  7:59         ` Ondrej Mosnacek
@ 2021-04-01 15:36           ` Paul Moore
  2021-04-01 15:54             ` Ondrej Mosnacek
  0 siblings, 1 reply; 16+ messages in thread
From: Paul Moore @ 2021-04-01 15:36 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley

On Thu, Apr 1, 2021 at 3:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Apr 1, 2021 at 1:20 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > ...
> >
> > > > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > > > >  {
> > > > > -       int i;
> > > > > -       struct avtab_node *node, *tmp, *tail;
> > > > > -
> > > > > -       memset(new, 0, sizeof(*new));
> > > > > +       int rc;
> > > > > +       u32 shift = 0;
> > > > > +       u32 work = nrules;
> > > > > +       u32 nslot = 0;
> > > > >
> > > > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > > > -       if (!new->htable)
> > > > > -               return -ENOMEM;
> > > > > -       new->nslot = orig->nslot;
> > > > > -       new->mask = orig->mask;
> > > > > -
> > > > > -       for (i = 0; i < orig->nslot; i++) {
> > > > > -               tail = NULL;
> > > > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > > > -                       if (!tmp)
> > > > > -                               goto error;
> > > > > -                       tmp->key = node->key;
> > > > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > > > -                               tmp->datum.u.xperms =
> > > > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > > > -                                                       GFP_KERNEL);
> > > > > -                               if (!tmp->datum.u.xperms) {
> > > > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > > > -                                       goto error;
> > > > > -                               }
> > > > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > > > -                       } else
> > > > > -                               tmp->datum.u.data = node->datum.u.data;
> > > > > -
> > > > > -                       if (tail)
> > > > > -                               tail->next = tmp;
> > > > > -                       else
> > > > > -                               new->htable[i] = tmp;
> > > > > -
> > > > > -                       tail = tmp;
> > > > > -                       new->nel++;
> > > > > +       if (nrules != 0) {
> > > > > +               while (work) {
> > > > > +                       work  = work >> 1;
> > > >
> > > > Extra  horizontal  spaces  are  awkward  and  bad.
> > > >
> > > > > +                       shift++;
> > > > >                 }
> > > > > +               if (shift > 2)
> > > > > +                       shift = shift - 2;
> > > >
> > > > Since we are getting nit-picky with this code, why not make the
> > > > loop/if a bit more elegant?  How about something like below?
> > > >
> > > >   u32 shift = 2;
> > > >   u32 work = nrules >> 4;
> > > >   while (work) {
> > > >     work >>= 1;
> > > >     shift++;
> > > >   }
> > >
> > > I think you meant:
> > > u32 shift = 0;
> > > u32 work = nrules >> 2;
> > > while (work) {
> > >     work >>= 1;
> > >     shift++;
> > > }
> > >
> > > ...which is equivalent to the current code and yes, I'll use that :)
> >
> > Well, no, not really, but that's okay as looking at it now we both got
> > it wrong :)
> >
> > Basically I wanted to avoid the odd problem where the current code has
> > a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
> > or 7.  While the code I proposed yesterday did that, it inflated the
> > number of buckets beyond the current code; your suggestion had
> > problems with low numbers resulting in zero buckets.
>
> Aah, I wrongly parsed the "if (shift > 2) shift -= 2;" statement...
> Yeah, I guess even the original intent was different than what the
> code does.

I think we all mis-interpreted that bit of code at some point.  I
seriously doubt that dip was what the original author intended :)

> Anyway, my code doesn't result in zero buckets, at worst in 1 bucket
> (nslot = 1 << shift) ...

Sorry, yes, I mis-spoke (mis-typed?); I was talking about the
shift/exponent value.

> So I'll argue that my proposed solution is actually slightly better
> (and avoids adding a new magic number).

The magic number argument isn't really valid as both approaches use
them to some degree.  Creating a #define constant is overkill here,
but I guess a short comment wouldn't be a bad idea if you wanted to
add one; I'm not going to require it in this case.

Since we are at -rc5 I really want to wrap this up soon so I'm going
to make one final suggestion:

  shift = 1;
  work = nrules >> 3;
  while (work) {
    work >>= 1;
    shift++;
  }

... this preserves the original nslot count, minus the bump/dip seen
with low rule numbers.  The shift value starts at 1, increases to 2
when the rules reach 8, increases to 3 when the rules reach 16, and so
on.  Of the approaches we've discussed, I believe it is the most
faithful to original intent.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-04-01 15:36           ` Paul Moore
@ 2021-04-01 15:54             ` Ondrej Mosnacek
  2021-04-01 15:56               ` Paul Moore
  0 siblings, 1 reply; 16+ messages in thread
From: Ondrej Mosnacek @ 2021-04-01 15:54 UTC (permalink / raw)
  To: Paul Moore; +Cc: SElinux list, Stephen Smalley

On Thu, Apr 1, 2021 at 5:36 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Apr 1, 2021 at 3:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > On Thu, Apr 1, 2021 at 1:20 AM Paul Moore <paul@paul-moore.com> wrote:
> > > On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > > On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > ...
> > >
> > > > > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > > > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > > > > >  {
> > > > > > -       int i;
> > > > > > -       struct avtab_node *node, *tmp, *tail;
> > > > > > -
> > > > > > -       memset(new, 0, sizeof(*new));
> > > > > > +       int rc;
> > > > > > +       u32 shift = 0;
> > > > > > +       u32 work = nrules;
> > > > > > +       u32 nslot = 0;
> > > > > >
> > > > > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > > > > -       if (!new->htable)
> > > > > > -               return -ENOMEM;
> > > > > > -       new->nslot = orig->nslot;
> > > > > > -       new->mask = orig->mask;
> > > > > > -
> > > > > > -       for (i = 0; i < orig->nslot; i++) {
> > > > > > -               tail = NULL;
> > > > > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > > > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > > > > -                       if (!tmp)
> > > > > > -                               goto error;
> > > > > > -                       tmp->key = node->key;
> > > > > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > > > > -                               tmp->datum.u.xperms =
> > > > > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > > > > -                                                       GFP_KERNEL);
> > > > > > -                               if (!tmp->datum.u.xperms) {
> > > > > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > > > > -                                       goto error;
> > > > > > -                               }
> > > > > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > > > > -                       } else
> > > > > > -                               tmp->datum.u.data = node->datum.u.data;
> > > > > > -
> > > > > > -                       if (tail)
> > > > > > -                               tail->next = tmp;
> > > > > > -                       else
> > > > > > -                               new->htable[i] = tmp;
> > > > > > -
> > > > > > -                       tail = tmp;
> > > > > > -                       new->nel++;
> > > > > > +       if (nrules != 0) {
> > > > > > +               while (work) {
> > > > > > +                       work  = work >> 1;
> > > > >
> > > > > Extra  horizontal  spaces  are  awkward  and  bad.
> > > > >
> > > > > > +                       shift++;
> > > > > >                 }
> > > > > > +               if (shift > 2)
> > > > > > +                       shift = shift - 2;
> > > > >
> > > > > Since we are getting nit-picky with this code, why not make the
> > > > > loop/if a bit more elegant?  How about something like below?
> > > > >
> > > > >   u32 shift = 2;
> > > > >   u32 work = nrules >> 4;
> > > > >   while (work) {
> > > > >     work >>= 1;
> > > > >     shift++;
> > > > >   }
> > > >
> > > > I think you meant:
> > > > u32 shift = 0;
> > > > u32 work = nrules >> 2;
> > > > while (work) {
> > > >     work >>= 1;
> > > >     shift++;
> > > > }
> > > >
> > > > ...which is equivalent to the current code and yes, I'll use that :)
> > >
> > > Well, no, not really, but that's okay as looking at it now we both got
> > > it wrong :)
> > >
> > > Basically I wanted to avoid the odd problem where the current code has
> > > a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
> > > or 7.  While the code I proposed yesterday did that, it inflated the
> > > number of buckets beyond the current code; your suggestion had
> > > problems with low numbers resulting in zero buckets.
> >
> > Aah, I wrongly parsed the "if (shift > 2) shift -= 2;" statement...
> > Yeah, I guess even the original intent was different than what the
> > code does.
>
> I think we all mis-interpreted that bit of code at some point.  I
> seriously doubt that dip was what the original author intended :)
>
> > Anyway, my code doesn't result in zero buckets, at worst in 1 bucket
> > (nslot = 1 << shift) ...
>
> Sorry, yes, I mis-spoke (mis-typed?); I was talking about the
> shift/exponent value.
>
> > So I'll argue that my proposed solution is actually slightly better
> > (and avoids adding a new magic number).
>
> The magic number argument isn't really valid as both approaches use
> them to some degree.  Creating a #define constant is overkill here,
> but I guess a short comment wouldn't be a bad idea if you wanted to
> add one; I'm not going to require it in this case.
>
> Since we are at -rc5 I really want to wrap this up soon so I'm going
> to make one final suggestion:
>
>   shift = 1;
>   work = nrules >> 3;
>   while (work) {
>     work >>= 1;
>     shift++;
>   }
>
> ... this preserves the original nslot count, minus the bump/dip seen
> with low rule numbers.  The shift value starts at 1, increases to 2
> when the rules reach 8, increases to 3 when the rules reach 16, and so
> on.  Of the approaches we've discussed, I believe it is the most
> faithful to original intent.

Ok, I agree it's not something worth obsessing over, so I'll just use
this last suggestion :) (One day maybe I'll try to simplify/optimize
it a bit, but that is for another patch...)

-- 
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.


^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
  2021-04-01 15:54             ` Ondrej Mosnacek
@ 2021-04-01 15:56               ` Paul Moore
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2021-04-01 15:56 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: SElinux list, Stephen Smalley

On Thu, Apr 1, 2021 at 11:54 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> Ok, I agree it's not something worth obsessing over, so I'll just use
> this last suggestion :) (One day maybe I'll try to simplify/optimize
> it a bit, but that is for another patch...)

Sounds good, thanks Ondrej.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup()
  2021-03-30 13:16 ` [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup() Ondrej Mosnacek
@ 2021-05-11  1:34   ` Paul Moore
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2021-05-11  1:34 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley

On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> We can do the allocation + copying of expr.nodes in one go using
> kmemdup().
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/conditional.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Merged into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] selinux: constify some avtab function arguments
  2021-03-30 13:16 ` [PATCH 3/3] selinux: constify some avtab function arguments Ondrej Mosnacek
@ 2021-05-11  1:36   ` Paul Moore
  0 siblings, 0 replies; 16+ messages in thread
From: Paul Moore @ 2021-05-11  1:36 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley

On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> This makes the code a bit easier to reason about.
>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>  security/selinux/ss/avtab.c       | 28 +++++++++++++++-------------
>  security/selinux/ss/avtab.h       | 16 +++++++++-------
>  security/selinux/ss/conditional.c |  3 ++-
>  3 files changed, 26 insertions(+), 21 deletions(-)

Mered into selinux/next, thanks.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-05-11  1:36 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-30 13:16 [PATCH 0/3] selinux: fix changing booleans Ondrej Mosnacek
2021-03-30 13:16 ` [PATCH 1/3] selinux: fix cond_list corruption when " Ondrej Mosnacek
2021-03-31  2:04   ` Paul Moore
2021-03-31  9:12     ` Ondrej Mosnacek
2021-03-31 23:19       ` Paul Moore
2021-04-01  7:59         ` Ondrej Mosnacek
2021-04-01 15:36           ` Paul Moore
2021-04-01 15:54             ` Ondrej Mosnacek
2021-04-01 15:56               ` Paul Moore
2021-03-30 13:16 ` [PATCH 2/3] selinux: simplify duplicate_policydb_cond_list() by using kmemdup() Ondrej Mosnacek
2021-05-11  1:34   ` Paul Moore
2021-03-30 13:16 ` [PATCH 3/3] selinux: constify some avtab function arguments Ondrej Mosnacek
2021-05-11  1:36   ` Paul Moore
2021-03-31  1:13 ` [PATCH 0/3] selinux: fix changing booleans Paul Moore
2021-03-31  8:23   ` Ondrej Mosnacek
2021-03-31 23:26     ` 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.