On Mon, Nov 27, 2017 at 1:20 PM, Peter Zijlstra <[email protected]> wrote: > > On Mon, Nov 27, 2017 at 06:34:17PM +0100, Peter Zijlstra wrote: > > On Mon, Nov 27, 2017 at 06:25:32PM +0100, Jiri Olsa wrote: > > > On Mon, Nov 27, 2017 at 06:12:03PM +0100, Peter Zijlstra wrote: > > > > 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 > > > > Then that's a straight up bug. > > > > > the current kernel interface does not check anything else > > > > Not enough, if the new attr would fail perf_event_open() it should also > > fail this modify thing. > > > On IRC you asked: > > <jolsa> peterz, I dont follow.. why should we check fields that we dont use? > > Suppose someone does: > > attr = malloc(sizeof(*attr)); // uninitialized memory > attr->type = BP; > attr->bp_addr = new_addr; > attr->bp_type = bp_type; > attr->bp_len = bp_len; > ioctl(fd, PERF_IOC_MOD_ATTR, &attr); > > And feeds absolute shite for the rest of the fields. > > Then we later want to extend IOC_MOD_ATTR to allow changing > attr::sample_type but we can't, because that would break the above > application. > > Therefore we must be very strict to check only the fields we can change > have changed.
The possible checks is infinite and checking against what was used in perf_event_open will further complicate matters. The original objective was very simple: allow bp_addr, bp_type, and bp_len to be modified without the rest of the baggage.
