Re: [PATCH 1/3] libsepol/cil: Don't destroy optionals whose parent will be destroyed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Sep 2, 2021 at 6:33 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Wed, Sep 1, 2021 at 3:26 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
> >
> >  On Mon, Aug 30, 2021 at 8:24 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > >
> > > If an optional that is to be disabled is the child of an optional that
> > > is going to be disabled, then there is no reason to add that optional
> > > to the stack of disabled optionals, because it is going to be destroyed
> > > anyways. This means that there is no reason to maintain a stack of
> > > disabled optionals at all.
> > >
> > > Instead of using a stack to track disabled optionals, use a pointer
> > > that points to the top-most optional that is to be disabled. When a
> > > rule fails to resolve in an optional, if the disabled optional pointer
> > > has not been set, then set it to that optional. If the pointer has
> > > been set already, then the optional is already going to be destroyed,
> > > so nothing else needs to be done. The resolution failure and the fact
> > > that the optional is being disabled is reported in either case.
> > >
> > > Signed-off-by: James Carter <jwcart2@xxxxxxxxx>
> >
> > For the 3 patches:
> >
> > Acked-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
> >
>
> This series has been merged.
>
> > (I liked these simplifications, and if someone encounters a stack
> > exhaustion issue because of changing cil_tree_children_destroy to a
> > recursive function, I guess a reasonable recursion limit could be
> > added where it would make sense)
> >
> Yes, my thought was that since cil_tree_walk() & cil_tree_walk_core()
> have similar
> limits, that it wouldn't be a problem. And, if it was, we could add a
> recursion limit as you suggest.
>
> Thanks,
> Jim

Hi,
I did not take long for OSS-Fuzz to find a crash due to the
cil_tree_children_destroy change:
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=38110 (this link
will be publicly open on 2021-12-03 at the latest).

Here is a simple reproducer, in shell:

   (echo '(' ; for i in $(seq 300000) ; do echo ';;*lmx 3 r' ; done) >
test.cil && secilc test.cil

On my system (x86_64 Arch Linux), this triggers a segmentation fault,
probably due to stack exhaustion, with maaaaany recursive calls to
cil_tree_children_destroy. Using the fuzzer, the limit is smaller
(with "seq 50000" I manage to trigger a segfault). So I guess the
number of successive line markers should be limited too.

Thanks,
Nicolas

> > Thanks,
> > Nicolas
> >
> > > ---
> > >  libsepol/cil/src/cil_resolve_ast.c | 17 +++++++----------
> > >  1 file changed, 7 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> > > index 18007324..aeedc7dd 100644
> > > --- a/libsepol/cil/src/cil_resolve_ast.c
> > > +++ b/libsepol/cil/src/cil_resolve_ast.c
> > > @@ -56,6 +56,7 @@ struct cil_args_resolve {
> > >         struct cil_tree_node *block;
> > >         struct cil_tree_node *macro;
> > >         struct cil_tree_node *optional;
> > > +       struct cil_tree_node *disabled_optional;
> > >         struct cil_tree_node *boolif;
> > >         struct cil_list *sidorder_lists;
> > >         struct cil_list *classorder_lists;
> > > @@ -63,7 +64,6 @@ 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)
> > > @@ -3873,7 +3873,6 @@ 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;
> > > @@ -3956,7 +3955,9 @@ int __cil_resolve_ast_node_helper(struct cil_tree_node *node, uint32_t *finished
> > >                 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);
> > > +                       if (!args->disabled_optional) {
> > > +                               args->disabled_optional = 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;
> > > @@ -4003,7 +4004,6 @@ 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,11 +4026,10 @@ 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;
> > > -               struct cil_stack_item *item = cil_stack_peek(disabled_optionals);
> > > -               if (item && item->data == parent) {
> > > -                       cil_stack_pop(disabled_optionals);
> > > +               if (args->disabled_optional == parent) {
> > >                         *(args->changed) = CIL_TRUE;
> > >                         cil_list_append(args->to_destroy, CIL_NODE, parent);
> > > +                       args->disabled_optional = NULL;
> > >                 }
> > >                 args->optional = NULL;
> > >                 while (n && n->flavor != CIL_ROOT) {
> > > @@ -4067,6 +4066,7 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > >         extra_args.block = NULL;
> > >         extra_args.macro = NULL;
> > >         extra_args.optional = NULL;
> > > +       extra_args.disabled_optional = NULL;
> > >         extra_args.boolif= NULL;
> > >         extra_args.sidorder_lists = NULL;
> > >         extra_args.classorder_lists = NULL;
> > > @@ -4074,7 +4074,6 @@ 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.to_destroy, CIL_NODE);
> > >         cil_list_init(&extra_args.sidorder_lists, CIL_LIST_ITEM);
> > > @@ -4083,7 +4082,6 @@ int cil_resolve_ast(struct cil_db *db, struct cil_tree_node *current)
> > >         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;
> > > @@ -4218,7 +4216,6 @@ exit:
> > >         __cil_ordered_lists_destroy(&extra_args.unordered_classorder_lists);
> > >         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.31.1
> > >
> >




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux