On Tue, Nov 07, 2017 at 09:15:41AM +0100, Peter Zijlstra wrote:
> On Mon, Nov 06, 2017 at 03:16:58PM -0800, Andi Kleen wrote:
> > > +static int _perf_event_modify_breakpoint(struct perf_event *bp,
> > > + struct perf_event_attr *attr)
> > > +{
> > > + u64 old_addr = bp->attr.bp_addr;
> > > + u64 old_len = bp->attr.bp_len;
> > > + int old_type = bp->attr.bp_type;
> > > + int err = 0;
> > > +
> > > + _perf_event_disable(bp);
> > > +
> > > + bp->attr.bp_addr = attr->bp_addr;
> > > + bp->attr.bp_type = attr->bp_type;
> > > + bp->attr.bp_len = attr->bp_len;
> >
> > You don't check any of the other fields, so user space is free
> > to fill in junk. That means they can never be used for anything.
> > It would be better to check at least some of them for being
> > zero, and also that the type matches the break point.
>
> Yes, the values should at the very least get the exact same validation
> they would get on creating an event with those values.
In this case the ioctl could be also generalized. Not call it _BREAKPOINT,
just _MODIFY. Just for now it would be only implemented for break points,
but that could be potentially extended later.
-Andi
