On Thu, Apr 1, 2021 at 11:59 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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
