| Message ID | 20210401155913.1693097-3-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: fix changing booleans | expand |
On Thu, Apr 1, 2021 at 11:59 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(-) ... > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > index 2aee4c965c25..f9d60010041e 100644 > --- a/security/selinux/ss/avtab.c > +++ b/security/selinux/ss/avtab.c > @@ -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; Now that all of the avtab initialization is done in avtab_init(), thanks to patch 1/2, it doesn't really make sense to call avtab_alloc_common() here when 'nrules == 0', right? Granted it is safe since you check for zero rules in the function, but making the call in the first place seems a bit pointless. I would suggest either moving the avtab_alloc_common() call up into the if body above it or checking for 'nrules == 0' at the top of the function and jumping down to the pr_debug()/return if true. > + pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules); > return 0; > -error: > - avtab_destroy(new); > - return -ENOMEM; > +}
On Fri, Apr 2, 2021 at 1:02 AM Paul Moore <paul@paul-moore.com> wrote: > On Thu, Apr 1, 2021 at 11:59 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(-) > > ... > > > diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c > > index 2aee4c965c25..f9d60010041e 100644 > > --- a/security/selinux/ss/avtab.c > > +++ b/security/selinux/ss/avtab.c > > @@ -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; > > Now that all of the avtab initialization is done in avtab_init(), > thanks to patch 1/2, it doesn't really make sense to call > avtab_alloc_common() here when 'nrules == 0', right? Granted it is > safe since you check for zero rules in the function, but making the > call in the first place seems a bit pointless. I would suggest either > moving the avtab_alloc_common() call up into the if body above it or > checking for 'nrules == 0' at the top of the function and jumping down > to the pr_debug()/return if true. Ok, moved it under the conditional in v3. > > > + pr_debug("SELinux: %d avtab hash slots, %d rules.\n", nslot, nrules); > > return 0; > > -error: > > - avtab_destroy(new); > > - return -ENOMEM; > > +} > > -- > paul moore > www.paul-moore.com >
diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index 2aee4c965c25..f9d60010041e 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;
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(-)