Messages in this thread Patch in this message | | | Date | Tue, 1 Aug 2017 10:11:21 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH v1 01/15] perf, tools, stat: Fix buffer overflow while freeing events |
| |
On Mon, Jul 24, 2017 at 04:40:01PM -0700, Andi Kleen wrote:
SNIP
> > The event is allocated with cpus == 1, but freed with cpus == real number > When the evsel close function walks the file descriptors it exceeds the > fd xyarray boundaries and reads random memory. > > Just make sure to always use the same dummy cpu map following > the same logic as the open call. > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > tools/perf/builtin-stat.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 48ac53b199fc..97d6b6c42014 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -715,6 +715,8 @@ static int __run_perf_stat(int argc, const char **argv) > * group leaders. > */ > read_counters(); > + if (!target__has_cpu(&target)) > + evsel_list->cpus = cpu_map__dummy_new();
you're leaking evsel_list->cpus right here..
I can see there's the issue when we mix system_wide event (with cpumask defined) and normal event:
- we open such group as per_thread events (not system_wide), forcing both evsel->fd xyarray to be allocated from dummy cpus (with ncpus == 1)
- but when we call perf_evlist__close we take ncpus from int n = evsel->cpus ? evsel->cpus->nr : ncpus;
which is wrong for system_wide event and will cause the xyarray out of bounds access
I can see the solution either in: 1) keeping the bounds for xyarray in it and use it for iterations 2) or forcing system_wide target if there's single system_wide event specified (patch below)
but not sure there's any sense in meassuting system_wide event in non system_wide mode (ad 1).. thoughts?
thanks, jirka
--- diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 866da7aa54bf..9ccb4d671568 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -2540,8 +2540,8 @@ static void setup_system_wide(int forks) * conditions is met: * * - there's no workload specified - * - there is workload specified but all requested - * events are system wide events + * - there is workload specified and one + * of the events is system wide */ if (!target__none(&target)) return; @@ -2552,12 +2552,11 @@ static void setup_system_wide(int forks) struct perf_evsel *counter; evlist__for_each_entry(evsel_list, counter) { - if (!counter->system_wide) + if (counter->system_wide) { + target.system_wide = true; return; + } } - - if (evsel_list->nr_entries) - target.system_wide = true; } }
| |