linux-kernel - Re: [PATCH] perf report: Fix invalid memory accessing

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150907130314.GB23219@krava.brq.redhat.com>
Date:	Mon, 7 Sep 2015 15:03:14 +0200
From:	Jiri Olsa <jolsa@...hat.com>
To:	Wang Nan <wangnan0@...wei.com>
Cc:	acme@...nel.org, kan.liang@...el.com, linux-kernel@...r.kernel.org,
	lizefan@...wei.com, pi3orama@....com,
	Adrian Hunter <adrian.hunter@...el.com>,
	Andi Kleen <ak@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>,
	Namhyung Kim <namhyung@...nel.org>,
	Stephane Eranian <eranian@...gle.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH] perf report: Fix invalid memory accessing

On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote:
> Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add
> processor socket info to hist_entry and addr_location) reads env->cpu
> array for each sample using index al.cpu. However, al.cpu can be -1 if
> sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if
> feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu
> before setting al.socket.
> 
> Signed-off-by: Wang Nan <wangnan0@...wei.com>
> Cc: Kan Liang <kan.liang@...el.com>
> Cc: Adrian Hunter <adrian.hunter@...el.com>
> Cc: Andi Kleen <ak@...ux.intel.com>
> Cc: Jiri Olsa <jolsa@...nel.org>
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Stephane Eranian <eranian@...gle.com>
> Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> ---
> 
> Although theoretically CPU_TOPOLOGY feature should always be selected by
> 'perf record', I did generate a perf.data without that feature. It has
> header like this:
> 
>  # perf report -i ./bad.perf.data  --header-only          
>  # ========
>  # captured on: Thu Jan  8 09:30:15 2009
>  # hostname : localhost
>  # os release : 3.10.49-gd672fc4
>  # perf version : 4.2.gc9df
>  # arch : aarch64
>  # nrcpus online : 8
>  # nrcpus avail : 8
>  # total memory : 1850768 kB
>  # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 
>  # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
>  # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
>  # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>  # ========
>  #
> 
> It should be:
> 
>  # ========
>  # captured on: Thu Jan  8 11:26:41 2009
>  ...
>  # HEADER_CPU_TOPOLOGY info available, use -I to display
>  # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>  # ========
> 
> However, bad perf.data appears randomly. I can't stably reproduce it, so I
> guess there might have another invalid memory accessing.
> 
> ---
>  tools/perf/builtin-report.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 4b43245..16d097d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool,
>  		return -1;
>  	}
>  
> -	/* read socket id from perf.data for perf report */
> -	al.socket = env->cpu[al.cpu].socket_id;
> +	/*
> +	 * read socket id from perf.data for perf report
> +	 * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this
> +	 * sample.
> +	 * env->cpu is invalid if CPU_TOPOLOGY feature is not set in
> +	 * header.
> +	 */
> +	al.socket = -1;
> +	if (env->cpu && al.cpu >= 0)
> +		al.socket = env->cpu[al.cpu].socket_id;

perf_event__preprocess_sample initializes al.socket from current system
do we want to move this over there?

also this change is just report specific, and we could need
this in at least perf top

jirka
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/