linux-kernel - Re: [PATCH] perf tui: avoid crash when annotating recursive functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YdyAG9G6uPWvTJ9g@kernel.org>
Date:   Mon, 10 Jan 2022 15:51:07 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Dario Petrillo <dario.pk1@...il.com>
Cc:     linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>
Subject: Re: [PATCH] perf tui: avoid crash when annotating recursive functions

Em Mon, Jan 10, 2022 at 12:44:41AM +0100, Dario Petrillo escreveu:
> In perf report, entering a recursive function from inside of itself
> (either directly of indirectly through some other function) results in
> calling symbol__annotate2 multiple times, and freeing the whole disassembly
> when exiting from the innermost instance. The first issue causes the
> function's disassembly to be duplicated, and the latter a heap
> use-after-free (and crash) when trying to access the disassembly again.
> 
> I reproduced the bug on perf 5.11.22 (Ubuntu 20.04.3 LTS) and 5.16.rc8
> with the following testcase (compile with gcc recursive.c -o recursive).
> To reproduce:
> - perf record ./recursive
> - perf report
> - enter fibonacci and annotate it
> - move the cursor on one of the "callq fibonacci" instructions and press enter
>   - at this point there will be two copies of the function in the disassembly
> - go back by pressing q, and perf will crash
> 
> #include <stdio.h>
> 
> int fibonacci(int n)
> {
>     if(n <= 2) return 1;
>     return fibonacci(n-1) + fibonacci(n-2);
> }
> 
> int main()
> {
>     printf("%d\n", fibonacci(40));
> }
> 
> This patch addresses the issue by annotating a function and freeing the
> associated memory on exit only if no annotation is already present,
> so that a recursive function is only annotated on entry.

Problem reproduced, patch applied, problem fixed.

Thanks, applied.

Signed-off-by: Arnaldo Carvalho de Melo <acme@...hat.com>
 
> Signed-off-by: Dario Petrillo <dario.pk1@...il.com>
> 
> ---
>  tools/perf/ui/browsers/annotate.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index e81c2493efdf..44ba900828f6 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -966,6 +966,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>  		.opts = opts,
>  	};
>  	int ret = -1, err;
> +	int not_annotated = list_empty(&notes->src->source);
>  
>  	if (sym == NULL)
>  		return -1;
> @@ -973,13 +974,15 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>  	if (ms->map->dso->annotate_warned)
>  		return -1;
>  
> -	err = symbol__annotate2(ms, evsel, opts, &browser.arch);
> -	if (err) {
> -		char msg[BUFSIZ];
> -		ms->map->dso->annotate_warned = true;
> -		symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
> -		ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
> -		goto out_free_offsets;
> +	if (not_annotated) {
> +		err = symbol__annotate2(ms, evsel, opts, &browser.arch);
> +		if (err) {
> +			char msg[BUFSIZ];
> +			ms->map->dso->annotate_warned = true;
> +			symbol__strerror_disassemble(ms, err, msg, sizeof(msg));
> +			ui__error("Couldn't annotate %s:\n%s", sym->name, msg);
> +			goto out_free_offsets;
> +		}
>  	}
>  
>  	ui_helpline__push("Press ESC to exit");
> @@ -994,9 +997,11 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
>  
>  	ret = annotate_browser__run(&browser, evsel, hbt);
>  
> -	annotated_source__purge(notes->src);
> +	if(not_annotated)
> +		annotated_source__purge(notes->src);
>  
>  out_free_offsets:
> -	zfree(&notes->offsets);
> +	if(not_annotated)
> +		zfree(&notes->offsets);
>  	return ret;
>  }
> -- 
> 2.25.1

-- 

- Arnaldo