Re: [PATCH v1 01/15] perf, tools, stat: Fix buffer overflow while freeing events

[Jiri Olsa]

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 <a...@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;
        }
 }