[PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[Jiri Olsa]

Currently the multi_kprobe link attach does not check error
injection list for programs with bpf_override_return helper
and allows them to attach anywhere. Adding the missing check.

Cc: stable@vger.kernel.org
Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index a7264b2c17ad..c1c1af63ced2 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
 	return arr.mods_cnt;
 }
 
+static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
+{
+	u32 i;
+
+	for (i = 0; i < cnt; i++) {
+		if (!within_error_injection_list(addrs[i]))
+			return -EINVAL;
+	}
+	return 0;
+}
+
 int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
 {
 	struct bpf_kprobe_multi_link *link = NULL;
@@ -2930,6 +2941,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
 			goto error;
 	}
 
+	if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
+		err = -EINVAL;
+		goto error;
+	}
+
 	link = kzalloc(sizeof(*link), GFP_KERNEL);
 	if (!link) {
 		err = -ENOMEM;
-- 
2.41.0

Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[Alan Maguire]

On 07/09/2023 21:06, Jiri Olsa wrote:
> Currently the multi_kprobe link attach does not check error
> injection list for programs with bpf_override_return helper
> and allows them to attach anywhere. Adding the missing check.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

For the series,

Reviewed-by: Alan Maguire <alan.maguire@oracle.com>

...with one small question below...

> ---
>  kernel/trace/bpf_trace.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index a7264b2c17ad..c1c1af63ced2 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
>  	return arr.mods_cnt;
>  }
>  
> +static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> +{
> +	u32 i;
> +
> +	for (i = 0; i < cnt; i++) {
> +		if (!within_error_injection_list(addrs[i]))
> +			return -EINVAL;

do we need a check like trace_kprobe_on_func_entry() to verify that
it's a combination of function entry+kprobe override, or is that
handled elsewhere/not needed? perf_event_attach_bpf_prog() does

/*
 * Kprobe override only works if they are on the function entry,
 * and only if they are on the opt-in list.
 */
 	if (prog->kprobe_override &&
 	    (!trace_kprobe_on_func_entry(event->tp_event) ||
 	     !trace_kprobe_error_injectable(event->tp_event)))
 		return -EINVAL;


if this is needed, it might be good to augment the selftest to
cover the case of specifying non-entry+kprobe override. thanks!

Alan


> +	return 0;
> +}
> +
>  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  {
>  	struct bpf_kprobe_multi_link *link = NULL;
> @@ -2930,6 +2941,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
>  			goto error;
>  	}
>  
> +	if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
> +		err = -EINVAL;
> +		goto error;
> +	}
> +
>  	link = kzalloc(sizeof(*link), GFP_KERNEL);
>  	if (!link) {
>  		err = -ENOMEM;

Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[Andrii Nakryiko]

On Fri, Sep 8, 2023 at 6:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On 07/09/2023 21:06, Jiri Olsa wrote:
> > Currently the multi_kprobe link attach does not check error
> > injection list for programs with bpf_override_return helper
> > and allows them to attach anywhere. Adding the missing check.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> For the series,
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
> ...with one small question below...
>
> > ---
> >  kernel/trace/bpf_trace.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index a7264b2c17ad..c1c1af63ced2 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
> >       return arr.mods_cnt;
> >  }
> >
> > +static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> > +{
> > +     u32 i;
> > +
> > +     for (i = 0; i < cnt; i++) {
> > +             if (!within_error_injection_list(addrs[i]))
> > +                     return -EINVAL;
>
> do we need a check like trace_kprobe_on_func_entry() to verify that
> it's a combination of function entry+kprobe override, or is that
> handled elsewhere/not needed? perf_event_attach_bpf_prog() does

multi-kprobe programs are always attached at function entry, so I
believe it's not necessary?

>
> /*
>  * Kprobe override only works if they are on the function entry,
>  * and only if they are on the opt-in list.
>  */
>         if (prog->kprobe_override &&
>             (!trace_kprobe_on_func_entry(event->tp_event) ||
>              !trace_kprobe_error_injectable(event->tp_event)))
>                 return -EINVAL;
>
>
> if this is needed, it might be good to augment the selftest to
> cover the case of specifying non-entry+kprobe override. thanks!
>
> Alan
>
>
> > +     return 0;
> > +}
> > +
> >  int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
> >  {
> >       struct bpf_kprobe_multi_link *link = NULL;
> > @@ -2930,6 +2941,11 @@ int bpf_kprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> >                       goto error;
> >       }
> >
> > +     if (prog->kprobe_override && addrs_check_error_injection_list(addrs, cnt)) {
> > +             err = -EINVAL;
> > +             goto error;
> > +     }
> > +
> >       link = kzalloc(sizeof(*link), GFP_KERNEL);
> >       if (!link) {
> >               err = -ENOMEM;

Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Thu,  7 Sep 2023 22:06:51 +0200 you wrote:
> Currently the multi_kprobe link attach does not check error
> injection list for programs with bpf_override_return helper
> and allows them to attach anywhere. Adding the missing check.
> 
> Cc: stable@vger.kernel.org
> Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Add override check to kprobe multi link attach
    https://git.kernel.org/bpf/bpf/c/41bc46c12a80
  - [bpf,2/2] selftests/bpf: Add kprobe_multi override test
    https://git.kernel.org/bpf/bpf/c/7182e56411b9

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH bpf 1/2] bpf: Add override check to kprobe multi link attach

[Jiri Olsa]

On Fri, Sep 08, 2023 at 04:50:54PM -0700, Andrii Nakryiko wrote:
> On Fri, Sep 8, 2023 at 6:49 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On 07/09/2023 21:06, Jiri Olsa wrote:
> > > Currently the multi_kprobe link attach does not check error
> > > injection list for programs with bpf_override_return helper
> > > and allows them to attach anywhere. Adding the missing check.
> > >
> > > Cc: stable@vger.kernel.org
> > > Fixes: 0dcac2725406 ("bpf: Add multi kprobe link")
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >
> > For the series,
> >
> > Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> >
> > ...with one small question below...
> >
> > > ---
> > >  kernel/trace/bpf_trace.c | 16 ++++++++++++++++
> > >  1 file changed, 16 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index a7264b2c17ad..c1c1af63ced2 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
> > >       return arr.mods_cnt;
> > >  }
> > >
> > > +static int addrs_check_error_injection_list(unsigned long *addrs, u32 cnt)
> > > +{
> > > +     u32 i;
> > > +
> > > +     for (i = 0; i < cnt; i++) {
> > > +             if (!within_error_injection_list(addrs[i]))
> > > +                     return -EINVAL;
> >
> > do we need a check like trace_kprobe_on_func_entry() to verify that
> > it's a combination of function entry+kprobe override, or is that
> > handled elsewhere/not needed? perf_event_attach_bpf_prog() does
> 
> multi-kprobe programs are always attached at function entry, so I
> believe it's not necessary?

yes, fprobe allows only function entry.. should have put it in comment

thanks,
jirka