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 <ak@xxxxxxxxxxxxxxx>
> ---
>  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;
 	}
 }