[PATCH] fix use-after-free in perf_sched__lat

[PATCH] fix use-after-free in perf_sched__lat

[Wei Li]

After thread is added to machine->threads[i].dead in
__machine__remove_thread, the machine->threads[i].dead is freed
when calling free(session) in perf_session__delete(). So it get a
Segmentation fault when accessing it in thread__put().

In this patch, we delay the perf_session__delete until all threads
have been deleted.

This can be reproduced by following steps:
	ulimit -c unlimited
	export MALLOC_MMAP_THRESHOLD_=0
	perf sched record sleep 10
	perf sched latency --sort max
	Segmentation fault (core dumped)

Signed-off-by: Zhipeng Xie <xiezhipeng1@huawei.com>
Signed-off-by: Wei Li <liwei391@huawei.com>
---
 tools/perf/builtin-sched.c | 44 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index cbf39dab19c1..17849ae2eb1e 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -3130,11 +3130,48 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
 static int perf_sched__lat(struct perf_sched *sched)
 {
 	struct rb_node *next;
+	const struct perf_evsel_str_handler handlers[] = {
+		{ "sched:sched_switch",	      process_sched_switch_event, },
+		{ "sched:sched_stat_runtime", process_sched_runtime_event, },
+		{ "sched:sched_wakeup",	      process_sched_wakeup_event, },
+		{ "sched:sched_wakeup_new",   process_sched_wakeup_event, },
+		{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
+	};
+	struct perf_session *session;
+	struct perf_data data = {
+		.file      = {
+			.path = input_name,
+		},
+		.mode      = PERF_DATA_MODE_READ,
+		.force     = sched->force,
+	};
+	int rc = -1;
 
 	setup_pager();
 
-	if (perf_sched__read_events(sched))
+	session = perf_session__new(&data, false, &sched->tool);
+	if (session == NULL) {
+		pr_debug("No Memory for session\n");
 		return -1;
+	}
+
+	symbol__init(&session->header.env);
+
+	if (perf_session__set_tracepoints_handlers(session, handlers))
+		goto out_delete;
+
+	if (perf_session__has_traces(session, "record -R")) {
+		int err = perf_session__process_events(session);
+
+		if (err) {
+			pr_err("Failed to process events, error %d", err);
+			goto out_delete;
+		}
+
+		sched->nr_events      = session->evlist->stats.nr_events[0];
+		sched->nr_lost_events = session->evlist->stats.total_lost;
+		sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
+	}
 
 	perf_sched__merge_lat(sched);
 	perf_sched__sort_lat(sched);
@@ -3163,7 +3200,10 @@ static int perf_sched__lat(struct perf_sched *sched)
 	print_bad_events(sched);
 	printf("\n");
 
-	return 0;
+	rc = 0;
+out_delete:
+	perf_session__delete(session);
+	return rc;
 }
 
 static int setup_map_cpus(struct perf_sched *sched)
-- 
2.17.1

Re: [PATCH] fix use-after-free in perf_sched__lat

[Jiri Olsa]

On Fri, May 03, 2019 at 10:35:55AM +0800, Wei Li wrote:
> After thread is added to machine->threads[i].dead in
> __machine__remove_thread, the machine->threads[i].dead is freed
> when calling free(session) in perf_session__delete(). So it get a
> Segmentation fault when accessing it in thread__put().
> 
> In this patch, we delay the perf_session__delete until all threads
> have been deleted.
> 
> This can be reproduced by following steps:
> 	ulimit -c unlimited
> 	export MALLOC_MMAP_THRESHOLD_=0

what's this for?

> 	perf sched record sleep 10
> 	perf sched latency --sort max
> 	Segmentation fault (core dumped)
> 
> Signed-off-by: Zhipeng Xie <xiezhipeng1@huawei.com>
> Signed-off-by: Wei Li <liwei391@huawei.com>
> ---
>  tools/perf/builtin-sched.c | 44 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 42 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
> index cbf39dab19c1..17849ae2eb1e 100644
> --- a/tools/perf/builtin-sched.c
> +++ b/tools/perf/builtin-sched.c
> @@ -3130,11 +3130,48 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
>  static int perf_sched__lat(struct perf_sched *sched)
>  {
>  	struct rb_node *next;
> +	const struct perf_evsel_str_handler handlers[] = {
> +		{ "sched:sched_switch",	      process_sched_switch_event, },
> +		{ "sched:sched_stat_runtime", process_sched_runtime_event, },
> +		{ "sched:sched_wakeup",	      process_sched_wakeup_event, },
> +		{ "sched:sched_wakeup_new",   process_sched_wakeup_event, },
> +		{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
> +	};
> +	struct perf_session *session;
> +	struct perf_data data = {
> +		.file      = {
> +			.path = input_name,
> +		},

I can't compile this:

builtin-sched.c: In function ‘perf_sched__lat’:
builtin-sched.c:3144:12: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
    .path = input_name,


> +		.mode      = PERF_DATA_MODE_READ,
> +		.force     = sched->force,
> +	};
> +	int rc = -1;
>  
>  	setup_pager();
>  
> -	if (perf_sched__read_events(sched))

so it's basically perf_sched__read_events code in here now, right?

might be better to add __perf_sched__read_events function
that would take session argument, something like:

        session = perf_session__new(&data, false, &sched->tool);
	...
	__perf_sched__read_events(sched, session)
	...
	perf_session__delete(session);

to avoid the code ducplication

thanks,
jirka

> +	session = perf_session__new(&data, false, &sched->tool);
> +	if (session == NULL) {
> +		pr_debug("No Memory for session\n");
>  		return -1;
> +	}
> +
> +	symbol__init(&session->header.env);
> +
> +	if (perf_session__set_tracepoints_handlers(session, handlers))
> +		goto out_delete;
> +
> +	if (perf_session__has_traces(session, "record -R")) {
> +		int err = perf_session__process_events(session);
> +
> +		if (err) {
> +			pr_err("Failed to process events, error %d", err);
> +			goto out_delete;
> +		}
> +
> +		sched->nr_events      = session->evlist->stats.nr_events[0];
> +		sched->nr_lost_events = session->evlist->stats.total_lost;
> +		sched->nr_lost_chunks = session->evlist->stats.nr_events[PERF_RECORD_LOST];
> +	}
>  
>  	perf_sched__merge_lat(sched);
>  	perf_sched__sort_lat(sched);
> @@ -3163,7 +3200,10 @@ static int perf_sched__lat(struct perf_sched *sched)
>  	print_bad_events(sched);
>  	printf("\n");
>  
> -	return 0;
> +	rc = 0;
> +out_delete:
> +	perf_session__delete(session);
> +	return rc;
>  }
>  
>  static int setup_map_cpus(struct perf_sched *sched)
> -- 
> 2.17.1
> 

Re: [PATCH] fix use-after-free in perf_sched__lat

[liwei (GF)]

Hi Jiri，
Thanks for your reply.

On 2019/5/7 16:51, Jiri Olsa wrote:
> On Fri, May 03, 2019 at 10:35:55AM +0800, Wei Li wrote:
>> After thread is added to machine->threads[i].dead in
>> __machine__remove_thread, the machine->threads[i].dead is freed
>> when calling free(session) in perf_session__delete(). So it get a
>> Segmentation fault when accessing it in thread__put().
>>
>> In this patch, we delay the perf_session__delete until all threads
>> have been deleted.
>>
>> This can be reproduced by following steps:
>> 	ulimit -c unlimited
>> 	export MALLOC_MMAP_THRESHOLD_=0
> 
> what's this for?

When we set env "MALLOC_MMAP_THRESHOLD_=0"，the memory-allocation functions employ
mmap instead of increasing the program break using sbrk what may be maintained
with cache. Thus it can report "Segmentation fault" immediately when going into
this use-after-free code.

> 
>> 	perf sched record sleep 10
>> 	perf sched latency --sort max
>> 	Segmentation fault (core dumped)
>>
>> Signed-off-by: Zhipeng Xie <xiezhipeng1@huawei.com>
>> Signed-off-by: Wei Li <liwei391@huawei.com>
>> ---
>>  tools/perf/builtin-sched.c | 44 ++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 42 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
>> index cbf39dab19c1..17849ae2eb1e 100644
>> --- a/tools/perf/builtin-sched.c
>> +++ b/tools/perf/builtin-sched.c
>> @@ -3130,11 +3130,48 @@ static void perf_sched__merge_lat(struct perf_sched *sched)
>>  static int perf_sched__lat(struct perf_sched *sched)
>>  {
>>  	struct rb_node *next;
>> +	const struct perf_evsel_str_handler handlers[] = {
>> +		{ "sched:sched_switch",	      process_sched_switch_event, },
>> +		{ "sched:sched_stat_runtime", process_sched_runtime_event, },
>> +		{ "sched:sched_wakeup",	      process_sched_wakeup_event, },
>> +		{ "sched:sched_wakeup_new",   process_sched_wakeup_event, },
>> +		{ "sched:sched_migrate_task", process_sched_migrate_task_event, },
>> +	};
>> +	struct perf_session *session;
>> +	struct perf_data data = {
>> +		.file      = {
>> +			.path = input_name,
>> +		},
> 
> I can't compile this:
> 
> builtin-sched.c: In function ‘perf_sched__lat’:
> builtin-sched.c:3144:12: error: initialization discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
>     .path = input_name,
> 
Sorry, this place has been changed recently in mainline code, i will update to
the latest code.

> 
>> +		.mode      = PERF_DATA_MODE_READ,
>> +		.force     = sched->force,
>> +	};
>> +	int rc = -1;
>>  
>>  	setup_pager();
>>  
>> -	if (perf_sched__read_events(sched))
> 
> so it's basically perf_sched__read_events code in here now, right?
> 
> might be better to add __perf_sched__read_events function
> that would take session argument, something like:
> 
>         session = perf_session__new(&data, false, &sched->tool);
> 	...
> 	__perf_sched__read_events(sched, session)
> 	...
> 	perf_session__delete(session);
> 
> to avoid the code ducplication

Yes, what you suggest is reasonable, i will send a v2 with your suggestion soon.

Thanks，
Wei