linux-kernel - Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw

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]

Date:   Tue, 28 Nov 2017 12:24:47 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andi Kleen <ak@...ux.intel.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Milind Chabbi <chabbi.milind@...il.com>,
        Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        David Ahern <dsahern@...il.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Hari Bathini <hbathini@...ux.vnet.ibm.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Kan Liang <kan.liang@...el.com>,
        Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
        Oleg Nesterov <onestero@...hat.com>,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH 4/6] hw_breakpoint: Factor out
 __modify_user_hw_breakpoint function

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);