* [PATCH v3 0/2] selinux: fix changing booleans
@ 2021-04-02 8:56 Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek
0 siblings, 2 replies; 4+ messages in thread
From: Ondrej Mosnacek @ 2021-04-02 8:56 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.
v3:
- move the avtab_alloc_common() call in avtab_alloc() under the
conditional block
v2:
- drop the follow-up cleanup patches from this series
- add a patch fixing the current handling of nrules/nslots being zero
- fix this handling also in the original v1 patch
- simplify the loop that computes nslots
Ondrej Mosnacek (2):
selinux: make nslot handling in avtab more robust
selinux: fix cond_list corruption when changing booleans
security/selinux/ss/avtab.c | 101 ++++++++++--------------------
security/selinux/ss/avtab.h | 2 +-
security/selinux/ss/conditional.c | 12 ++--
3 files changed, 40 insertions(+), 75 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v3 1/2] selinux: make nslot handling in avtab more robust
2021-04-02 8:56 [PATCH v3 0/2] selinux: fix changing booleans Ondrej Mosnacek
@ 2021-04-02 8:56 ` Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek
1 sibling, 0 replies; 4+ messages in thread
From: Ondrej Mosnacek @ 2021-04-02 8:56 UTC (permalink / raw)
To: selinux, Paul Moore; +Cc: Stephen Smalley
1. Make sure all fileds are initialized in avtab_init().
2. Slightly refactor avtab_alloc() to use the above fact.
3. Use h->nslot == 0 as a sentinel in the access functions to prevent
dereferencing h->htable when it's not allocated.
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/avtab.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 6dcb6aa4db7f..2aee4c965c25 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -109,7 +109,7 @@ static int avtab_insert(struct avtab *h, struct avtab_key *key, struct avtab_dat
struct avtab_node *prev, *cur, *newnode;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
- if (!h)
+ if (!h || !h->nslot)
return -EINVAL;
hvalue = avtab_hash(key, h->mask);
@@ -154,7 +154,7 @@ avtab_insert_nonunique(struct avtab *h, struct avtab_key *key, struct avtab_datu
struct avtab_node *prev, *cur;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
- if (!h)
+ if (!h || !h->nslot)
return NULL;
hvalue = avtab_hash(key, h->mask);
for (prev = NULL, cur = h->htable[hvalue];
@@ -184,7 +184,7 @@ struct avtab_datum *avtab_search(struct avtab *h, struct avtab_key *key)
struct avtab_node *cur;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
- if (!h)
+ if (!h || !h->nslot)
return NULL;
hvalue = avtab_hash(key, h->mask);
@@ -220,7 +220,7 @@ avtab_search_node(struct avtab *h, struct avtab_key *key)
struct avtab_node *cur;
u16 specified = key->specified & ~(AVTAB_ENABLED|AVTAB_ENABLED_OLD);
- if (!h)
+ if (!h || !h->nslot)
return NULL;
hvalue = avtab_hash(key, h->mask);
@@ -295,6 +295,7 @@ void avtab_destroy(struct avtab *h)
}
kvfree(h->htable);
h->htable = NULL;
+ h->nel = 0;
h->nslot = 0;
h->mask = 0;
}
@@ -303,14 +304,15 @@ void avtab_init(struct avtab *h)
{
h->htable = NULL;
h->nel = 0;
+ h->nslot = 0;
+ h->mask = 0;
}
int avtab_alloc(struct avtab *h, u32 nrules)
{
- u32 mask = 0;
u32 shift = 0;
u32 work = nrules;
- u32 nslot = 0;
+ u32 nslot;
if (nrules == 0)
goto avtab_alloc_out;
@@ -324,16 +326,15 @@ int avtab_alloc(struct avtab *h, u32 nrules)
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;
+ h->mask = nslot - 1;
+
+avtab_alloc_out:
pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
h->nslot, nrules);
return 0;
--
2.30.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans
2021-04-02 8:56 [PATCH v3 0/2] selinux: fix changing booleans Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek
@ 2021-04-02 8:56 ` Ondrej Mosnacek
2021-04-02 15:50 ` Paul Moore
1 sibling, 1 reply; 4+ messages in thread
From: Ondrej Mosnacek @ 2021-04-02 8:56 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
While there, also simplify the computation of nslots. This changes the
nslots values for nrules 2 or 3 to just two slots instead of 4, which
makes the sequence more consistent.
Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
security/selinux/ss/avtab.c | 88 +++++++++----------------------
security/selinux/ss/avtab.h | 2 +-
security/selinux/ss/conditional.c | 12 ++---
3 files changed, 33 insertions(+), 69 deletions(-)
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c
index 2aee4c965c25..75df32906055 100644
--- a/security/selinux/ss/avtab.c
+++ b/security/selinux/ss/avtab.c
@@ -308,24 +308,10 @@ void avtab_init(struct avtab *h)
h->mask = 0;
}
-int avtab_alloc(struct avtab *h, u32 nrules)
+static int avtab_alloc_common(struct avtab *h, u32 nslot)
{
- u32 shift = 0;
- u32 work = nrules;
- u32 nslot;
-
- 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;
+ if (!nslot)
+ return 0;
h->htable = kvcalloc(nslot, sizeof(void *), GFP_KERNEL);
if (!h->htable)
@@ -333,59 +319,37 @@ int avtab_alloc(struct avtab *h, u32 nrules)
h->nslot = nslot;
h->mask = nslot - 1;
-
-avtab_alloc_out:
- pr_debug("SELinux: %d avtab hash slots, %d rules.\n",
- h->nslot, nrules);
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));
-
- 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++;
+ int rc;
+ u32 nslot = 0;
+
+ if (nrules != 0) {
+ u32 shift = 1;
+ u32 work = nrules >> 3;
+ 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);
}
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] 4+ messages in thread
* Re: [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans
2021-04-02 8:56 ` [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek
@ 2021-04-02 15:50 ` Paul Moore
0 siblings, 0 replies; 4+ messages in thread
From: Paul Moore @ 2021-04-02 15:50 UTC (permalink / raw)
To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley
On Fri, Apr 2, 2021 at 4:56 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
>
> While there, also simplify the computation of nslots. This changes the
> nslots values for nrules 2 or 3 to just two slots instead of 4, which
> makes the sequence more consistent.
>
> Fixes: c7c556f1e81b ("selinux: refactor changing booleans")
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
> security/selinux/ss/avtab.c | 88 +++++++++----------------------
> security/selinux/ss/avtab.h | 2 +-
> security/selinux/ss/conditional.c | 12 ++---
> 3 files changed, 33 insertions(+), 69 deletions(-)
Thanks. I just merged both patch 1/2 and this patch into
selinux/stable-5.12, tagging both for -stable. Assuming the test runs
come back clean, I'll send these up to Linus early next week.
--
paul moore
www.paul-moore.com
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-02 15:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-02 8:56 [PATCH v3 0/2] selinux: fix changing booleans Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek
2021-04-02 8:56 ` [PATCH v3 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek
2021-04-02 15:50 ` 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.