Skip to contentSkip to conversation

bpf: Add override check to kprobe multi link attach

PATCH Applied Jiri Olsa 5 participants 6 messages Sep 7, 2023 – Sep 9, 2023

Subsystems (7)
Stable branch Kselftest BPF Tracing +3 more subsystems
lore
Share & tools
lore

Jiri Olsa

author reviewer
#0 lore raw

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

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 [email protected]
Fixes 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by Jiri Olsa <[email protected]>
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
kernel/trace/bpf_trace.c +16 -0
index a7264b2c17ad..c1c1af63ced2 100644
@@ -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;
Signature · 1 line
2.41.0
author reviewer
#1 lore raw

PATCH 2/2 selftests/bpf: Add kprobe_multi override test

Adding test that tries to attach program with bpf_override_return helper to function not within error injection list.

Signed-off-by Jiri Olsa <[email protected]>
---
 .../bpf/prog_tests/kprobe_multi_test.c        | 37 +++++++++++++++++++
 .../bpf/progs/kprobe_multi_override.c         | 13 +++++++
 2 files changed, 50 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/progs/kprobe_multi_override.c
tools/testing/selftests/bpf/prog_tests/kprobe_multi_test.c +37 -0
index 179fe300534f..e05477b210a5 100644
@@ -3,6 +3,7 @@
 #include "kprobe_multi.skel.h"
 #include "trace_helpers.h"
 #include "kprobe_multi_empty.skel.h"
+#include "kprobe_multi_override.skel.h"
 #include "bpf/libbpf_internal.h"
 #include "bpf/hashmap.h"
 

@@ -453,6 +454,40 @@ static void test_kprobe_multi_bench_attach(bool kernel)
 	}
 }
 
+void test_attach_override(void)
+{
+	struct kprobe_multi_override *skel = NULL;
+	struct bpf_link *link = NULL;
+
+	skel = kprobe_multi_override__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "kprobe_multi_empty__open_and_load"))
+		goto cleanup;
+
+	/* The test_override calls bpf_override_return so it should fail
+	 * to attach to bpf_fentry_test1 function, which is not on error
+	 * injection list.
+	 */
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_override,
+						     "bpf_fentry_test1", NULL);
+	if (!ASSERT_ERR_PTR(link, "override_attached_bpf_fentry_test1")) {
+		bpf_link__destroy(link);
+		goto cleanup;
+	}
+
+	/* The should_fail_bio function is on error injection list,
+	 * attach should succeed.
+	 */
+	link = bpf_program__attach_kprobe_multi_opts(skel->progs.test_override,
+						     "should_fail_bio", NULL);
+	if (!ASSERT_OK_PTR(link, "override_attached_should_fail_bio"))
+		goto cleanup;
+
+	bpf_link__destroy(link);
+
+cleanup:
+	kprobe_multi_override__destroy(skel);
+}
+
 void serial_test_kprobe_multi_bench_attach(void)
 {
 	if (test__start_subtest("kernel"))

@@ -480,4 +515,6 @@ void test_kprobe_multi_test(void)
 		test_attach_api_syms();
 	if (test__start_subtest("attach_api_fails"))
 		test_attach_api_fails();
+	if (test__start_subtest("attach_override"))
+		test_attach_override();
 }
tools/testing/selftests/bpf/progs/kprobe_multi_override.c new file +13 -0
new file mode 100644
index 000000000000..28f8487c9059
@@ -0,0 +1,13 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+char _license[] SEC("license") = "GPL";
+
+SEC("kprobe.multi")
+int test_override(struct pt_regs *ctx)
+{
+	bpf_override_return(ctx, 123);
+	return 0;
+}
Signature · 1 line
2.41.0
Reviewed-by on v1 1/2
#2 lore raw
In reply to Jiri Olsa
Quoted 7 lines · 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 [email protected]
Fixes 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by Jiri Olsa <[email protected]>

For the series,

Reviewed-by Alan Maguire <[email protected]>

...with one small question below...

Quoted 9 lines
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
kernel/trace/bpf_trace.c +0 -0
index a7264b2c17ad..c1c1af63ced2 100644

kernel/trace/bpf_trace.c static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3 · line 2862

Quoted 10 lines
kernel/trace/bpf_trace.c +7 -0
@@ -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

Quoted 18 lines
kernel/trace/bpf_trace.c +8 -0
+	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;
#3 lore raw
Quoted 28 lines · Alan Maguire
Quoted 7 lines · 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 [email protected]
Fixes 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by Jiri Olsa <[email protected]>

For the series,

Reviewed-by Alan Maguire <[email protected]>

...with one small question below...

Quoted 12 lines
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
kernel/trace/bpf_trace.c +0 -0
index a7264b2c17ad..c1c1af63ced2 100644
@@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
      return arr.mods_cnt;
 }
Quoted 11 lines

kernel/trace/bpf_trace.c static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3 · line 2862

Quoted 7 lines
kernel/trace/bpf_trace.c +7 -0
+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?

Quoted 35 lines

/*
* 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

Quoted 18 lines
kernel/trace/bpf_trace.c +8 -0
+     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;
#4 lore raw
Quoted 41 lines · Andrii Nakryiko
Quoted 28 lines · Alan Maguire
Quoted 7 lines · 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 [email protected]
Fixes 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by Jiri Olsa <[email protected]>

For the series,

Reviewed-by Alan Maguire <[email protected]>

...with one small question below...

Quoted 12 lines
---
 kernel/trace/bpf_trace.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
kernel/trace/bpf_trace.c +0 -0
index a7264b2c17ad..c1c1af63ced2 100644
@@ -2853,6 +2853,17 @@ static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3
      return arr.mods_cnt;
 }
Quoted 11 lines

kernel/trace/bpf_trace.c static int get_modules_for_addrs(struct module ***mods, unsigned long *addrs, u3 · line 2862

Quoted 7 lines
kernel/trace/bpf_trace.c +7 -0
+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

Quoted 2 lines

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

CI Applied
#5 lore raw
In reply to Jiri Olsa
This series was applied to bpf/bpf.git (master) by Andrii Nakryiko <[email protected]>:

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <[email protected]>:

Quoted 9 lines · 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 [email protected]
Fixes 0dcac2725406 ("bpf: Add multi kprobe link")
Signed-off-by Jiri Olsa <[email protected]>

[...]

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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html