Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw_breakpoint function

[Jiri Olsa <jolsa@redhat.com>]

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