Re: [PATCH 5.10 37/71] selftests/bpf: Add test for bpf_timer overwriting crash

Re: [PATCH 5.10 37/71] selftests/bpf: Add test for bpf_timer overwriting crash

[gregkh]

On Fri, Mar 18, 2022 at 02:42:49PM +0000, Geliang Tang wrote:
> Hi Greg,
> 
> I got this bpf selftests build break today on the stable branch 5.10.106:
> 
> =========================================================================
> CLNG-LLC [test_maps] test_tracepoint.o
> progs/timer_crash.c:8:19: error: field has incomplete type 'struct bpf_timer'
>         struct bpf_timer timer;
>                          ^
> progs/timer_crash.c:8:9: note: forward declaration of 'struct bpf_timer'
>         struct bpf_timer timer;
>                ^
> progs/timer_crash.c:35:6: warning: implicit declaration of function 'bpf_get_current_task_btf' is invalid in C99 [-Wimplicit-function-declaration]
>         if (bpf_get_current_task_btf()->tgid != pid)
>             ^
> progs/timer_crash.c:35:34: error: member reference type 'int' is not a pointer
>         if (bpf_get_current_task_btf()->tgid != pid)
>             ~~~~~~~~~~~~~~~~~~~~~~~~~~  ^
> progs/timer_crash.c:49:3: warning: implicit declaration of function 'bpf_timer_cancel' is invalid in C99 [-Wimplicit-function-declaration]
>                 bpf_timer_cancel(&e->timer);
>                 ^
> 2 warnings and 2 errors generated.
>   CLNG-LLC [test_maps] test_trace_ext_tracing.o
> llc: error: llc: <stdin>:1:1: error: expected top-level entity
> BPF obj compilation failed
> ^
> make: *** [Makefile:402: tools/testing/selftests/bpf/timer_crash.o] Error 1
> make: *** Waiting for unfinished jobs....
>   CLNG-LLC [test_maps] test_trace_ext.o
> =========================================================================
> 
> It is introduced by this commit, "selftests/bpf: Add test for bpf_timer
> overwriting crash". Since the commit "bpf: Introduce bpf timers." has not
> been merged into the stable branch yet.
> 
> I am writing to you to report this bug.
> 

Now reverted, thanks!

greg k-h

[PATCH 5.10 37/71] selftests/bpf: Add test for bpf_timer overwriting crash

[Greg Kroah-Hartman]

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>

[ Upstream commit a7e75016a0753c24d6c995bc02501ae35368e333 ]

Add a test that validates that timer value is not overwritten when doing
a copy_map_value call in the kernel. Without the prior fix, this test
triggers a crash.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Link: https://lore.kernel.org/bpf/20220209070324.1093182-3-memxor@gmail.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 .../selftests/bpf/prog_tests/timer_crash.c    | 32 +++++++++++
 .../testing/selftests/bpf/progs/timer_crash.c | 54 +++++++++++++++++++
 2 files changed, 86 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/timer_crash.c
 create mode 100644 tools/testing/selftests/bpf/progs/timer_crash.c

diff --git a/tools/testing/selftests/bpf/prog_tests/timer_crash.c b/tools/testing/selftests/bpf/prog_tests/timer_crash.c
new file mode 100644
index 000000000000..f74b82305da8
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/timer_crash.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+#include "timer_crash.skel.h"
+
+enum {
+	MODE_ARRAY,
+	MODE_HASH,
+};
+
+static void test_timer_crash_mode(int mode)
+{
+	struct timer_crash *skel;
+
+	skel = timer_crash__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "timer_crash__open_and_load"))
+		return;
+	skel->bss->pid = getpid();
+	skel->bss->crash_map = mode;
+	if (!ASSERT_OK(timer_crash__attach(skel), "timer_crash__attach"))
+		goto end;
+	usleep(1);
+end:
+	timer_crash__destroy(skel);
+}
+
+void test_timer_crash(void)
+{
+	if (test__start_subtest("array"))
+		test_timer_crash_mode(MODE_ARRAY);
+	if (test__start_subtest("hash"))
+		test_timer_crash_mode(MODE_HASH);
+}
diff --git a/tools/testing/selftests/bpf/progs/timer_crash.c b/tools/testing/selftests/bpf/progs/timer_crash.c
new file mode 100644
index 000000000000..f8f7944e70da
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/timer_crash.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+struct map_elem {
+	struct bpf_timer timer;
+	struct bpf_spin_lock lock;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct map_elem);
+} amap SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_HASH);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct map_elem);
+} hmap SEC(".maps");
+
+int pid = 0;
+int crash_map = 0; /* 0 for amap, 1 for hmap */
+
+SEC("fentry/do_nanosleep")
+int sys_enter(void *ctx)
+{
+	struct map_elem *e, value = {};
+	void *map = crash_map ? (void *)&hmap : (void *)&amap;
+
+	if (bpf_get_current_task_btf()->tgid != pid)
+		return 0;
+
+	*(void **)&value = (void *)0xdeadcaf3;
+
+	bpf_map_update_elem(map, &(int){0}, &value, 0);
+	/* For array map, doing bpf_map_update_elem will do a
+	 * check_and_free_timer_in_array, which will trigger the crash if timer
+	 * pointer was overwritten, for hmap we need to use bpf_timer_cancel.
+	 */
+	if (crash_map == 1) {
+		e = bpf_map_lookup_elem(map, &(int){0});
+		if (!e)
+			return 0;
+		bpf_timer_cancel(&e->timer);
+	}
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1