| Message ID | 20230907200652.926951-1-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 41bc46c12a8053a1b3279a379bd6b5e87b045b85 |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf,1/2] bpf: Add override check to kprobe multi link attach | expand |
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;
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;
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!
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
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;
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(+)