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

[Jiri Olsa]

On Mon, Nov 27, 2017 at 06:12:03PM +0100, Peter Zijlstra wrote:
> On Mon, Nov 27, 2017 at 06:09:11PM +0100, Jiri Olsa wrote:
> > On Mon, Nov 27, 2017 at 05:46:39PM +0100, Peter Zijlstra wrote:
> > > On Mon, Nov 27, 2017 at 05:21:31PM +0100, Jiri Olsa wrote:
> > > > +static int __modify_user_hw_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;
> > > > +	bool modify  = attr->bp_type != old_type;
> > > > +	int err = 0;
> > > > +
> > > > +	bp->attr.bp_addr = attr->bp_addr;
> > > > +	bp->attr.bp_type = attr->bp_type;
> > > > +	bp->attr.bp_len  = attr->bp_len;
> > > > +
> > > > +	err = validate_hw_breakpoint(bp);
> > > > +	if (!err && modify)
> > > > +		err = modify_bp_slot(bp, old_type);
> > > > +
> > > > +	if (err) {
> > > > +		bp->attr.bp_addr = old_addr;
> > > > +		bp->attr.bp_type = old_type;
> > > > +		bp->attr.bp_len  = old_len;
> > > > +		return err;
> > > > +	}
> > > > +
> > > > +	bp->attr.disabled = attr->disabled;
> > > > +	return 0;
> > > > +}
> > > 
> > > I think this function is failing to check if anything else in the attr
> > > changes.
> > > 
> > > For example, someone could have added PERF_SAMPLE_BRANCH_STACK. That's
> > > something you'll fail to create breakpoints with, but this modification
> > > would 'accept'.
> > > 
> > 
> > hum, I dont think so.. the only things you're allowed to change
> > are bp_addr, bp_type and bp_len.. we put new values in those
> > fields and keep the rest untouched.. apart from 'disabled' bit
> 
> But what validates the input attr is the same as the event attr, aside
> from those fields?

we don't.. the attr serves as a holder to carry those fields
into the function

the current kernel interface does not check anything else

there's one more check in the ioctl path, we check the
type in perf_event_modify_attr:

        if (event->attr.type != attr->type)
                return -EINVAL;


jirka