[PATCH v1] perf stat: Fix aggr mode initialization

[PATCH v1] perf stat: Fix aggr mode initialization

[Ian Rogers]

Generating metrics llc_code_read_mpi_demand_plus_prefetch,
llc_data_read_mpi_demand_plus_prefetch,
llc_miss_local_memory_bandwidth_read,
llc_miss_local_memory_bandwidth_write,
nllc_miss_remote_memory_bandwidth_read, memory_bandwidth_read,
memory_bandwidth_write, uncore_frequency, upi_data_transmit_bw,
C2_Pkg_Residency, C3_Core_Residency, C3_Pkg_Residency,
C6_Core_Residency, C6_Pkg_Residency, C7_Core_Residency,
C7_Pkg_Residency, UNCORE_FREQ and tma_info_system_socket_clks would
trigger an address sanitizer heap-buffer-overflows on a SkylakeX.

```
==2567752==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5020003ed098 at pc 0x5621a816654e bp 0x7fffb55d4da0 sp 0x7fffb55d4d98
READ of size 4 at 0x5020003eee78 thread T0
    #0 0x558265d6654d in aggr_cpu_id__is_empty tools/perf/util/cpumap.c:694:12
    #1 0x558265c914da in perf_stat__get_aggr tools/perf/builtin-stat.c:1490:6
    #2 0x558265c914da in perf_stat__get_global_cached tools/perf/builtin-stat.c:1530:9
    #3 0x558265e53290 in should_skip_zero_counter tools/perf/util/stat-display.c:947:31
    #4 0x558265e53290 in print_counter_aggrdata tools/perf/util/stat-display.c:985:18
    #5 0x558265e51931 in print_counter tools/perf/util/stat-display.c:1110:3
    #6 0x558265e51931 in evlist__print_counters tools/perf/util/stat-display.c:1571:5
    #7 0x558265c8ec87 in print_counters tools/perf/builtin-stat.c:981:2
    #8 0x558265c8cc71 in cmd_stat tools/perf/builtin-stat.c:2837:3
    #9 0x558265bb9bd4 in run_builtin tools/perf/perf.c:323:11
    #10 0x558265bb98eb in handle_internal_command tools/perf/perf.c:377:8
    #11 0x558265bb9389 in run_argv tools/perf/perf.c:421:2
    #12 0x558265bb9389 in main tools/perf/perf.c:537:3
```

The issue was the use of testing a cpumap with NULL rather than using
empty, as a map containing the dummy value isn't NULL and the -1
results in an empty aggr map being allocated which legitimately
overflows when any member is accessed.

Fixes: 8a96f454f566 ("perf stat: Avoid SEGV if core.cpus isn't set")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 07b48f6df48e..a3af805a1d57 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1622,7 +1622,7 @@ static int perf_stat_init_aggr_mode(void)
 	 * taking the highest cpu number to be the size of
 	 * the aggregation translate cpumap.
 	 */
-	if (evsel_list->core.user_requested_cpus)
+	if (!perf_cpu_map__empty(evsel_list->core.user_requested_cpus))
 		nr = perf_cpu_map__max(evsel_list->core.user_requested_cpus).cpu;
 	else
 		nr = 0;
-- 
2.42.0.283.g2d96d420d3-goog

Re: [PATCH v1] perf stat: Fix aggr mode initialization

[Namhyung Kim]

Hi Ian,

On Tue, Sep 5, 2023 at 5:39 PM Ian Rogers <irogers@google.com> wrote:
>
> Generating metrics llc_code_read_mpi_demand_plus_prefetch,
> llc_data_read_mpi_demand_plus_prefetch,
> llc_miss_local_memory_bandwidth_read,
> llc_miss_local_memory_bandwidth_write,
> nllc_miss_remote_memory_bandwidth_read, memory_bandwidth_read,
> memory_bandwidth_write, uncore_frequency, upi_data_transmit_bw,
> C2_Pkg_Residency, C3_Core_Residency, C3_Pkg_Residency,
> C6_Core_Residency, C6_Pkg_Residency, C7_Core_Residency,
> C7_Pkg_Residency, UNCORE_FREQ and tma_info_system_socket_clks would
> trigger an address sanitizer heap-buffer-overflows on a SkylakeX.
>
> ```
> ==2567752==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x5020003ed098 at pc 0x5621a816654e bp 0x7fffb55d4da0 sp 0x7fffb55d4d98
> READ of size 4 at 0x5020003eee78 thread T0
>     #0 0x558265d6654d in aggr_cpu_id__is_empty tools/perf/util/cpumap.c:694:12
>     #1 0x558265c914da in perf_stat__get_aggr tools/perf/builtin-stat.c:1490:6
>     #2 0x558265c914da in perf_stat__get_global_cached tools/perf/builtin-stat.c:1530:9
>     #3 0x558265e53290 in should_skip_zero_counter tools/perf/util/stat-display.c:947:31
>     #4 0x558265e53290 in print_counter_aggrdata tools/perf/util/stat-display.c:985:18
>     #5 0x558265e51931 in print_counter tools/perf/util/stat-display.c:1110:3
>     #6 0x558265e51931 in evlist__print_counters tools/perf/util/stat-display.c:1571:5
>     #7 0x558265c8ec87 in print_counters tools/perf/builtin-stat.c:981:2
>     #8 0x558265c8cc71 in cmd_stat tools/perf/builtin-stat.c:2837:3
>     #9 0x558265bb9bd4 in run_builtin tools/perf/perf.c:323:11
>     #10 0x558265bb98eb in handle_internal_command tools/perf/perf.c:377:8
>     #11 0x558265bb9389 in run_argv tools/perf/perf.c:421:2
>     #12 0x558265bb9389 in main tools/perf/perf.c:537:3
> ```
>
> The issue was the use of testing a cpumap with NULL rather than using
> empty, as a map containing the dummy value isn't NULL and the -1
> results in an empty aggr map being allocated which legitimately
> overflows when any member is accessed.

Right, evlist__create_maps() set it to non-NULL.

>
> Fixes: 8a96f454f566 ("perf stat: Avoid SEGV if core.cpus isn't set")
> Signed-off-by: Ian Rogers <irogers@google.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/builtin-stat.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 07b48f6df48e..a3af805a1d57 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -1622,7 +1622,7 @@ static int perf_stat_init_aggr_mode(void)
>          * taking the highest cpu number to be the size of
>          * the aggregation translate cpumap.
>          */
> -       if (evsel_list->core.user_requested_cpus)
> +       if (!perf_cpu_map__empty(evsel_list->core.user_requested_cpus))
>                 nr = perf_cpu_map__max(evsel_list->core.user_requested_cpus).cpu;
>         else
>                 nr = 0;
> --
> 2.42.0.283.g2d96d420d3-goog
>

Re: [PATCH v1] perf stat: Fix aggr mode initialization

[Arnaldo Carvalho de Melo]

Em Tue, Sep 05, 2023 at 07:58:20PM -0700, Namhyung Kim escreveu:
> On Tue, Sep 5, 2023 at 5:39 PM Ian Rogers <irogers@google.com> wrote:
> > The issue was the use of testing a cpumap with NULL rather than using
> > empty, as a map containing the dummy value isn't NULL and the -1
> > results in an empty aggr map being allocated which legitimately
> > overflows when any member is accessed.
> 
> Right, evlist__create_maps() set it to non-NULL.
> 
> >
> > Fixes: 8a96f454f566 ("perf stat: Avoid SEGV if core.cpus isn't set")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks, applied.

- Arnaldo
 
> Thanks,
> Namhyung