linux-kernel - Re: [PATCH 1/2] bpf: add a bpf_override

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:   Fri, 3 Nov 2017 10:31:36 -0400
From:   Josef Bacik <josef@...icpanda.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Josef Bacik <josef@...icpanda.com>, rostedt@...dmis.org,
        mingo@...hat.com, davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ast@...nel.org, kernel-team@...com,
        Josef Bacik <jbacik@...com>
Subject: Re: [PATCH 1/2] bpf: add a bpf_override_function helper

On Fri, Nov 03, 2017 at 12:12:13AM +0100, Daniel Borkmann wrote:
> Hi Josef,
> 
> one more issue I just noticed, see comment below:
> 
> On 11/02/2017 03:37 PM, Josef Bacik wrote:
> [...]
> > diff --git a/include/linux/filter.h b/include/linux/filter.h
> > index cdd78a7beaae..dfa44fd74bae 100644
> > --- a/include/linux/filter.h
> > +++ b/include/linux/filter.h
> > @@ -458,7 +458,8 @@ struct bpf_prog {
> >   				locked:1,	/* Program image locked? */
> >   				gpl_compatible:1, /* Is filter GPL compatible? */
> >   				cb_access:1,	/* Is control block accessed? */
> > -				dst_needed:1;	/* Do we need dst entry? */
> > +				dst_needed:1,	/* Do we need dst entry? */
> > +				kprobe_override:1; /* Do we override a kprobe? */
> >   	kmemcheck_bitfield_end(meta);
> >   	enum bpf_prog_type	type;		/* Type of BPF program */
> >   	u32			len;		/* Number of filter blocks */
> [...]
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index d906775e12c1..f8f7927a9152 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -4189,6 +4189,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
> >   			prog->dst_needed = 1;
> >   		if (insn->imm == BPF_FUNC_get_prandom_u32)
> >   			bpf_user_rnd_init_once();
> > +		if (insn->imm == BPF_FUNC_override_return)
> > +			prog->kprobe_override = 1;
> >   		if (insn->imm == BPF_FUNC_tail_call) {
> >   			/* If we tail call into other programs, we
> >   			 * cannot make any assumptions since they can
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 9660ee65fbef..0d7fce52391d 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -8169,6 +8169,13 @@ static int perf_event_set_bpf_prog(struct perf_event *event, u32 prog_fd)
> >   		return -EINVAL;
> >   	}
> > 
> > +	/* Kprobe override only works for kprobes, not uprobes. */
> > +	if (prog->kprobe_override &&
> > +	    !(event->tp_event->flags & TRACE_EVENT_FL_KPROBE)) {
> > +		bpf_prog_put(prog);
> > +		return -EINVAL;
> > +	}
> 
> Can we somehow avoid the prog->kprobe_override flag here completely
> and also same in the perf_event_attach_bpf_prog() handler?
> 
> Reason is that it's not reliable for bailing out this way: Think of
> the main program you're attaching doesn't use bpf_override_return()
> helper, but it tail-calls into other BPF progs that make use of it
> instead. So above check would be useless and will fail and we continue
> to attach the prog for probes where it's not intended to be used.
> 
> We've had similar issues in the past e.g. c2002f983767 ("bpf: fix
> checking xdp_adjust_head on tail calls") is just one of those. Thus,
> can we avoid the flag altogether and handle such error case differently?
> 

So if I'm reading this right there's no way to know what we'll tail call at any
given point, so I need to go back to my previous iteration of this patch and
always save the state of the kprobe in the per-cpu variable to make sure we
don't use bpf_override_return in the wrong case?

The tail call functions won't be in the BPF_PROG_ARRAY right?  It'll be just
some other arbitrary function?  If that's the case then we really need something
like this

https://patchwork.kernel.org/patch/10034815/

and I need to bring that back right?  Thanks,

Josef