| Message ID | 20210330131646.1401838-2-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | selinux: fix changing booleans | expand |
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); > }
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 >
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.
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.
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.
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...)
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.
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;
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(-)