[1/2] bpf: add a bpf_override_function helper

From: Josef Bacik <jbacik@fb.com>

Error injection is sloppy and very ad-hoc.  BPF could fill this niche
perfectly with it's kprobe functionality.  We could make sure errors are
only triggered in specific call chains that we care about with very
specific situations.  Accomplish this with the bpf_override_funciton
helper.  This will modify the probe'd callers return value to the
specified value and set the PC to an override function that simply
returns, bypassing the originally probed function.  This gives us a nice
clean way to implement systematic error injection for all of our code
paths.

Acked-by: Alexei Starovoitov <ast@kernel.org>
Signed-off-by: Josef Bacik <jbacik@fb.com>
---
 arch/Kconfig                     |  3 +++
 arch/x86/Kconfig                 |  1 +
 arch/x86/include/asm/kprobes.h   |  4 ++++
 arch/x86/include/asm/ptrace.h    |  5 +++++
 arch/x86/kernel/kprobes/ftrace.c | 14 ++++++++++++++
 include/linux/filter.h           |  3 ++-
 include/linux/trace_events.h     |  1 +
 include/uapi/linux/bpf.h         |  7 ++++++-
 kernel/bpf/verifier.c            |  2 ++
 kernel/events/core.c             |  7 +++++++
 kernel/trace/Kconfig             | 11 +++++++++++
 kernel/trace/bpf_trace.c         | 35 +++++++++++++++++++++++++++++++++++
 kernel/trace/trace_kprobe.c      | 40 +++++++++++++++++++++++++++++++++-------
 kernel/trace/trace_probe.h       |  6 ++++++
 14 files changed, 130 insertions(+), 9 deletions(-)

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?

>   	if (is_tracepoint || is_syscall_tp) {
>   		int off = trace_event_get_offsets(event->tp_event);

Thanks,
Daniel

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

On 11/03/2017 03:31 PM, Josef Bacik wrote:
> 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?

Yeah.

> 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

With BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array
for the tracing/multiprog attach point? The program you're calling into
is inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time
and can have nesting as well.

> https://patchwork.kernel.org/patch/10034815/
>
> and I need to bring that back right?  Thanks,

I'm afraid so. The thing with skb cb_access which was brought up there is
that once you have a tail call in the prog you cannot make any assumptions
anymore, therefore the cb_access flag is set to 1 so we save/restore for
those cases precautionary since it could be accessed or not later on. In
your case I think this wouldn't work since legitimate bpf kprobes progs could
use tail calls today, so setting prog->kprobe_override there would prevent
attaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE
check.

On Fri, Nov 03, 2017 at 05:52:22PM +0100, Daniel Borkmann wrote:
> On 11/03/2017 03:31 PM, Josef Bacik wrote:
> > 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?
> 
> Yeah.
> 
> > 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
> 
> With BPF_PROG_ARRAY you mean BPF_MAP_TYPE_PROG_ARRAY or the prog array
> for the tracing/multiprog attach point? The program you're calling into
> is inside the BPF_MAP_TYPE_PROG_ARRAY map, but can change at any time
> and can have nesting as well.
> 
> > https://patchwork.kernel.org/patch/10034815/
> > 
> > and I need to bring that back right?  Thanks,
> 
> I'm afraid so. The thing with skb cb_access which was brought up there is
> that once you have a tail call in the prog you cannot make any assumptions
> anymore, therefore the cb_access flag is set to 1 so we save/restore for
> those cases precautionary since it could be accessed or not later on. In
> your case I think this wouldn't work since legitimate bpf kprobes progs could
> use tail calls today, so setting prog->kprobe_override there would prevent
> attaching for non-kprobes due to subsequent flags & TRACE_EVENT_FL_KPROBE
> check.

how about preventing programs that use bpf_override_return to be
store into prog_array? imo that would be cleaner solution.
doing tail_call into them is kinda useless anyway.
Then we can keep the bit and fast path.