* [PATCH v2 0/2] selinux: fix changing booleans
@ 2021-04-01 15:59 Ondrej Mosnacek
2021-04-01 15:59 ` [PATCH v2 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek
2021-04-01 15:59 ` [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek
0 siblings, 2 replies; 5+ messages in thread
From: Ondrej Mosnacek @ 2021-04-01 15:59 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.
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] 5+ messages in thread* [PATCH v2 1/2] selinux: make nslot handling in avtab more robust 2021-04-01 15:59 [PATCH v2 0/2] selinux: fix changing booleans Ondrej Mosnacek @ 2021-04-01 15:59 ` Ondrej Mosnacek 2021-04-01 15:59 ` [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek 1 sibling, 0 replies; 5+ messages in thread From: Ondrej Mosnacek @ 2021-04-01 15:59 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] 5+ messages in thread
* [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans 2021-04-01 15:59 [PATCH v2 0/2] selinux: fix changing booleans Ondrej Mosnacek 2021-04-01 15:59 ` [PATCH v2 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek @ 2021-04-01 15:59 ` Ondrej Mosnacek 2021-04-01 23:02 ` Paul Moore 1 sibling, 1 reply; 5+ messages in thread From: Ondrej Mosnacek @ 2021-04-01 15:59 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..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; -- 2.30.2 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans 2021-04-01 15:59 ` [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek @ 2021-04-01 23:02 ` Paul Moore 2021-04-02 8:58 ` Ondrej Mosnacek 0 siblings, 1 reply; 5+ messages in thread From: Paul Moore @ 2021-04-01 23:02 UTC (permalink / raw) To: Ondrej Mosnacek; +Cc: selinux, Stephen Smalley 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; > +} -- paul moore www.paul-moore.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans 2021-04-01 23:02 ` Paul Moore @ 2021-04-02 8:58 ` Ondrej Mosnacek 0 siblings, 0 replies; 5+ messages in thread From: Ondrej Mosnacek @ 2021-04-02 8:58 UTC (permalink / raw) To: Paul Moore; +Cc: SElinux list, Stephen Smalley 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 > -- Ondrej Mosnacek Software Engineer, Linux Security - SELinux kernel Red Hat, Inc. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-04-02 8:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-01 15:59 [PATCH v2 0/2] selinux: fix changing booleans Ondrej Mosnacek 2021-04-01 15:59 ` [PATCH v2 1/2] selinux: make nslot handling in avtab more robust Ondrej Mosnacek 2021-04-01 15:59 ` [PATCH v2 2/2] selinux: fix cond_list corruption when changing booleans Ondrej Mosnacek 2021-04-01 23:02 ` Paul Moore 2021-04-02 8:58 ` Ondrej Mosnacek
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.