From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753024AbbIJKCL (ORCPT ); Thu, 10 Sep 2015 06:02:11 -0400 Received: from casper.infradead.org ([85.118.1.10]:39079 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750741AbbIJKCJ (ORCPT ); Thu, 10 Sep 2015 06:02:09 -0400 Date: Thu, 10 Sep 2015 12:01:58 +0200 From: Peter Zijlstra To: Stephane Eranian Cc: Sasha Levin , Ingo Molnar , Vince Weaver , Jiri Olsa , "Liang, Kan" , LKML , Andrew Hunter , Maria Dimakopoulou Subject: Re: [PATCH 01/10] perf,x86: Fix event/group validation Message-ID: <20150910100157.GS3644@twins.programming.kicks-ass.net> References: <20150521111710.475482798@infradead.org> <20150521111932.592505273@infradead.org> <55D78AA0.6000106@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Sep 10, 2015 at 01:54:18AM -0700, Stephane Eranian wrote: > On Fri, Aug 21, 2015 at 1:31 PM, Sasha Levin wrote: > > > > On 05/21/2015 07:17 AM, Peter Zijlstra wrote: > > > --- a/arch/x86/kernel/cpu/perf_event_intel.c > > > +++ b/arch/x86/kernel/cpu/perf_event_intel.c > > > @@ -2106,7 +2106,7 @@ static struct event_constraint * > > > intel_get_event_constraints(struct cpu_hw_events *cpuc, int idx, > > > struct perf_event *event) > > > { > > > - struct event_constraint *c1 = event->hw.constraint; > > > + struct event_constraint *c1 = cpuc->event_constraint[idx]; > > > struct event_constraint *c2; > > > > Hey Peter, > > > > I was chasing a memory corruption in this area and I think I found > > a possible culprit: > > > > After this patch, In the code above, we'd access "cpuc->event_constraint[idx]" > > and read/change memory. > > > > The problem is that a valid value for idx is also -1, which isn't checked > > here, so we end up accessing and possibly corrupting memory that isn't ours. > > > > > I believe your analysis is correct, the following path will create the problem: > > validate_group() > validate_event() > x86_pmu.get_event_constraints(fake_cpuc, -1, event) > intel_get_event_constraints(cpuc, idx, event) > struct event_constraints *c1 = cpuc->event_constraints[idx]; > > here idx = -1, and the kernel is accessing an invalid memory location. > > If think the code could be changed to: > > struct event_constraint *c1 = NULL; > if (idx > -1) > c1 = cpuc->event_constraints[idx]; > > idx is not used in the __intel_get_event_constraints() path if I read > the code correctly. I prefer >= 0, but yes that looks about right. I still want to rework all this fake stuff some time, but we should fix this asap. Something like so then? --- Subject: perf, intel: Fix out-of-bound From: Peter Zijlstra Date: Thu Sep 10 11:58:27 CEST 2015 Sasha reported that we can get here with .idx==-1, and cpuc->event_constraints unallocated. Cc: stable@vger.kernel.org Fixes: b371b5943178 ("perf/x86: Fix event/group validation") Reported-by: Sasha Levin Suggested-by: Stephane Eranian Signed-off-by: Peter Zijlstra (Intel) --- arch/x86/kernel/cpu/perf_event_intel.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) --- a/arch/x86/kernel/cpu/perf_event_intel.c +++ b/arch/x86/kernel/cpu/perf_event_intel.c @@ -2316,9 +2316,12 @@ static struct event_constraint * intel_get_event_constraints(struct cpu_hw_events *cpuc, int idx, struct perf_event *event) { - struct event_constraint *c1 = cpuc->event_constraint[idx]; + struct event_constraint *c1 = NULL; struct event_constraint *c2; + if (idx >= 0) /* fake does < 0 */ + c1 = cpuc->event_constraint[idx]; + /* * first time only * - static constraint: no change across incremental scheduling calls