linux-kernel - Re: [PATCH v2 perf/core] perf script: fix a use after free crash.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170104083739.GA3009@templeofstupid.com>
Date:   Wed, 4 Jan 2017 00:37:39 -0800
From:   Krister Johansen <kjlx@...pleofstupid.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Krister Johansen <kjlx@...pleofstupid.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Frédéric Weisbecker <fweisbec@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 perf/core] perf script: fix a use after free crash.

On Mon, Jan 02, 2017 at 09:30:33PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 02, 2017 at 04:39:04PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Mon, Jan 02, 2017 at 02:36:57PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Mon, Jan 02, 2017 at 02:35:30PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Em Mon, Jan 02, 2017 at 12:15:14PM -0300, Arnaldo Carvalho de Melo escreveu:
> > >  {
> > >         zfree(&iter->priv);
> > >         iter->he = NULL;
> > > +       map__zput(al->map);
> > 
> > What this pairs to? I was expecting that since this is called via:
> > 
> >    hist_entry_iter__add()
> >    {
> >            <SNIP>
> >            err2 = iter->ops->finish_entry(iter, al);
> >    }
> > 
> > Then it would have to match something done earlier in
> > hist_entry_iter__add(), most likely by some iter->ops->() method, but I
> > couldn'd find anything to that extent, can you clarify?
> 
> With the following patch it has been running all day, care to explain
> why it is needed? I need to run this on valgrind or with Masami's
> refcount debugger to get more clues :-\
> 
> - Arnaldo
> 
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 72f5c82798e9..c27bda16e9cd 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -980,7 +980,6 @@ iter_finish_cumulative_entry(struct hist_entry_iter *iter,
>  {
>  	zfree(&iter->priv);
>  	iter->he = NULL;
> -	map__zput(al->map);
>  
>  	return 0;
>  }

Thanks for taking the time to debug it this far.  I certainly appreciate
it.

The map_zput() in iter_finish_cumulative_entry() was intended to offset
the map_get() in iter_next_cumulative_entry() -> fill_callchain_info().
The call to fill_callchain_info should perform a map__get() on the
addr_location that gets passed in.  However, it looks like I mistakenly
assumed that fill_callcahin_info would always get called from
iter_next_cumulative_entry when clearly that doesn't happen if
callchain_cursor_current returns a NULL node.

Looking back, it's not entirely clear to me that the map__get() in
fill_callchain_info is necessary either, since its only caller is the
hist_iter_cumulative used by hist_entry_iter__add.

Let me try a version of this patch that dispenses with the code in both
fill_callchain_info() and iter_finish_cumulative_entry.  However, before
I do that I'll make sure I can figure out how to reproduce the core that
you were seeing.  I don't want to send you yet another patch that
doesn't run.  The busy system may be the clue I needed.  I had been
running these on a mostly idle system.

Thanks again, and apologies for all of the inconvenience.

-K