Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 5.0 39/93] perf top: Delete the evlist before
 perf_session, fixing heap-use-after-free issue
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-kernel@vger.kernel.org>
CC:     <stable@vger.kernel.org>, Changbin Du <changbin.du@gmail.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Namhyung Kim <namhyung@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        "Steven Rostedt (VMware)" <rostedt@goodmis.org>,
        Arnaldo Carvalho de Melo <acme@redhat.com>,
        Sasha Levin <sashal@kernel.org>
References: <20190418160436.781762249@linuxfoundation.org>
 <20190418160441.339477070@linuxfoundation.org>
From:   Thomas Backlund <tmb@mageia.org>
Message-ID: <b33d7fd2-c18d-9513-1a99-fea31d5f2df3@mageia.org>
Date:   Thu, 18 Apr 2019 22:33:16 +0300
MIME-Version: 1.0
In-Reply-To: <20190418160441.339477070@linuxfoundation.org>
Content-Type: multipart/mixed;
        boundary="------------F151799C9EBAFCA68FED3310"
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

--------------F151799C9EBAFCA68FED3310
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit

Den 18-04-2019 kl. 20:57, skrev Greg Kroah-Hartman:
> [ Upstream commit 0dba9e4be95b59e77060645ca8e37ca3231061f5 ]
> 
> The evlist should be destroyed before the perf session.
> 
> Detected with gcc's ASan:
> 
>    =================================================================
>    ==27350==ERROR: AddressSanitizer: heap-use-after-free on address 0x62b000002e38 at pc 0x5611da276999 bp 0x7ffce8f1d1a0 sp 0x7ffce8f1d190
>    WRITE of size 8 at 0x62b000002e38 thread T0
>        #0 0x5611da276998 in __list_del /home/work/linux/tools/include/linux/list.h:89
>        #1 0x5611da276d4a in __list_del_entry /home/work/linux/tools/include/linux/list.h:102
>        #2 0x5611da276e77 in list_del_init /home/work/linux/tools/include/linux/list.h:145
>        #3 0x5611da2781cd in thread__put util/thread.c:130
>        #4 0x5611da2cc0a8 in __thread__zput util/thread.h:68
>        #5 0x5611da2d2dcb in hist_entry__delete util/hist.c:1148
>        #6 0x5611da2cdf91 in hists__delete_entry util/hist.c:337
>        #7 0x5611da2ce19e in hists__delete_entries util/hist.c:365
>        #8 0x5611da2db2ab in hists__delete_all_entries util/hist.c:2639
>        #9 0x5611da2db325 in hists_evsel__exit util/hist.c:2651
>        #10 0x5611da1c5352 in perf_evsel__exit util/evsel.c:1304
>        #11 0x5611da1c5390 in perf_evsel__delete util/evsel.c:1309
>        #12 0x5611da1b35f0 in perf_evlist__purge util/evlist.c:124
>        #13 0x5611da1b38e2 in perf_evlist__delete util/evlist.c:148
>        #14 0x5611da069781 in cmd_top /home/changbin/work/linux/tools/perf/builtin-top.c:1645
>        #15 0x5611da17d038 in run_builtin /home/changbin/work/linux/tools/perf/perf.c:302
>        #16 0x5611da17d577 in handle_internal_command /home/changbin/work/linux/tools/perf/perf.c:354
>        #17 0x5611da17d97b in run_argv /home/changbin/work/linux/tools/perf/perf.c:398
>        #18 0x5611da17e0e9 in main /home/changbin/work/linux/tools/perf/perf.c:520
>        #19 0x7fdcc970f09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
>        #20 0x5611d9ff35c9 in _start (/home/work/linux/tools/perf/perf+0x3e95c9)
> 
>    0x62b000002e38 is located 11320 bytes inside of 27448-byte region [0x62b000000200,0x62b000006d38)
>    freed by thread T0 here:
>        #0 0x7fdccb04ab70 in free (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xedb70)
>        #1 0x5611da260df4 in perf_session__delete util/session.c:201
>        #2 0x5611da063de5 in __cmd_top /home/changbin/work/linux/tools/perf/builtin-top.c:1300
>        #3 0x5611da06973c in cmd_top /home/changbin/work/linux/tools/perf/builtin-top.c:1642
>        #4 0x5611da17d038 in run_builtin /home/changbin/work/linux/tools/perf/perf.c:302
>        #5 0x5611da17d577 in handle_internal_command /home/changbin/work/linux/tools/perf/perf.c:354
>        #6 0x5611da17d97b in run_argv /home/changbin/work/linux/tools/perf/perf.c:398
>        #7 0x5611da17e0e9 in main /home/changbin/work/linux/tools/perf/perf.c:520
>        #8 0x7fdcc970f09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
> 
>    previously allocated by thread T0 here:
>        #0 0x7fdccb04b138 in calloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xee138)
>        #1 0x5611da26010c in zalloc util/util.h:23
>        #2 0x5611da260824 in perf_session__new util/session.c:118
>        #3 0x5611da0633a6 in __cmd_top /home/changbin/work/linux/tools/perf/builtin-top.c:1192
>        #4 0x5611da06973c in cmd_top /home/changbin/work/linux/tools/perf/builtin-top.c:1642
>        #5 0x5611da17d038 in run_builtin /home/changbin/work/linux/tools/perf/perf.c:302
>        #6 0x5611da17d577 in handle_internal_command /home/changbin/work/linux/tools/perf/perf.c:354
>        #7 0x5611da17d97b in run_argv /home/changbin/work/linux/tools/perf/perf.c:398
>        #8 0x5611da17e0e9 in main /home/changbin/work/linux/tools/perf/perf.c:520
>        #9 0x7fdcc970f09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
> 
>    SUMMARY: AddressSanitizer: heap-use-after-free /home/work/linux/tools/include/linux/list.h:89 in __list_del
>    Shadow bytes around the buggy address:
>      0x0c567fff8570: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff8580: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff8590: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff85a0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff85b0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>    =>0x0c567fff85c0: fd fd fd fd fd fd fd[fd]fd fd fd fd fd fd fd fd
>      0x0c567fff85d0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff85e0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff85f0: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff8600: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>      0x0c567fff8610: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>    Shadow byte legend (one shadow byte represents 8 application bytes):
>      Addressable:           00
>      Partially addressable: 01 02 03 04 05 06 07
>      Heap left redzone:       fa
>      Freed heap region:       fd
>      Stack left redzone:      f1
>      Stack mid redzone:       f2
>      Stack right redzone:     f3
>      Stack after return:      f5
>      Stack use after scope:   f8
>      Global redzone:          f9
>      Global init order:       f6
>      Poisoned by user:        f7
>      Container overflow:      fc
>      Array cookie:            ac
>      Intra object redzone:    bb
>      ASan internal:           fe
>      Left alloca redzone:     ca
>      Right alloca redzone:    cb
>    ==27350==ABORTING
> 
> Signed-off-by: Changbin Du <changbin.du@gmail.com>
> Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Link: http://lkml.kernel.org/r/20190316080556.3075-8-changbin.du@gmail.com
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>   tools/perf/builtin-top.c | 42 ++++++++++++++++++----------------------
>   1 file changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index f64e312db787..9b215007924b 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1192,23 +1192,19 @@ static int __cmd_top(struct perf_top *top)
>   	pthread_t thread, thread_process;
>   	int ret;
>   
> -	top->session = perf_session__new(NULL, false, NULL);
> -	if (top->session == NULL)
> -		return -1;
> -
>   	if (!top->annotation_opts.objdump_path) {
>   		ret = perf_env__lookup_objdump(&top->session->header.env,
>   					       &top->annotation_opts.objdump_path);
>   		if (ret)
> -			goto out_delete;
> +			return ret;
>   	}
>   
>   	ret = callchain_param__setup_sample_type(&callchain_param);
>   	if (ret)
> -		goto out_delete;
> +		return ret;
>   
>   	if (perf_session__register_idle_thread(top->session) < 0)
> -		goto out_delete;
> +		return ret;
>   
>   	if (top->nr_threads_synthesize > 1)
>   		perf_set_multithreaded();
> @@ -1224,13 +1220,18 @@ static int __cmd_top(struct perf_top *top)
>   
>   	if (perf_hpp_list.socket) {
>   		ret = perf_env__read_cpu_topology_map(&perf_env);
> -		if (ret < 0)
> -			goto out_err_cpu_topo;
> +		if (ret < 0) {
> +			char errbuf[BUFSIZ];
> +			const char *err = str_error_r(-ret, errbuf, sizeof(errbuf));
> +
> +			ui__error("Could not read the CPU topology map: %s\n", err);
> +			return ret;
> +		}
>   	}
>   
>   	ret = perf_top__start_counters(top);
>   	if (ret)
> -		goto out_delete;
> +		return ret;
>   
>   	ret = perf_evlist__apply_drv_configs(evlist, &pos, &err_term);
>   	if (ret) {
> @@ -1257,7 +1258,7 @@ static int __cmd_top(struct perf_top *top)
>   	ret = -1;
>   	if (pthread_create(&thread_process, NULL, process_thread, top)) {
>   		ui__error("Could not create process thread.\n");
> -		goto out_delete;
> +		return ret;
>   	}
>   
>   	if (pthread_create(&thread, NULL, (use_browser > 0 ? display_thread_tui :
> @@ -1301,19 +1302,7 @@ static int __cmd_top(struct perf_top *top)
>   out_join_thread:
>   	pthread_cond_signal(&top->qe.cond);
>   	pthread_join(thread_process, NULL);
> -out_delete:
> -	perf_session__delete(top->session);
> -	top->session = NULL;
> -
>   	return ret;
> -
> -out_err_cpu_topo: {
> -	char errbuf[BUFSIZ];
> -	const char *err = str_error_r(-ret, errbuf, sizeof(errbuf));
> -
> -	ui__error("Could not read the CPU topology map: %s\n", err);
> -	goto out_delete;
> -}
>   }
>   
>   static int
> @@ -1644,10 +1633,17 @@ int cmd_top(int argc, const char **argv)
>   		signal(SIGWINCH, winch_sig);
>   	}
>   
> +	top.session = perf_session__new(NULL, false, NULL);
> +	if (top.session == NULL) {
> +		status = -1;
> +		goto out_delete_evlist;
> +	}
> +
>   	status = __cmd_top(&top);
>   
>   out_delete_evlist:
>   	perf_evlist__delete(top.evlist);
> +	perf_session__delete(top.session);
>   
>   	return status;
>   }
> 


This one breaks perf build like this:

builtin-top.c: In function '__cmd_top':
builtin-top.c:1241:3: error: label 'out_delete' used but not defined
    goto out_delete;


Suggested 5.0 specific fix attached.

--------------F151799C9EBAFCA68FED3310
Content-Type: text/x-patch; name="tools-perf-builtin-top-fix-build.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="tools-perf-builtin-top-fix-build.patch"


Subject: perf top: fix builtin-top build breakage.

In 5.0 -stable queue, backported upstream commit 0dba9e4be95b (perf top: Delete
the evlist before perf_session, fixing heap-use-after-free issue)

causes the perf build to break with:

builtin-top.c: In function '__cmd_top':
builtin-top.c:1241:3: error: label 'out_delete' used but not defined
   goto out_delete;
   ^~~~

This does not happend in upstream linus tree as the affected code is removed
in commit 159b0da50adb (perf pmu: Remove set_drv_config API) that I assume
is not ok to backport in -stable trees.

Fix it up like other code in commit 0dba9e4be95b.

Signed-off-by: Thomas Backlund <tmb@mageia.org>

--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1238,7 +1238,7 @@ static int __cmd_top(struct perf_top *to
 		pr_err("failed to set config \"%s\" on event %s with %d (%s)\n",
 			err_term->val.drv_cfg, perf_evsel__name(pos), errno,
 			str_error_r(errno, msg, sizeof(msg)));
-		goto out_delete;
+		return ret;
 	}
 
 	top->session->evlist = top->evlist;

--------------F151799C9EBAFCA68FED3310--