On Wed, Feb 09, 2022 at 12:33:24PM IST, Kumar Kartikeya Dwivedi wrote:
> 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@xxxxxxxxx>
> ---
> .../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.
> + */
Also, in this case, there seems to be a difference of behavior. When we do
bpf_map_update_elem for array map, it seems to invoke
check_and_free_timer_in_array and free any timers that were part of the value.
In case of hash/lru_hash, that doesn't seem to happen, hence why the test has
two 'modes', to then trigger a dereference of the overwritten pointer using
bpf_timer_cancel.
So in case of array map it crashes right when doing bpf_map_update_elem, and in
case of hash it crashes inside bpf_timer_cancel.
This seems inconsistent to me, is there a specific reason behind doing it for
array map differently than hash map? If not, is it now too late to change this?
> + 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.35.1
>
--
Kartikeya
