linux-kernel - Re: [PATCH 4/6] hw_breakpoint: Factor out __modify_user_hw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 27 Nov 2017 18:25:32 +0100
From:   Jiri Olsa <jolsa@...hat.com>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Jiri Olsa <jolsa@...nel.org>, Ingo Molnar <mingo@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        lkml <linux-kernel@...r.kernel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        David Ahern <dsahern@...il.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Milind Chabbi <chabbi.milind@...il.com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Hari Bathini <hbathini@...ux.vnet.ibm.com>,
        Jin Yao <yao.jin@...ux.intel.com>,
        Kan Liang <kan.liang@...el.com>,
        Sukadev Bhattiprolu <sukadev@...ux.vnet.ibm.com>,
        Oleg Nesterov <onestero@...hat.com>,
        Will Deacon <will.deacon@....com>
Subject: Re: [PATCH 4/6] hw_breakpoint: Factor out
 __modify_user_hw_breakpoint function

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