[PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[zhe.he]

From: He Zhe <zhe.he@windriver.com>

NULL pointer may be passed to perf_cpu_map__cpu and then cause the
following crash.

perf ftrace -G start_kernel ls
failed to set tracing filters
[  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
               sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
[  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
                     76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
                     4d f4 31 c9 8b 45 08 8b9
Segmentation fault

Program received signal SIGSEGV, Segmentation fault.
0x5677dc98 in perf_cpu_map__cpu (cpus=0x0, idx=0) at cpumap.c:250
250     cpumap.c: No such file or directory.
(gdb) bt
0  0x5677dc98 in perf_cpu_map__cpu (cpus=0x0, idx=0) at cpumap.c:250
1  0x566790bd in evlist__close (evlist=0x56a6f470) at util/evlist.c:1222
2  0x566792aa in evlist__delete (evlist=evlist@entry=0x56a6f470)
   at util/evlist.c:152
3  0x5667936b in evlist__delete (evlist=0x56a6f470) at util/evlist.c:148
4  0x565efd39 in cmd_ftrace (argc=1, argv=0xffffdd18) at builtin-ftrace.c:520
5  0x56660ee7 in run_builtin (p=0x56993004 <commands+324>, argc=4,
   argv=0xffffdd18) at perf.c:312
6  0x565e7fae in handle_internal_command (argv=<optimized out>,
   argc=<optimized out>) at perf.c:364
7  run_argv (argcp=<optimized out>, argv=<optimized out>) at perf.c:408
8  main (argc=<optimized out>, argv=<optimized out>) at perf.c:538

Add null pointer check for iteration and NULL assignment for all_cpus.
And there is no need to iterate if there is no cpus.

Signed-off-by: He Zhe <zhe.he@windriver.com>
---
 tools/lib/perf/cpumap.c | 4 ++--
 tools/lib/perf/evlist.c | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
index f93f4e703e4c..128386647ac0 100644
--- a/tools/lib/perf/cpumap.c
+++ b/tools/lib/perf/cpumap.c
@@ -247,7 +247,7 @@ struct perf_cpu_map *perf_cpu_map__new(const char *cpu_list)
 
 int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx)
 {
-	if (idx < cpus->nr)
+	if (cpus && idx < cpus->nr)
 		return cpus->map[idx];
 
 	return -1;
@@ -255,7 +255,7 @@ int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx)
 
 int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
 {
-	return cpus ? cpus->nr : 1;
+	return cpus ? cpus->nr : 0;
 }
 
 bool perf_cpu_map__empty(const struct perf_cpu_map *map)
diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index ae9e65aa2491..d57adf3020fe 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -127,6 +127,7 @@ void perf_evlist__exit(struct perf_evlist *evlist)
 	perf_cpu_map__put(evlist->cpus);
 	perf_thread_map__put(evlist->threads);
 	evlist->cpus = NULL;
+	evlist->all_cpus = NULL;
 	evlist->threads = NULL;
 	fdarray__exit(&evlist->pollfd);
 }
-- 
2.24.1

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[Arnaldo Carvalho de Melo]

Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
> From: He Zhe <zhe.he@windriver.com>
> 
> NULL pointer may be passed to perf_cpu_map__cpu and then cause the
> following crash.
> 
> perf ftrace -G start_kernel ls
> failed to set tracing filters
> [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
>                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
> [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
>                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
>                      4d f4 31 c9 8b 45 08 8b9
> Segmentation fault

I'm not being able to repro this here, what is the tree you are using?

- Arnaldo
 
> Program received signal SIGSEGV, Segmentation fault.
> 0x5677dc98 in perf_cpu_map__cpu (cpus=0x0, idx=0) at cpumap.c:250
> 250     cpumap.c: No such file or directory.
> (gdb) bt
> 0  0x5677dc98 in perf_cpu_map__cpu (cpus=0x0, idx=0) at cpumap.c:250
> 1  0x566790bd in evlist__close (evlist=0x56a6f470) at util/evlist.c:1222
> 2  0x566792aa in evlist__delete (evlist=evlist@entry=0x56a6f470)
>    at util/evlist.c:152
> 3  0x5667936b in evlist__delete (evlist=0x56a6f470) at util/evlist.c:148
> 4  0x565efd39 in cmd_ftrace (argc=1, argv=0xffffdd18) at builtin-ftrace.c:520
> 5  0x56660ee7 in run_builtin (p=0x56993004 <commands+324>, argc=4,
>    argv=0xffffdd18) at perf.c:312
> 6  0x565e7fae in handle_internal_command (argv=<optimized out>,
>    argc=<optimized out>) at perf.c:364
> 7  run_argv (argcp=<optimized out>, argv=<optimized out>) at perf.c:408
> 8  main (argc=<optimized out>, argv=<optimized out>) at perf.c:538
> 
> Add null pointer check for iteration and NULL assignment for all_cpus.
> And there is no need to iterate if there is no cpus.
> 
> Signed-off-by: He Zhe <zhe.he@windriver.com>
> ---
>  tools/lib/perf/cpumap.c | 4 ++--
>  tools/lib/perf/evlist.c | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/lib/perf/cpumap.c b/tools/lib/perf/cpumap.c
> index f93f4e703e4c..128386647ac0 100644
> --- a/tools/lib/perf/cpumap.c
> +++ b/tools/lib/perf/cpumap.c
> @@ -247,7 +247,7 @@ struct perf_cpu_map *perf_cpu_map__new(const char *cpu_list)
>  
>  int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx)
>  {
> -	if (idx < cpus->nr)
> +	if (cpus && idx < cpus->nr)
>  		return cpus->map[idx];
>  
>  	return -1;
> @@ -255,7 +255,7 @@ int perf_cpu_map__cpu(const struct perf_cpu_map *cpus, int idx)
>  
>  int perf_cpu_map__nr(const struct perf_cpu_map *cpus)
>  {
> -	return cpus ? cpus->nr : 1;
> +	return cpus ? cpus->nr : 0;
>  }
>  
>  bool perf_cpu_map__empty(const struct perf_cpu_map *map)
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index ae9e65aa2491..d57adf3020fe 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -127,6 +127,7 @@ void perf_evlist__exit(struct perf_evlist *evlist)
>  	perf_cpu_map__put(evlist->cpus);
>  	perf_thread_map__put(evlist->threads);
>  	evlist->cpus = NULL;
> +	evlist->all_cpus = NULL;
>  	evlist->threads = NULL;
>  	fdarray__exit(&evlist->pollfd);
>  }
> -- 
> 2.24.1

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[Andi Kleen]

On Thu, Mar 05, 2020 at 12:27:55PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
> > From: He Zhe <zhe.he@windriver.com>
> > 
> > NULL pointer may be passed to perf_cpu_map__cpu and then cause the
> > following crash.
> > 
> > perf ftrace -G start_kernel ls
> > failed to set tracing filters
> > [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
> >                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
> > [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
> >                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
> >                      4d f4 31 c9 8b 45 08 8b9
> > Segmentation fault
> 
> I'm not being able to repro this here, what is the tree you are using?

I believe that's the same bug that Jann Horn reported recently for perf trace.
I thought the patch for that went in.

-Andi

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[Arnaldo Carvalho de Melo]

Em Thu, Mar 05, 2020 at 10:32:06AM -0800, Andi Kleen escreveu:
> On Thu, Mar 05, 2020 at 12:27:55PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
> > > From: He Zhe <zhe.he@windriver.com>
> > > 
> > > NULL pointer may be passed to perf_cpu_map__cpu and then cause the
> > > following crash.
> > > 
> > > perf ftrace -G start_kernel ls
> > > failed to set tracing filters
> > > [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
> > >                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
> > > [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
> > >                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
> > >                      4d f4 31 c9 8b 45 08 8b9
> > > Segmentation fault
> > 
> > I'm not being able to repro this here, what is the tree you are using?
> 
> I believe that's the same bug that Jann Horn reported recently for perf trace.
> I thought the patch for that went in.

Ok, Zhe, that patch is at the end of this message, and it is in:

[acme@five perf]$ git tag --contains cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1 | grep ^v
v5.6-rc1
v5.6-rc2
v5.6-rc3
v5.6-rc4
[acme@five perf]$

Can you try with that?

- Arnaldo

commit cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1
Author: Jiri Olsa <jolsa@kernel.org>
Date:   Fri Jan 10 16:15:37 2020 +0100

    libperf: Setup initial evlist::all_cpus value
    
    Jann Horn reported crash in perf ftrace because evlist::all_cpus isn't
    initialized if there's evlist without events, which is the case for perf
    ftrace.
    
    Adding initial initialization of evlist::all_cpus from given cpus,
    regardless of events in the evlist.
    
    Fixes: 7736627b865d ("perf stat: Use affinity for closing file descriptors")
    Reported-by: Jann Horn <jannh@google.com>
    Signed-off-by: Jiri Olsa <jolsa@kernel.org>
    Acked-by: Andi Kleen <ak@linux.intel.com>
    Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
    Cc: Michael Petlan <mpetlan@redhat.com>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Cc: Peter Zijlstra <peterz@infradead.org>
    Link: http://lore.kernel.org/lkml/20200110151537.153012-1-jolsa@kernel.org
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index ae9e65aa2491..5b9f2ca50591 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -164,6 +164,9 @@ void perf_evlist__set_maps(struct perf_evlist *evlist,
 		evlist->threads = perf_thread_map__get(threads);
 	}
 
+	if (!evlist->all_cpus && cpus)
+		evlist->all_cpus = perf_cpu_map__get(cpus);
+
 	perf_evlist__propagate_maps(evlist);
 }
 

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[He Zhe]

On 3/6/20 3:58 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Mar 05, 2020 at 10:32:06AM -0800, Andi Kleen escreveu:
>> On Thu, Mar 05, 2020 at 12:27:55PM -0300, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
>>>> From: He Zhe <zhe.he@windriver.com>
>>>>
>>>> NULL pointer may be passed to perf_cpu_map__cpu and then cause the
>>>> following crash.
>>>>
>>>> perf ftrace -G start_kernel ls
>>>> failed to set tracing filters
>>>> [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
>>>>                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
>>>> [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
>>>>                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
>>>>                      4d f4 31 c9 8b 45 08 8b9
>>>> Segmentation fault
>>> I'm not being able to repro this here, what is the tree you are using?
>> I believe that's the same bug that Jann Horn reported recently for perf trace.
>> I thought the patch for that went in.
> Ok, Zhe, that patch is at the end of this message, and it is in:
>
> [acme@five perf]$ git tag --contains cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1 | grep ^v
> v5.6-rc1
> v5.6-rc2
> v5.6-rc3
> v5.6-rc4
> [acme@five perf]$
>
> Can you try with that?

Thanks, that does fix the issue I met.

BTW, my change in perf_cpu_map__cpu can be used as a preventive check
and the "1"  in perf_cpu_map__cpu should be "0", and assigning a NULL in
perf_evlist__exit makes the clearing complete. So are they worth a new patch?

Regards,
Zhe

>
> - Arnaldo
>
> commit cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1
> Author: Jiri Olsa <jolsa@kernel.org>
> Date:   Fri Jan 10 16:15:37 2020 +0100
>
>     libperf: Setup initial evlist::all_cpus value
>     
>     Jann Horn reported crash in perf ftrace because evlist::all_cpus isn't
>     initialized if there's evlist without events, which is the case for perf
>     ftrace.
>     
>     Adding initial initialization of evlist::all_cpus from given cpus,
>     regardless of events in the evlist.
>     
>     Fixes: 7736627b865d ("perf stat: Use affinity for closing file descriptors")
>     Reported-by: Jann Horn <jannh@google.com>
>     Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>     Acked-by: Andi Kleen <ak@linux.intel.com>
>     Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>     Cc: Michael Petlan <mpetlan@redhat.com>
>     Cc: Namhyung Kim <namhyung@kernel.org>
>     Cc: Peter Zijlstra <peterz@infradead.org>
>     Link: http://lore.kernel.org/lkml/20200110151537.153012-1-jolsa@kernel.org
>     Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
> index ae9e65aa2491..5b9f2ca50591 100644
> --- a/tools/lib/perf/evlist.c
> +++ b/tools/lib/perf/evlist.c
> @@ -164,6 +164,9 @@ void perf_evlist__set_maps(struct perf_evlist *evlist,
>  		evlist->threads = perf_thread_map__get(threads);
>  	}
>  
> +	if (!evlist->all_cpus && cpus)
> +		evlist->all_cpus = perf_cpu_map__get(cpus);
> +
>  	perf_evlist__propagate_maps(evlist);
>  }
>  
>

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[Jiri Olsa]

On Fri, Mar 06, 2020 at 03:20:55PM +0800, He Zhe wrote:
> 
> 
> On 3/6/20 3:58 AM, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Mar 05, 2020 at 10:32:06AM -0800, Andi Kleen escreveu:
> >> On Thu, Mar 05, 2020 at 12:27:55PM -0300, Arnaldo Carvalho de Melo wrote:
> >>> Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
> >>>> From: He Zhe <zhe.he@windriver.com>
> >>>>
> >>>> NULL pointer may be passed to perf_cpu_map__cpu and then cause the
> >>>> following crash.
> >>>>
> >>>> perf ftrace -G start_kernel ls
> >>>> failed to set tracing filters
> >>>> [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
> >>>>                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
> >>>> [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
> >>>>                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
> >>>>                      4d f4 31 c9 8b 45 08 8b9
> >>>> Segmentation fault
> >>> I'm not being able to repro this here, what is the tree you are using?
> >> I believe that's the same bug that Jann Horn reported recently for perf trace.
> >> I thought the patch for that went in.
> > Ok, Zhe, that patch is at the end of this message, and it is in:
> >
> > [acme@five perf]$ git tag --contains cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1 | grep ^v
> > v5.6-rc1
> > v5.6-rc2
> > v5.6-rc3
> > v5.6-rc4
> > [acme@five perf]$
> >
> > Can you try with that?
> 
> Thanks, that does fix the issue I met.
> 
> BTW, my change in perf_cpu_map__cpu can be used as a preventive check
> and the "1"  in perf_cpu_map__cpu should be "0", and assigning a NULL in

I agree, can't see why we had 1 in here.. must be connected to the dummy
map.. could you please double check with all the perf_cpu_map__nr usages
that the 0 will work as expected?

> perf_evlist__exit makes the clearing complete. So are they worth a new patch?

the rest of the hunks looks good as preventive checks

thanks,
jirka

Re: [PATCH] perf: Fix crash due to null pointer dereference when iterating cpu map

[He Zhe]

On 3/6/20 4:30 PM, Jiri Olsa wrote:
> On Fri, Mar 06, 2020 at 03:20:55PM +0800, He Zhe wrote:
>>
>> On 3/6/20 3:58 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Mar 05, 2020 at 10:32:06AM -0800, Andi Kleen escreveu:
>>>> On Thu, Mar 05, 2020 at 12:27:55PM -0300, Arnaldo Carvalho de Melo wrote:
>>>>> Em Thu, Mar 05, 2020 at 06:47:19PM +0800, zhe.he@windriver.com escreveu:
>>>>>> From: He Zhe <zhe.he@windriver.com>
>>>>>>
>>>>>> NULL pointer may be passed to perf_cpu_map__cpu and then cause the
>>>>>> following crash.
>>>>>>
>>>>>> perf ftrace -G start_kernel ls
>>>>>> failed to set tracing filters
>>>>>> [  208.710716] perf[341]: segfault at 4 ip 00000000567c7c98
>>>>>>                sp 00000000ff937ae0 error 4 in perf[56630000+1b2000]
>>>>>> [  208.724778] Code: fc ff ff e8 aa 9b 01 00 8d b4 26 00 00 00 00 8d
>>>>>>                      76 00 55 89 e5 83 ec 18 65 8b 0d 14 00 00 00 89
>>>>>>                      4d f4 31 c9 8b 45 08 8b9
>>>>>> Segmentation fault
>>>>> I'm not being able to repro this here, what is the tree you are using?
>>>> I believe that's the same bug that Jann Horn reported recently for perf trace.
>>>> I thought the patch for that went in.
>>> Ok, Zhe, that patch is at the end of this message, and it is in:
>>>
>>> [acme@five perf]$ git tag --contains cb71f7d43ece3d5a4f400f510c61b2ec7c9ce9a1 | grep ^v
>>> v5.6-rc1
>>> v5.6-rc2
>>> v5.6-rc3
>>> v5.6-rc4
>>> [acme@five perf]$
>>>
>>> Can you try with that?
>> Thanks, that does fix the issue I met.
>>
>> BTW, my change in perf_cpu_map__cpu can be used as a preventive check
>> and the "1"  in perf_cpu_map__cpu should be "0", and assigning a NULL in
> I agree, can't see why we had 1 in here.. must be connected to the dummy
> map.. could you please double check with all the perf_cpu_map__nr usages
> that the 0 will work as expected?

I just checked the callers of perf_cpu_map__nr. They really depend on it
returning 1 as the only one cpu at least. And the same trick is played in
perf_thread_map__nr. So perf_cpu_map__nr should remain unchanged.

I'll send v2 for the rest of the hunks.

Thanks,
Zhe

>
>> perf_evlist__exit makes the clearing complete. So are they worth a new patch?
> the rest of the hunks looks good as preventive checks
>
> thanks,
> jirka
>