From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751911AbdHAIL1 (ORCPT ); Tue, 1 Aug 2017 04:11:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35690 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751102AbdHAILY (ORCPT ); Tue, 1 Aug 2017 04:11:24 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6A4D8104AE1 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=jolsa@redhat.com Date: Tue, 1 Aug 2017 10:11:21 +0200 From: Jiri Olsa To: Andi Kleen Cc: acme@kernel.org, jolsa@kernel.org, linux-kernel@vger.kernel.org, Andi Kleen Subject: Re: [PATCH v1 01/15] perf, tools, stat: Fix buffer overflow while freeing events Message-ID: <20170801081121.GA7799@krava> References: <20170724234015.5165-1-andi@firstfloor.org> <20170724234015.5165-2-andi@firstfloor.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170724234015.5165-2-andi@firstfloor.org> User-Agent: Mutt/1.8.3 (2017-05-23) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 01 Aug 2017 08:11:23 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 24, 2017 at 04:40:01PM -0700, Andi Kleen wrote: SNIP > > The event is allocated with cpus == 1, but freed with cpus == real number > When the evsel close function walks the file descriptors it exceeds the > fd xyarray boundaries and reads random memory. > > Just make sure to always use the same dummy cpu map following > the same logic as the open call. > > Signed-off-by: Andi Kleen > --- > tools/perf/builtin-stat.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c > index 48ac53b199fc..97d6b6c42014 100644 > --- a/tools/perf/builtin-stat.c > +++ b/tools/perf/builtin-stat.c > @@ -715,6 +715,8 @@ static int __run_perf_stat(int argc, const char **argv) > * group leaders. > */ > read_counters(); > + if (!target__has_cpu(&target)) > + evsel_list->cpus = cpu_map__dummy_new(); you're leaking evsel_list->cpus right here.. I can see there's the issue when we mix system_wide event (with cpumask defined) and normal event: - we open such group as per_thread events (not system_wide), forcing both evsel->fd xyarray to be allocated from dummy cpus (with ncpus == 1) - but when we call perf_evlist__close we take ncpus from int n = evsel->cpus ? evsel->cpus->nr : ncpus; which is wrong for system_wide event and will cause the xyarray out of bounds access I can see the solution either in: 1) keeping the bounds for xyarray in it and use it for iterations 2) or forcing system_wide target if there's single system_wide event specified (patch below) but not sure there's any sense in meassuting system_wide event in non system_wide mode (ad 1).. thoughts? thanks, jirka --- diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 866da7aa54bf..9ccb4d671568 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -2540,8 +2540,8 @@ static void setup_system_wide(int forks) * conditions is met: * * - there's no workload specified - * - there is workload specified but all requested - * events are system wide events + * - there is workload specified and one + * of the events is system wide */ if (!target__none(&target)) return; @@ -2552,12 +2552,11 @@ static void setup_system_wide(int forks) struct perf_evsel *counter; evlist__for_each_entry(evsel_list, counter) { - if (!counter->system_wide) + if (counter->system_wide) { + target.system_wide = true; return; + } } - - if (evsel_list->nr_entries) - target.system_wide = true; } }