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