| Message ID | 20190517123006.5788-1-sven@narfation.org |
|---|---|
| State | Accepted |
| Delegated to: | Hans Dedecker |
| Headers | show |
| Series | [OpenWrt-Devel,v2] uci: fix options list of section aftertype change | expand |
On Fri, May 17, 2019 at 2:30 PM Sven Eckelmann <sven@narfation.org> wrote: > > A section can store its name in the same memory region as the section > (after the actual section object). The object is then reallocated when the > type is later changed via an uci_set. But the original address of the > section is (indirectly) stored in the section list, the object and the > object list (HEAD) of this section. > > But only the section list was fixed in commit 4fb6a564b8ee ("clean up > uci_set") after the realloc finished. Traversing the object list or > accessing the section pointer caused heap-use-after-free errors. > > Reported-by: Charlemagne Lasse <charlemagnelasse@gmail.com> > Fixes: 4fb6a564b8ee ("clean up uci_set") > Signed-off-by: Sven Eckelmann <sven@narfation.org> Patch applied; thx Hans > --- > v2: > - drop unnecessary include of stdbool.h in cli.c > > list.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/list.c b/list.c > index 25aec56..78efbaf 100644 > --- a/list.c > +++ b/list.c > @@ -182,6 +182,32 @@ static void uci_fixup_section(struct uci_context *ctx, struct uci_section *s) > s->e.name = uci_strdup(ctx, buf); > } > > +/* fix up option list HEAD pointers and pointer to section in options */ > +static void uci_section_fixup_options(struct uci_section *s, bool no_options) > +{ > + struct uci_element *e; > + > + if (no_options) { > + /* > + * enforce empty list pointer state (s->next == s) when original > + * section had no options in the first place > + */ > + uci_list_init(&s->options); > + return; > + } > + > + /* fix pointers to HEAD at end/beginning of list */ > + uci_list_fixup(&s->options); > + > + /* fix back pointer to section in options */ > + uci_foreach_element(&s->options, e) { > + struct uci_option *o; > + > + o = uci_to_option(e); > + o->section = s; > + } > +} > + > static struct uci_section * > uci_alloc_section(struct uci_package *p, const char *type, const char *name) > { > @@ -713,10 +739,15 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) > char *s = uci_strdup(ctx, ptr->value); > > if (ptr->s->type == uci_dataptr(ptr->s)) { > + /* drop the in-section storage of type name */ > + bool no_options; > + > + no_options = uci_list_empty(&ptr->s->options); > ptr->last = NULL; > ptr->last = uci_realloc(ctx, ptr->s, sizeof(struct uci_section)); > ptr->s = uci_to_section(ptr->last); > uci_list_fixup(&ptr->s->e.list); > + uci_section_fixup_options(ptr->s, no_options); > } else { > free(ptr->s->type); > } > -- > 2.20.1 > > > _______________________________________________ > openwrt-devel mailing list > openwrt-devel@lists.openwrt.org > https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff --git a/list.c b/list.c index 25aec56..78efbaf 100644 --- a/list.c +++ b/list.c @@ -182,6 +182,32 @@ static void uci_fixup_section(struct uci_context *ctx, struct uci_section *s) s->e.name = uci_strdup(ctx, buf); } +/* fix up option list HEAD pointers and pointer to section in options */ +static void uci_section_fixup_options(struct uci_section *s, bool no_options) +{ + struct uci_element *e; + + if (no_options) { + /* + * enforce empty list pointer state (s->next == s) when original + * section had no options in the first place + */ + uci_list_init(&s->options); + return; + } + + /* fix pointers to HEAD at end/beginning of list */ + uci_list_fixup(&s->options); + + /* fix back pointer to section in options */ + uci_foreach_element(&s->options, e) { + struct uci_option *o; + + o = uci_to_option(e); + o->section = s; + } +} + static struct uci_section * uci_alloc_section(struct uci_package *p, const char *type, const char *name) { @@ -713,10 +739,15 @@ int uci_set(struct uci_context *ctx, struct uci_ptr *ptr) char *s = uci_strdup(ctx, ptr->value); if (ptr->s->type == uci_dataptr(ptr->s)) { + /* drop the in-section storage of type name */ + bool no_options; + + no_options = uci_list_empty(&ptr->s->options); ptr->last = NULL; ptr->last = uci_realloc(ctx, ptr->s, sizeof(struct uci_section)); ptr->s = uci_to_section(ptr->last); uci_list_fixup(&ptr->s->e.list); + uci_section_fixup_options(ptr->s, no_options); } else { free(ptr->s->type); }
A section can store its name in the same memory region as the section (after the actual section object). The object is then reallocated when the type is later changed via an uci_set. But the original address of the section is (indirectly) stored in the section list, the object and the object list (HEAD) of this section. But only the section list was fixed in commit 4fb6a564b8ee ("clean up uci_set") after the realloc finished. Traversing the object list or accessing the section pointer caused heap-use-after-free errors. Reported-by: Charlemagne Lasse <charlemagnelasse@gmail.com> Fixes: 4fb6a564b8ee ("clean up uci_set") Signed-off-by: Sven Eckelmann <sven@narfation.org> --- v2: - drop unnecessary include of stdbool.h in cli.c list.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)