linux-kernel - [PATCH] perf report: Fix memory corruption in --branch-history mode --branch-history

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: <20180216123619.GA9945@krava>
Date:   Fri, 16 Feb 2018 13:36:19 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     "Jin, Yao" <yao.jin@...ux.intel.com>, acme@...nel.org
Cc:     jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
        alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
        ak@...ux.intel.com, kan.liang@...el.com, yao.jin@...el.com
Subject: [PATCH] perf report: Fix memory corruption in --branch-history mode
 --branch-history

On Fri, Feb 16, 2018 at 08:53:04AM +0100, Jiri Olsa wrote:
> On Fri, Feb 16, 2018 at 10:25:31AM +0800, Jin, Yao wrote:
> 
> SNIP
> 
> > >  From my opinion, the option '--max-stack' in perf report looks not very
> > > necessary. While it's just my personal opinion, need to hear from more
> > > people. :)
> > > 
> > > Thanks
> > > Jin Yao
> > > 
> > > > thanks,
> > > > jirka
> > > > 
> > > > 
> > > > ---
> > > > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > > > index b6140950301e..b50b7b70dcca 100644
> > > > --- a/tools/perf/util/hist.c
> > > > +++ b/tools/perf/util/hist.c
> > > > @@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct
> > > > hist_entry_iter *iter,
> > > >        * cumulated only one time to prevent entries more than 100%
> > > >        * overhead.
> > > >        */
> > > > -    he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
> > > > +    he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
> > > >       if (he_cache == NULL)
> > > >           return -ENOMEM;
> > > > 
> > 
> > Hi Jiri,
> > 
> > I guess you will post this patch, right?
> 
> yep, later today

here it is.. I think we want this change now to fix the crash, and
some more fixes later to ensure that the branch stack code follows
properly the logic of --max-stack, which is not the case now

thanks,
jirka


---
Jin Yao reported memory corrupton in perf report with
branch info used for stack trace:

> Following command lines will cause perf crash.

> perf record -j call -g -a <application>
> perf report --branch-history
>
> *** Error in `perf': double free or corruption (!prev): 0x00000000104aa040 ***
> ======= Backtrace: =========
> /lib/x86_64-linux-gnu/libc.so.6(+0x77725)[0x7f6b37254725]
> /lib/x86_64-linux-gnu/libc.so.6(+0x7ff4a)[0x7f6b3725cf4a]
> /lib/x86_64-linux-gnu/libc.so.6(cfree+0x4c)[0x7f6b37260abc]
> perf[0x51b914]
> perf(hist_entry_iter__add+0x1e5)[0x51f305]
> perf[0x43cf01]
> perf[0x4fa3bf]
> perf[0x4fa923]
> perf[0x4fd396]
> perf[0x4f9614]
> perf(perf_session__process_events+0x89e)[0x4fc38e]
> perf(cmd_report+0x15d2)[0x43f202]
> perf[0x4a059f]
> perf(main+0x631)[0x427b71]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f6b371fd830]
> perf(_start+0x29)[0x427d89]

For the cumulative output, we allocate he_cache array based
on the --max-stack option value and populate it with data
from callchain_cursor.

The --max-stack option value does not ensure now the limit
for number of callchain_cursor nodes, so the cumulative
iter code will allocate smaller array than it's actually
needed and cause above corruption.

I think the --max-stack limit does not apply here anyway,
because we add callchain data as normal hist entries,
while the --max-stack control the limit of single entry
callchain depth.

Using the callchain_cursor.nr as he_cache array count
to fix this. Also removing struct hist_entry_iter::max_stack,
because there's no longer any use for it.

We need more fixes to ensure that the branch stack code
follows properly the logic of --max-stack, which is not
the case at the moment.

Reported-by: Jin Yao <yao.jin@...ux.intel.com>
Original-patch-by: Jin Yao <yao.jin@...ux.intel.com>
Link: http://lkml.kernel.org/n/tip-qj1kdpvyu25ac6w22lhmy7m2@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@...nel.org>
---
 tools/perf/util/hist.c | 4 +---
 tools/perf/util/hist.h | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index b6140950301e..44a8456cea10 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -879,7 +879,7 @@ iter_prepare_cumulative_entry(struct hist_entry_iter *iter,
 	 * cumulated only one time to prevent entries more than 100%
 	 * overhead.
 	 */
-	he_cache = malloc(sizeof(*he_cache) * (iter->max_stack + 1));
+	he_cache = malloc(sizeof(*he_cache) * (callchain_cursor.nr + 1));
 	if (he_cache == NULL)
 		return -ENOMEM;
 
@@ -1045,8 +1045,6 @@ int hist_entry_iter__add(struct hist_entry_iter *iter, struct addr_location *al,
 	if (err)
 		return err;
 
-	iter->max_stack = max_stack_depth;
-
 	err = iter->ops->prepare_entry(iter, al);
 	if (err)
 		goto out;
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 02721b579746..e869cad4d89f 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -107,7 +107,6 @@ struct hist_entry_iter {
 	int curr;
 
 	bool hide_unresolved;
-	int max_stack;
 
 	struct perf_evsel *evsel;
 	struct perf_sample *sample;
-- 
2.13.6