On Wed, Jun 23, 2021 at 4:05 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
>
> On Thu, Jun 17, 2021 at 7:43 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > The commit d155b410d4bbc90d28f361b966f0429598da8188 (libsepol/cil:
> > Check for duplicate blocks, optionals, and macros) fixed a bug
> > that allowed duplicate blocks, optionals, and macros with the same
> > name in the same namespace. For blocks and macros, a duplicate
> > is always a problem, but optional block names are only used for
> > in-statement resolution. If no in-statement refers to an optional
> > block, then it does not matter if more than one with same name
> > exists.
> >
> > One easy way to generate multiple optional blocks with the same
> > name declaration is to call a macro with an optional block multiple
> > times in the same namespace.
> >
> > As an example, here is a portion of CIL policy
> > (macro m1 ((type t))
> > (optional op1
> > (allow t self (CLASS (PERM)))
> > )
> > )
> > (type t1)
> > (call m1 (t1))
> > (type t2)
> > (call m1 (t2))
> > This will result in two optional blocks with the name op1.
> >
> > There are three parts to allowing multiple optional blocks with
> > the same name declaration.
> >
> > 1) Track an optional block's enabled status in a different way.
> >
> > One hinderance to allowing multiple optional blocks with the same
> > name declaration is that they cannot share the same datum. This is
> > because the datum is used to get the struct cil_optional which has
> > the enabled field and each block's enabled status is independent of
> > the others.
> >
> > Remove the enabled field from struct cil_optional, so it only contains
> > the datum. Use a stack to track which optional blocks are being
> > disabled, so they can be deleted in the right order.
> >
> > 2) Allow multiple declarations of optional blocks.
> >
> > Update cil_allow_multiple_decls() so that a flavor of CIL_OPTIONAL
> > will return CIL_TRUE. Also remove the check in cil_copy_optional().
> >
> > 3) Check if an in-statement refers to an optional with multiple
> > declarations and exit with an error if it does.
> >
> > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
>
> Hello,
>
> This patch looks good to me, even though it conflicts with the pending
> patch "libsepol/cil: Improve degenerate inheritance check" in
> libsepol/cil/src/cil_resolve_ast.c:
>
> ++<<<<<<< HEAD
> + struct cil_stack *disabled_optionals;
> ++=======
> + int *inheritance_check;
> ++>>>>>>> c8dde7093e4a (libsepol/cil: Improve degenerate inheritance check)
> ...
> ++<<<<<<< HEAD
> + extra_args.disabled_optionals = NULL;
> ++=======
> + extra_args.inheritance_check = &inheritance_check;
> ++>>>>>>> c8dde7093e4a (libsepol/cil: Improve degenerate inheritance check)
>
> I guess both additions need to be kept when merging both patches
>
> Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
>
Yes, I will have to resolve that conflict when I merge them.
Thanks for the review,
Jim
> > ---
> > libsepol/cil/src/cil.c | 1 -
> > libsepol/cil/src/cil_build_ast.c | 3 ++
> > libsepol/cil/src/cil_copy_ast.c | 11 +-----
> > libsepol/cil/src/cil_internal.h | 1 -
> > libsepol/cil/src/cil_resolve_ast.c | 55 ++++++++++++++++++------------
> > 5 files changed, 37 insertions(+), 34 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil.c b/libsepol/cil/src/cil.c
> > index 0d351b49..671b5ec6 100644
> > --- a/libsepol/cil/src/cil.c
> > +++ b/libsepol/cil/src/cil.c
> > @@ -2752,7 +2752,6 @@ void cil_call_init(struct cil_call **call)
> > void cil_optional_init(struct cil_optional **optional)
> > {
> > *optional = cil_malloc(sizeof(**optional));
> > - (*optional)->enabled = CIL_TRUE;
> > cil_symtab_datum_init(&(*optional)->datum);
> > }
> >
> > diff --git a/libsepol/cil/src/cil_build_ast.c b/libsepol/cil/src/cil_build_ast.c
> > index 71f14e20..2c72accc 100644
> > --- a/libsepol/cil/src/cil_build_ast.c
> > +++ b/libsepol/cil/src/cil_build_ast.c
> > @@ -96,6 +96,9 @@ static int cil_allow_multiple_decls(struct cil_db *db, enum cil_flavor f_new, en
> > return CIL_TRUE;
> > }
> > break;
> > + case CIL_OPTIONAL:
> > + return CIL_TRUE;
> > + break;
> > default:
> > break;
> > }
> > diff --git a/libsepol/cil/src/cil_copy_ast.c b/libsepol/cil/src/cil_copy_ast.c
> > index 954eab33..9c0231f2 100644
> > --- a/libsepol/cil/src/cil_copy_ast.c
> > +++ b/libsepol/cil/src/cil_copy_ast.c
> > @@ -1529,19 +1529,10 @@ int cil_copy_macro(__attribute__((unused)) struct cil_db *db, void *data, void *
> > return SEPOL_OK;
> > }
> >
> > -int cil_copy_optional(__attribute__((unused)) struct cil_db *db, void *data, void **copy, symtab_t *symtab)
> > +int cil_copy_optional(__attribute__((unused)) struct cil_db *db, __attribute__((unused)) void *data, void **copy, __attribute__((unused)) symtab_t *symtab)
> > {
> > - struct cil_optional *orig = data;
> > - char *key = orig->datum.name;
> > - struct cil_symtab_datum *datum = NULL;
> > struct cil_optional *new;
> >
> > - cil_symtab_get_datum(symtab, key, &datum);
> > - if (datum != NULL) {
> > - cil_tree_log(NODE(datum), CIL_ERR, "Re-declaration of %s %s", cil_node_to_string(NODE(datum)), key);
> > - return SEPOL_ERR;
> > - }
> > -
> > cil_optional_init(&new);
> > *copy = new;
> >
> > diff --git a/libsepol/cil/src/cil_internal.h b/libsepol/cil/src/cil_internal.h
> > index a77c9520..24be09aa 100644
> > --- a/libsepol/cil/src/cil_internal.h
> > +++ b/libsepol/cil/src/cil_internal.h
> > @@ -358,7 +358,6 @@ struct cil_in {
> >
> > struct cil_optional {
> > struct cil_symtab_datum datum;
> > - int enabled;
> > };
> >
> > struct cil_perm {
> > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > index 77ffe0ff..62e0e013 100644
> > --- a/libsepol/cil/src/cil_resolve_ast.c
> > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > @@ -46,12 +46,13 @@
> > #include "cil_verify.h"
> > #include "cil_strpool.h"
> > #include "cil_symtab.h"
> > +#include "cil_stack.h"
> >
> > struct cil_args_resolve {
> > struct cil_db *db;
> > enum cil_pass pass;
> > uint32_t *changed;
> > - struct cil_list *disabled_optionals;
> > + struct cil_list *to_destroy;
> > struct cil_tree_node *block;
> > struct cil_tree_node *macro;
> > struct cil_tree_node *optional;
> > @@ -62,6 +63,7 @@ struct cil_args_resolve {
> > struct cil_list *catorder_lists;
> > struct cil_list *sensitivityorder_lists;
> > struct cil_list *in_list;
> > + struct cil_stack *disabled_optionals;
> > };
> >
> > static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key, struct cil_tree_node *ast_node)
> > @@ -2552,6 +2554,15 @@ int cil_resolve_in(struct cil_tree_node *current, void *extra_args)
> >
> > block_node = NODE(block_datum);
> >
> > + if (block_node->flavor == CIL_OPTIONAL) {
> > + if (block_datum->nodes && block_datum->nodes->head != block_datum->nodes->tail) {
> > + cil_tree_log(current, CIL_ERR, "Multiple optional blocks referred to by in-statement");
> > + cil_tree_log(block_node, CIL_ERR, "First optional block");
> > + rc = SEPOL_ERR;
> > + goto exit;
> > + }
> > + }
> > +
> > rc = cil_copy_ast(db, current, block_node);
> > if (rc != SEPOL_OK) {
> > cil_tree_log(current, CIL_ERR, "Failed to copy in-statement");
> > @@ -3867,6 +3878,7 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > struct cil_tree_node *macro = args->macro;
> > struct cil_tree_node *optional = args->optional;
> > struct cil_tree_node *boolif = args->boolif;
> > + struct cil_stack *disabled_optionals = args->disabled_optionals;
> >
> > if (node == NULL) {
> > goto exit;
> > @@ -3946,22 +3958,14 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> >
> > rc = __cil_resolve_ast_node(node, extra_args);
> > if (rc == SEPOL_ENOENT) {
> > - enum cil_log_level lvl = CIL_ERR;
> > -
> > - if (optional != NULL) {
> > - lvl = CIL_INFO;
> > -
> > - struct cil_optional *opt = (struct cil_optional *)optional->data;
> > - struct cil_tree_node *opt_node = NODE(opt);;
> > - /* disable an optional if something failed to resolve */
> > - opt->enabled = CIL_FALSE;
> > - cil_tree_log(node, lvl, "Failed to resolve %s statement", cil_node_to_string(node));
> > - cil_tree_log(opt_node, lvl, "Disabling optional '%s'", opt->datum.name);
> > + if (optional == NULL) {
> > + cil_tree_log(node, CIL_ERR, "Failed to resolve %s statement", cil_node_to_string(node));
> > + } else {
> > + cil_stack_push(disabled_optionals, CIL_NODE, optional);
> > + cil_tree_log(node, CIL_INFO, "Failed to resolve %s statement", cil_node_to_string(node));
> > + cil_tree_log(optional, CIL_INFO, "Disabling optional '%s'", DATUM(optional->data)->name);
> > rc = SEPOL_OK;
> > - goto exit;
> > }
> > -
> > - cil_tree_log(node, lvl, "Failed to resolve %s statement", cil_node_to_string(node));
> > goto exit;
> > }
> >
> > @@ -4004,6 +4008,7 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > {
> > int rc = SEPOL_ERR;
> > struct cil_args_resolve *args = extra_args;
> > + struct cil_stack *disabled_optionals = args->disabled_optionals;
> > struct cil_tree_node *parent = NULL;
> >
> > if (current == NULL || extra_args == NULL) {
> > @@ -4026,9 +4031,11 @@ int __cil_resolve_ast_last_child_helper(struct cil_tree_node *current, void *ext
> > args->macro = NULL;
> > } else if (parent->flavor == CIL_OPTIONAL) {
> > struct cil_tree_node *n = parent->parent;
> > - if (((struct cil_optional *)parent->data)->enabled == CIL_FALSE) {
> > + struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> > + if (item && item->data == parent) {
> > + cil_stack_pop(disabled_optionals);
> > *(args->changed) = CIL_TRUE;
> > - cil_list_append(args->disabled_optionals, CIL_NODE, parent);
> > + cil_list_append(args->to_destroy, CIL_NODE, parent);
> > }
> > args->optional = NULL;
> > while (n && n->flavor != CIL_ROOT) {
> > @@ -4072,14 +4079,17 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > extra_args.catorder_lists = NULL;
> > extra_args.sensitivityorder_lists = NULL;
> > extra_args.in_list = NULL;
> > + extra_args.disabled_optionals = NULL;
> >
> > - cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
> > + cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.classorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.unordered_classorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.catorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.sensitivityorder_lists, CIL_LIST_ITEM);
> > cil_list_init(&extra_args.in_list, CIL_IN);
> > + cil_stack_init(&extra_args.disabled_optionals);
> > +
> > for (pass = CIL_PASS_TIF; pass < CIL_PASS_NUM; pass++) {
> > extra_args.pass = pass;
> > rc = cil_tree_walk(current, __cil_resolve_ast_node_helper, __cil_resolve_ast_first_child_helper, __cil_resolve_ast_last_child_helper, &extra_args);
> > @@ -4172,11 +4182,11 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > goto exit;
> > }
> > }
> > - cil_list_for_each(item, extra_args.disabled_optionals) {
> > + cil_list_for_each(item, extra_args.to_destroy) {
> > cil_tree_children_destroy(item->data);
> > }
> > - cil_list_destroy(&extra_args.disabled_optionals, CIL_FALSE);
> > - cil_list_init(&extra_args.disabled_optionals, CIL_NODE);
> > + cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > + cil_list_init(&extra_args.to_destroy, CIL_NODE);
> > changed = 0;
> > }
> > }
> > @@ -4193,8 +4203,9 @@ exit:
> > __cil_ordered_lists_destroy(&extra_args.catorder_lists);
> > __cil_ordered_lists_destroy(&extra_args.sensitivityorder_lists);
> > __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > - cil_list_destroy(&extra_args.disabled_optionals, CIL_FALSE);
> > + cil_list_destroy(&extra_args.to_destroy, CIL_FALSE);
> > cil_list_destroy(&extra_args.in_list, CIL_FALSE);
> > + cil_stack_destroy(&extra_args.disabled_optionals);
> >
> > return rc;
> > }
> > --
> > 2.26.3
> >
>
