Re: [PATCH bpf v2 2/2] selftests/bpf: Add test for bpf_timer overwriting crash

[Yonghong Song <yhs@fb.com>]

On 2/9/22 12:03 PM, Kumar Kartikeya Dwivedi wrote:
> 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@gmail.com>
>> ---
>>   .../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?

There is no map deletion operation for array map, so the timer is 
canceled and freed when a new update comes in which signals the previous
value is gone.

for hashmap, depending on whether a new element will be allocated or 
not. If it is, the old element (if exists) will have its timer freed.

> 
>> +	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