From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755223AbdK1LYy (ORCPT ); Tue, 28 Nov 2017 06:24:54 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52904 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755150AbdK1LYv (ORCPT ); Tue, 28 Nov 2017 06:24:51 -0500 Date: Tue, 28 Nov 2017 12:24:47 +0100 From: Jiri Olsa To: Andi Kleen Cc: Peter Zijlstra , Milind Chabbi , Jiri Olsa , Ingo Molnar , Arnaldo Carvalho de Melo , lkml , Namhyung Kim , David Ahern , Alexander Shishkin , Michael Ellerman , Hari Bathini , Jin Yao , Kan Liang , Sukadev Bhattiprolu , Oleg Nesterov , Will Deacon Subject: Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function Message-ID: <20171128112447.GA15683@krava> References: <20171127162133.21163-5-jolsa@kernel.org> <20171127164639.3ymnc6io3eae7n4c@hirez.programming.kicks-ass.net> <20171127170911.GA22026@krava> <20171127171203.tmdvcsnsownieijv@hirez.programming.kicks-ass.net> <20171127172532.GA23094@krava> <20171127173417.eokpkznt65yreoav@hirez.programming.kicks-ass.net> <20171127212003.aauonfkbl45pd7dj@hirez.programming.kicks-ass.net> <20171127220128.kzgywcu5ucudaeyl@hirez.programming.kicks-ass.net> <20171127230747.GC3070@tassilo.jf.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171127230747.GC3070@tassilo.jf.intel.com> User-Agent: Mutt/1.9.1 (2017-09-22) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 28 Nov 2017 11:24:51 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Nov 27, 2017 at 03:07:47PM -0800, Andi Kleen wrote: > On Mon, Nov 27, 2017 at 11:01:28PM +0100, Peter Zijlstra wrote: > > On Mon, Nov 27, 2017 at 01:50:30PM -0800, Milind Chabbi wrote: > > > The possible checks is infinite > > > > struct perf_event_attr is very much a finite data type. > > > > Something as simple as: > > > > struct perf_event_attr tmp1 = new_attr, tmp2 = event->attr; > > > > tmp1.bp_type = tmp2.bp_type; > > tmp1.bp_addr = tmp2.bp_addr; > > tmp1.bp_len = tmp2.bp_len; > > > > if (memcmp(&tmp1, &tmp2, sizeof(tmp1))) > > return -EINVAL; > > > > would actually do the checks __modify_user_hw_breakpoint() needs to do. > > It could fail with uninitialized padding. I think that should be fine.. both attrs go through perf_copy_attr, which should check on it.. I found we init attr.sample_max_stack out of perf_copy_attr, but we can move it there (attached) also modify_user_hw_breakpoint is exported.. not sure we can add this contrain and potentionaly break some kernel module? I check kernel all the current kernel users and they copy the whole perf_event_attr into attr argument before they change the allowed bp_* fields, so there's no harm. thanks, jirka --- diff --git a/kernel/events/core.c b/kernel/events/core.c index 799bb352d99f..028adb24bf7a 100644 --- a/kernel/events/core.c +++ b/kernel/events/core.c @@ -9673,6 +9673,9 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr, ret = -EINVAL; } + if (!attr->sample_max_stack) + attr->sample_max_stack = sysctl_perf_event_max_stack; + if (attr->sample_type & PERF_SAMPLE_REGS_INTR) ret = perf_reg_validate(attr->sample_regs_intr); out: @@ -9886,9 +9889,6 @@ SYSCALL_DEFINE5(perf_event_open, perf_paranoid_kernel() && !capable(CAP_SYS_ADMIN)) return -EACCES; - if (!attr.sample_max_stack) - attr.sample_max_stack = sysctl_perf_event_max_stack; - /* * In cgroup mode, the pid argument is used to pass the fd * opened to the cgroup directory in cgroupfs. The cpu argument diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c index a556aba223da..7b85160393b7 100644 --- a/kernel/events/hw_breakpoint.c +++ b/kernel/events/hw_breakpoint.c @@ -468,6 +468,9 @@ static int __modify_user_hw_breakpoint(struct perf_event *bp, struct perf_event_ bp->attr.bp_type = attr->bp_type; bp->attr.bp_len = attr->bp_len; + if (memcmp(&bp->attr, attr, sizeof(*attr))) + return -EINVAL; + err = validate_hw_breakpoint(bp); if (!err && modify) err = modify_bp_slot(bp, old_type);