[PATCH bpf v3 0/3] Don't reinit map value in prealloc_lru_pop

[PATCH bpf v3 0/3] Don't reinit map value in prealloc_lru_pop

[Kumar Kartikeya Dwivedi]

Fix for a bug in prealloc_lru_pop spotted while reading the code, then a test +
example that checks whether it is fixed.

Changelog:
----------
v2 -> v3:
v2: https://lore.kernel.org/bpf/20220809140615.21231-1-memxor@gmail.com

 * Switch test to use kptr instead of kptr_ref to stabilize test runs
 * Fix missing lru_bug__destroy (Yonghong)
 * Collect Acks

v1 -> v2:
v1: https://lore.kernel.org/bpf/20220806014603.1771-1-memxor@gmail.com

 * Expand commit log to include summary of the discussion with Yonghong
 * Make lru_bug selftest serial to not mess up refcount for map_kptr test

Kumar Kartikeya Dwivedi (3):
  bpf: Allow calling bpf_prog_test kfuncs in tracing programs
  bpf: Don't reinit map value in prealloc_lru_pop
  selftests/bpf: Add test for prealloc_lru_pop bug

 kernel/bpf/hashtab.c                          |  6 +--
 net/bpf/test_run.c                            |  1 +
 .../selftests/bpf/prog_tests/lru_bug.c        | 21 ++++++++
 tools/testing/selftests/bpf/progs/lru_bug.c   | 49 +++++++++++++++++++
 4 files changed, 72 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lru_bug.c
 create mode 100644 tools/testing/selftests/bpf/progs/lru_bug.c

-- 
2.34.1

[PATCH bpf v3 1/3] bpf: Allow calling bpf_prog_test kfuncs in tracing programs

[Kumar Kartikeya Dwivedi]

In addition to TC hook, enable these in tracing programs so that they
can be used in selftests.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 net/bpf/test_run.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index cbc9cd5058cb..d11209367dd0 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -1628,6 +1628,7 @@ static int __init bpf_prog_test_run_init(void)
 	int ret;
 
 	ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_SCHED_CLS, &bpf_prog_test_kfunc_set);
+	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_prog_test_kfunc_set);
 	return ret ?: register_btf_id_dtor_kfuncs(bpf_prog_test_dtor_kfunc,
 						  ARRAY_SIZE(bpf_prog_test_dtor_kfunc),
 						  THIS_MODULE);
-- 
2.34.1

[PATCH bpf v3 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Kumar Kartikeya Dwivedi]

The LRU map that is preallocated may have its elements reused while
another program holds a pointer to it from bpf_map_lookup_elem. Hence,
only check_and_free_fields is appropriate when the element is being
deleted, as it ensures proper synchronization against concurrent access
of the map value. After that, we cannot call check_and_init_map_value
again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while
they can be concurrently accessed from a BPF program.

This is safe to do as when the map entry is deleted, concurrent access
is protected against by check_and_free_fields, i.e. an existing timer
would be freed, and any existing kptr will be released by it. The
program can create further timers and kptrs after check_and_free_fields,
but they will eventually be released once the preallocated items are
freed on map destruction, even if the item is never reused again. Hence,
the deleted item sitting in the free list can still have resources
attached to it, and they would never leak.

With spin_lock, we never touch the field at all on delete or update, as
we may end up modifying the state of the lock. Since the verifier
ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock
call, the program will eventually release the lock so that on reuse the
new user of the value can take the lock.

Essentially, for the preallocated case, we must assume that the map
value may always be in use by the program, even when it is sitting in
the freelist, and handle things accordingly, i.e. use proper
synchronization inside check_and_free_fields, and never reinitialize the
special fields when it is reused on update.

Fixes: 68134668c17f ("bpf: Add map side support for bpf timers.")
Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/hashtab.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index da7578426a46..4d793a92301b 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -311,12 +311,8 @@ static struct htab_elem *prealloc_lru_pop(struct bpf_htab *htab, void *key,
 	struct htab_elem *l;
 
 	if (node) {
-		u32 key_size = htab->map.key_size;
-
 		l = container_of(node, struct htab_elem, lru_node);
-		memcpy(l->key, key, key_size);
-		check_and_init_map_value(&htab->map,
-					 l->key + round_up(key_size, 8));
+		memcpy(l->key, key, htab->map.key_size);
 		return l;
 	}
 
-- 
2.34.1

[PATCH bpf v3 3/3] selftests/bpf: Add test for prealloc_lru_pop bug

[Kumar Kartikeya Dwivedi]

Add a regression test to check against invalid check_and_init_map_value
call inside prealloc_lru_pop.

The kptr should not be reset to NULL once we set it after deleting the
map element. Hence, we trigger a program that updates the element
causing its reuse, and checks whether the unref kptr is reset or not.
If it is, prealloc_lru_pop does an incorrect check_and_init_map_value
call and the test fails.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../selftests/bpf/prog_tests/lru_bug.c        | 21 ++++++++
 tools/testing/selftests/bpf/progs/lru_bug.c   | 49 +++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/lru_bug.c
 create mode 100644 tools/testing/selftests/bpf/progs/lru_bug.c

diff --git a/tools/testing/selftests/bpf/prog_tests/lru_bug.c b/tools/testing/selftests/bpf/prog_tests/lru_bug.c
new file mode 100644
index 000000000000..3c7822390827
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/lru_bug.c
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+#include "lru_bug.skel.h"
+
+void test_lru_bug(void)
+{
+	struct lru_bug *skel;
+	int ret;
+
+	skel = lru_bug__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "lru_bug__open_and_load"))
+		return;
+	ret = lru_bug__attach(skel);
+	if (!ASSERT_OK(ret, "lru_bug__attach"))
+		goto end;
+	usleep(1);
+	ASSERT_OK(skel->data->result, "prealloc_lru_pop doesn't call check_and_init_map_value");
+end:
+	lru_bug__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/lru_bug.c b/tools/testing/selftests/bpf/progs/lru_bug.c
new file mode 100644
index 000000000000..687081a724b3
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/lru_bug.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <vmlinux.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_helpers.h>
+
+struct map_value {
+	struct task_struct __kptr *ptr;
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_LRU_HASH);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, struct map_value);
+} lru_map SEC(".maps");
+
+int pid = 0;
+int result = 1;
+
+SEC("fentry/bpf_ktime_get_ns")
+int printk(void *ctx)
+{
+	struct map_value v = {};
+
+	if (pid == bpf_get_current_task_btf()->pid)
+		bpf_map_update_elem(&lru_map, &(int){0}, &v, 0);
+	return 0;
+}
+
+SEC("fentry/do_nanosleep")
+int nanosleep(void *ctx)
+{
+	struct map_value val = {}, *v;
+	struct task_struct *current;
+
+	bpf_map_update_elem(&lru_map, &(int){0}, &val, 0);
+	v = bpf_map_lookup_elem(&lru_map, &(int){0});
+	if (!v)
+		return 0;
+	bpf_map_delete_elem(&lru_map, &(int){0});
+	current = bpf_get_current_task_btf();
+	v->ptr = current;
+	pid = current->pid;
+	bpf_ktime_get_ns();
+	result = !v->ptr;
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf v3 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Martin KaFai Lau]

On Tue, Aug 09, 2022 at 11:30:32PM +0200, Kumar Kartikeya Dwivedi wrote:
> The LRU map that is preallocated may have its elements reused while
> another program holds a pointer to it from bpf_map_lookup_elem. Hence,
> only check_and_free_fields is appropriate when the element is being
> deleted, as it ensures proper synchronization against concurrent access
> of the map value. After that, we cannot call check_and_init_map_value
> again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while
> they can be concurrently accessed from a BPF program.
> 
> This is safe to do as when the map entry is deleted, concurrent access
> is protected against by check_and_free_fields, i.e. an existing timer
> would be freed, and any existing kptr will be released by it. The
> program can create further timers and kptrs after check_and_free_fields,
> but they will eventually be released once the preallocated items are
> freed on map destruction, even if the item is never reused again. Hence,
> the deleted item sitting in the free list can still have resources
> attached to it, and they would never leak.
> 
> With spin_lock, we never touch the field at all on delete or update, as
> we may end up modifying the state of the lock. Since the verifier
> ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock
> call, the program will eventually release the lock so that on reuse the
> new user of the value can take the lock.
The bpf_spin_lock's verifier description makes sense.  Note that
the lru map does not support spin lock for now.

> 
> Essentially, for the preallocated case, we must assume that the map
> value may always be in use by the program, even when it is sitting in
> the freelist, and handle things accordingly, i.e. use proper
> synchronization inside check_and_free_fields, and never reinitialize the
> special fields when it is reused on update.
Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf v3 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Alexei Starovoitov]

On Tue, Aug 9, 2022 at 3:29 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Tue, Aug 09, 2022 at 11:30:32PM +0200, Kumar Kartikeya Dwivedi wrote:
> > The LRU map that is preallocated may have its elements reused while
> > another program holds a pointer to it from bpf_map_lookup_elem. Hence,
> > only check_and_free_fields is appropriate when the element is being
> > deleted, as it ensures proper synchronization against concurrent access
> > of the map value. After that, we cannot call check_and_init_map_value
> > again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while
> > they can be concurrently accessed from a BPF program.
> >
> > This is safe to do as when the map entry is deleted, concurrent access
> > is protected against by check_and_free_fields, i.e. an existing timer
> > would be freed, and any existing kptr will be released by it. The
> > program can create further timers and kptrs after check_and_free_fields,
> > but they will eventually be released once the preallocated items are
> > freed on map destruction, even if the item is never reused again. Hence,
> > the deleted item sitting in the free list can still have resources
> > attached to it, and they would never leak.
> >
> > With spin_lock, we never touch the field at all on delete or update, as
> > we may end up modifying the state of the lock. Since the verifier
> > ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock
> > call, the program will eventually release the lock so that on reuse the
> > new user of the value can take the lock.
> The bpf_spin_lock's verifier description makes sense.  Note that
> the lru map does not support spin lock for now.

ahh. then it's not a bpf tree material.
It's a minor cleanup for bpf-next?

> >
> > Essentially, for the preallocated case, we must assume that the map
> > value may always be in use by the program, even when it is sitting in
> > the freelist, and handle things accordingly, i.e. use proper
> > synchronization inside check_and_free_fields, and never reinitialize the
> > special fields when it is reused on update.
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf v3 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Kumar Kartikeya Dwivedi]

On Wed, 10 Aug 2022 at 02:50, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Aug 9, 2022 at 3:29 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Tue, Aug 09, 2022 at 11:30:32PM +0200, Kumar Kartikeya Dwivedi wrote:
> > > The LRU map that is preallocated may have its elements reused while
> > > another program holds a pointer to it from bpf_map_lookup_elem. Hence,
> > > only check_and_free_fields is appropriate when the element is being
> > > deleted, as it ensures proper synchronization against concurrent access
> > > of the map value. After that, we cannot call check_and_init_map_value
> > > again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while
> > > they can be concurrently accessed from a BPF program.
> > >
> > > This is safe to do as when the map entry is deleted, concurrent access
> > > is protected against by check_and_free_fields, i.e. an existing timer
> > > would be freed, and any existing kptr will be released by it. The
> > > program can create further timers and kptrs after check_and_free_fields,
> > > but they will eventually be released once the preallocated items are
> > > freed on map destruction, even if the item is never reused again. Hence,
> > > the deleted item sitting in the free list can still have resources
> > > attached to it, and they would never leak.
> > >
> > > With spin_lock, we never touch the field at all on delete or update, as
> > > we may end up modifying the state of the lock. Since the verifier
> > > ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock
> > > call, the program will eventually release the lock so that on reuse the
> > > new user of the value can take the lock.
> > The bpf_spin_lock's verifier description makes sense.  Note that
> > the lru map does not support spin lock for now.
>
> ahh. then it's not a bpf tree material.
> It's a minor cleanup for bpf-next?
>

I was just describing what we do for each of the three types in the
commit log. It still affects timers and kptrs, which lru map supports.

> > >
> > > Essentially, for the preallocated case, we must assume that the map
> > > value may always be in use by the program, even when it is sitting in
> > > the freelist, and handle things accordingly, i.e. use proper
> > > synchronization inside check_and_free_fields, and never reinitialize the
> > > special fields when it is reused on update.
> > Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [PATCH bpf v3 2/3] bpf: Don't reinit map value in prealloc_lru_pop

[Alexei Starovoitov]

On Tue, Aug 9, 2022 at 5:52 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Wed, 10 Aug 2022 at 02:50, Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Aug 9, 2022 at 3:29 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > On Tue, Aug 09, 2022 at 11:30:32PM +0200, Kumar Kartikeya Dwivedi wrote:
> > > > The LRU map that is preallocated may have its elements reused while
> > > > another program holds a pointer to it from bpf_map_lookup_elem. Hence,
> > > > only check_and_free_fields is appropriate when the element is being
> > > > deleted, as it ensures proper synchronization against concurrent access
> > > > of the map value. After that, we cannot call check_and_init_map_value
> > > > again as it may rewrite bpf_spin_lock, bpf_timer, and kptr fields while
> > > > they can be concurrently accessed from a BPF program.
> > > >
> > > > This is safe to do as when the map entry is deleted, concurrent access
> > > > is protected against by check_and_free_fields, i.e. an existing timer
> > > > would be freed, and any existing kptr will be released by it. The
> > > > program can create further timers and kptrs after check_and_free_fields,
> > > > but they will eventually be released once the preallocated items are
> > > > freed on map destruction, even if the item is never reused again. Hence,
> > > > the deleted item sitting in the free list can still have resources
> > > > attached to it, and they would never leak.
> > > >
> > > > With spin_lock, we never touch the field at all on delete or update, as
> > > > we may end up modifying the state of the lock. Since the verifier
> > > > ensures that a bpf_spin_lock call is always paired with bpf_spin_unlock
> > > > call, the program will eventually release the lock so that on reuse the
> > > > new user of the value can take the lock.
> > > The bpf_spin_lock's verifier description makes sense.  Note that
> > > the lru map does not support spin lock for now.
> >
> > ahh. then it's not a bpf tree material.
> > It's a minor cleanup for bpf-next?
> >
>
> I was just describing what we do for each of the three types in the
> commit log. It still affects timers and kptrs, which lru map supports.

got it.
The uaf-ing cpu can use kptr field of deleted elem
and things go bad it lru_pop-ing cpu will clear it.

Re: [PATCH bpf v3 0/3] Don't reinit map value in prealloc_lru_pop

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Alexei Starovoitov <ast@kernel.org>:

On Tue,  9 Aug 2022 23:30:30 +0200 you wrote:
> Fix for a bug in prealloc_lru_pop spotted while reading the code, then a test +
> example that checks whether it is fixed.
> 
> Changelog:
> ----------
> v2 -> v3:
> v2: https://lore.kernel.org/bpf/20220809140615.21231-1-memxor@gmail.com
> 
> [...]

Here is the summary with links:
  - [bpf,v3,1/3] bpf: Allow calling bpf_prog_test kfuncs in tracing programs
    https://git.kernel.org/bpf/bpf/c/1f0752628e76
  - [bpf,v3,2/3] bpf: Don't reinit map value in prealloc_lru_pop
    https://git.kernel.org/bpf/bpf/c/275c30bcee66
  - [bpf,v3,3/3] selftests/bpf: Add test for prealloc_lru_pop bug
    https://git.kernel.org/bpf/bpf/c/de7b9927105b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html