Re: Stale expression reference causing use-after-free

From: Catalin Marinas <catalin.marinas@arm.com>
To: Arnaud Lacombe <lacombar@gmail.com>
Cc: "mmarek@suse.cz" <mmarek@suse.cz>,
	"linux-kbuild@vger.kernel.org" <linux-kbuild@vger.kernel.org>
Subject: Re: Stale expression reference causing use-after-free
Date: Tue, 21 Sep 2010 18:03:39 +0100	[thread overview]
Message-ID: <1285088619.12653.22.camel@e102109-lin.cambridge.arm.com> (raw)
In-Reply-To: <AANLkTi=LceTkH+PsWg4ORMPO33c=sdY9nA3NA102DK9L@mail.gmail.com>

On Tue, 2010-09-21 at 12:57 -0400, Arnaud Lacombe wrote:
> On Tue, Sep 21, 2010 at 12:55 PM, Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > Arnaud Lacombe <lacombar@gmail.com> wrote:
> >> FYI, just this line makes the crash disappear, still I understand the
> >> expr_free(). However, wouldn't there be a better place to initialize
> >> `dir_dep' and avoid the need to use expr_free() ?
> >
> > dir_dep would need to be initialised somewhere, unless there is a memset
> > on the whole structure (I'm not familiar enough with kbuild). But I
> > think we still need the expr_free() in case dir_dep gets overwritten
> > (e.g. multiple 'depends on' lines where the full expression is re-built
> > on the previous line via expr_alloc_and()).
> 
> this is a huge shoot in the dark, but does not seem to cause any regression:
> 
> diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
> index 23acbdb..9eee093 100644
> --- a/scripts/kconfig/menu.c
> +++ b/scripts/kconfig/menu.c
> @@ -107,7 +107,6 @@ static struct expr *menu_check_dep(struct expr *e)
>  void menu_add_dep(struct expr *dep)
>  {
>         current_entry->dep = expr_alloc_and(current_entry->dep, menu_check_dep(dep));
> -       current_entry->dir_dep = current_entry->dep;
>  }
> 
>  void menu_set_type(int type)
> @@ -268,6 +267,7 @@ void menu_finalize(struct menu *parent)
>                         basedep = expr_alloc_and(expr_copy(parentdep), basedep);
>                         basedep = expr_eliminate_dups(basedep);
>                         menu->dep = basedep;
> +                       menu->dir_dep = expr_copy(basedep);
>                         if (menu->sym)
>                                 prop = menu->sym->prop;
>                         else

I'm not sure this would have the same effect as what I intended. The
dir_dep should only store the "depends on" clauses but the basedep at
this point may include some "select" clauses as well.

Thinking about it, my original patch may not be fully correct. Maybe
something like below:

diff --git a/scripts/kconfig/menu.c b/scripts/kconfig/menu.c
index 7298806..e43d8d0 100644
--- a/scripts/kconfig/menu.c
+++ b/scripts/kconfig/menu.c
@@ -107,7 +107,8 @@ static struct expr *menu_check_dep(struct expr *e)
 void menu_add_dep(struct expr *dep)
 {
 	current_entry->dep = expr_alloc_and(current_entry->dep, menu_check_dep(dep));
-	current_entry->dir_dep = current_entry->dep;
+	current_entry->dir_dep = expr_alloc_and(current_entry->dir_dep,
+						menu_check_dep(dep));
 }
 
 void menu_set_type(int type)


-- 
Catalin

