Re: [PATCH] libsepol/cil: Fix double free in cil_deny.c

On Fri, Jul 24, 2026 at 11:09 AM James Carter <[email protected]> wrote:
>
> The function cil_create_attribute_d1_and_not_d2() (let's call this
> f1) mallocs an ebitmap named types. This ebitmap is passed to the
> function cil_create_and_insert_attribute_and_set() (let's call this
> f2) which will create a new typeattribute which has its types field
> point to the types ebitmap passed in by f1. If there is an error in
> getting a symtab or inserting the new attribute into the symtab,
> then cil_destroy_typeattribute() is called during cleanup. This will
> free the ebitmap pointed to by the types field. After f2 exits with
> an error, f1 will perform cleanup and try to free the types ebitmap
> again.
>
> Move the calls to cil_get_symtab() and cil_symtab_insert() to
> earlier in f2 to make it easier to clean up in the case of an error.
> When cleaning up from an error do the cleanup manually instead of
> calling cil_destroy_typeattribute() and do not free the passed in
> types ebitmap. Function f1 needs to call ebitmap_destroy() (which
> is safe to call more than once on a ebitmap) and free the ebitmap.
>
> Also add ebitmap_destroy() calls to
> cil_create_attribute_all_and_not_d() and
> cil_create_attribute_d1_and_d2()
>
> Reported-by: oss-fuzz (issue 500754862)
> Signed-off-by: James Carter <[email protected]>

Acked-by: Stephen Smalley <[email protected]>

> ---
>  libsepol/cil/src/cil_deny.c | 44 ++++++++++++++++++-------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_deny.c b/libsepol/cil/src/cil_deny.c
> index c81b8beb..dc0f4ae9 100644
> --- a/libsepol/cil/src/cil_deny.c
> +++ b/libsepol/cil/src/cil_deny.c
> @@ -870,7 +870,6 @@ static int cil_create_and_insert_attribute_and_set(struct cil_db *db,
>         struct cil_tree_node *attr_node = NULL;
>         char *name;
>         struct cil_typeattribute *attr = NULL;
> -       struct cil_tree_node *attrset_node = NULL;
>         struct cil_typeattributeset *attrset = NULL;
>         symtab_t *symtab = NULL;
>         int rc = SEPOL_ERR;
> @@ -880,12 +879,25 @@ static int cil_create_and_insert_attribute_and_set(struct cil_db *db,
>                 goto exit;
>         }
>
> +       rc = cil_get_symtab(prev->parent, &symtab, CIL_SYM_TYPES);
> +       if (rc != SEPOL_OK) {
> +               goto exit;
> +       }
> +
> +       cil_typeattribute_init(&attr);
> +       attr_node = cil_create_and_insert_node(prev, CIL_TYPEATTRIBUTE, attr);
> +
> +       rc = cil_symtab_insert(symtab, name, &attr->datum, attr_node);
> +       if (rc != SEPOL_OK) {
> +               goto exit;
> +       }
> +
>         cil_typeattributeset_init(&attrset);
>         attrset->attr_str = name;
>         attrset->str_expr = str_expr;
>         attrset->datum_expr = datum_expr;
> +       cil_create_and_insert_node(attr_node, CIL_TYPEATTRIBUTESET, attrset);
>
> -       cil_typeattribute_init(&attr);
>         cil_list_init(&attr->expr_list, CIL_TYPE);
>         cil_list_append(attr->expr_list, CIL_LIST, datum_expr);
>         attr->types = types;
> @@ -894,20 +906,6 @@ static int cil_create_and_insert_attribute_and_set(struct cil_db *db,
>                              CIL_FALSE :
>                              CIL_TRUE;
>
> -       attr_node = cil_create_and_insert_node(prev, CIL_TYPEATTRIBUTE, attr);
> -       attrset_node = cil_create_and_insert_node(
> -               attr_node, CIL_TYPEATTRIBUTESET, attrset);
> -
> -       rc = cil_get_symtab(prev->parent, &symtab, CIL_SYM_TYPES);
> -       if (rc != SEPOL_OK) {
> -               goto exit;
> -       }
> -
> -       rc = cil_symtab_insert(symtab, name, &attr->datum, attr_node);
> -       if (rc != SEPOL_OK) {
> -               goto exit;
> -       }
> -
>         db->num_types_and_attrs++;
>
>         *d = &attr->datum;
> @@ -915,15 +913,14 @@ static int cil_create_and_insert_attribute_and_set(struct cil_db *db,
>         return SEPOL_OK;
>
>  exit:
> +       if (attr) {
> +               cil_symtab_datum_destroy(&attr->datum);
> +               free(attr);
> +       }
>         if (attr_node) {
> -               cil_destroy_typeattribute(
> -                       attr_node->data); // This will not destroy datum_expr
> +               prev->next = attr_node->next;
>                 free(attr_node);
>         }
> -       if (attrset_node) {
> -               prev->next = attrset_node->next;
> -               free(attrset_node);
> -       }
>         return rc;
>  }
>
> @@ -1038,6 +1035,7 @@ static int cil_create_attribute_all_and_not_d(struct cil_db *db,
>  exit:
>         cil_list_destroy(&str_expr, CIL_FALSE);
>         cil_list_destroy(&datum_expr, CIL_FALSE);
> +       ebitmap_destroy(types);
>         free(types);
>         return rc;
>  }
> @@ -1103,6 +1101,7 @@ static int cil_create_attribute_d1_and_not_d2(struct cil_db *db,
>  exit:
>         cil_list_destroy(&str_expr, CIL_FALSE);
>         cil_list_destroy(&datum_expr, CIL_FALSE);
> +       ebitmap_destroy(types);
>         free(types);
>         return rc;
>  }
> @@ -1168,6 +1167,7 @@ static int cil_create_attribute_d1_and_d2(struct cil_db *db,
>  exit:
>         cil_list_destroy(&str_expr, CIL_FALSE);
>         cil_list_destroy(&datum_expr, CIL_FALSE);
> +       ebitmap_destroy(types);
>         free(types);
>         return rc;
>  }
> --
> 2.55.0
>