[PATCH bpf 0/3] Block deletes from sockmap for tracing programs

[PATCH bpf 0/3] Block deletes from sockmap for tracing programs

[Jakub Sitnicki]

We have seen a few syzkaller reports of locking violations triggered by
map_delete from sockmap/sockhash from an unexpected code path, for instance
when irqs were disabled, or during a kfree inside a map_update.

The consensus is [1] to block map_delete op in the verifier for programs
which are not allowed to update sockmap/sockhash already today, instead of
trying to make sockmap deletes lock-safe in every possible context.

[1] https://lore.kernel.org/r/87a5kfwe8l.fsf@cloudflare.com

---
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Hillf Danton <hdanton@sina.com>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

---
Jakub Sitnicki (3):
      bpf: Allow delete from sockmap/sockhash only if update is allowed
      Revert "bpf, sockmap: Prevent lock inversion deadlock in map delete elem"
      selftests/bpf: Cover verifier checks for mutating sockmap/sockhash

 kernel/bpf/verifier.c                              |  10 +-
 net/core/sock_map.c                                |   6 -
 tools/testing/selftests/bpf/prog_tests/verifier.c  |   2 +
 .../selftests/bpf/progs/verifier_sockmap_mutate.c  | 187 +++++++++++++++++++++
 4 files changed, 196 insertions(+), 9 deletions(-)

[PATCH bpf 1/3] bpf: Allow delete from sockmap/sockhash only if update is allowed

[Jakub Sitnicki]

We have seen an influx of syzkaller reports where a BPF program attached to
a tracepoint triggers a locking rule violation by performing a map_delete
on a sockmap/sockhash.

We don't intend to support this artificial use scenario. Extend the
existing verifier allowed-program-type check for updating sockmap/sockhash
to also cover deleting from a map.

From now on only BPF programs which were previously allowed to update
sockmap/sockhash can delete from these map types.

Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Reported-and-tested-by: syzbot+ec941d6e24f633a59172@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=ec941d6e24f633a59172
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 kernel/bpf/verifier.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 77da1f438bec..48f3a9acdef3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8882,7 +8882,8 @@ static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
 	enum bpf_attach_type eatype = env->prog->expected_attach_type;
 	enum bpf_prog_type type = resolve_prog_type(env->prog);
 
-	if (func_id != BPF_FUNC_map_update_elem)
+	if (func_id != BPF_FUNC_map_update_elem &&
+	    func_id != BPF_FUNC_map_delete_elem)
 		return false;
 
 	/* It's not possible to get access to a locked struct sock in these
@@ -8893,6 +8894,11 @@ static bool may_update_sockmap(struct bpf_verifier_env *env, int func_id)
 		if (eatype == BPF_TRACE_ITER)
 			return true;
 		break;
+	case BPF_PROG_TYPE_SOCK_OPS:
+		/* map_update allowed only via dedicated helpers with event type checks */
+		if (func_id == BPF_FUNC_map_delete_elem)
+			return true;
+		break;
 	case BPF_PROG_TYPE_SOCKET_FILTER:
 	case BPF_PROG_TYPE_SCHED_CLS:
 	case BPF_PROG_TYPE_SCHED_ACT:
@@ -8988,7 +8994,6 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
 	case BPF_MAP_TYPE_SOCKMAP:
 		if (func_id != BPF_FUNC_sk_redirect_map &&
 		    func_id != BPF_FUNC_sock_map_update &&
-		    func_id != BPF_FUNC_map_delete_elem &&
 		    func_id != BPF_FUNC_msg_redirect_map &&
 		    func_id != BPF_FUNC_sk_select_reuseport &&
 		    func_id != BPF_FUNC_map_lookup_elem &&
@@ -8998,7 +9003,6 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env,
 	case BPF_MAP_TYPE_SOCKHASH:
 		if (func_id != BPF_FUNC_sk_redirect_hash &&
 		    func_id != BPF_FUNC_sock_hash_update &&
-		    func_id != BPF_FUNC_map_delete_elem &&
 		    func_id != BPF_FUNC_msg_redirect_hash &&
 		    func_id != BPF_FUNC_sk_select_reuseport &&
 		    func_id != BPF_FUNC_map_lookup_elem &&

-- 
2.40.1

RE: [PATCH bpf 1/3] bpf: Allow delete from sockmap/sockhash only if update is allowed

[John Fastabend]

Jakub Sitnicki wrote:
> We have seen an influx of syzkaller reports where a BPF program attached to
> a tracepoint triggers a locking rule violation by performing a map_delete
> on a sockmap/sockhash.
> 
> We don't intend to support this artificial use scenario. Extend the
> existing verifier allowed-program-type check for updating sockmap/sockhash
> to also cover deleting from a map.
> 
> From now on only BPF programs which were previously allowed to update
> sockmap/sockhash can delete from these map types.
> 
> Reported-by: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
> Reported-and-tested-by: syzbot+ec941d6e24f633a59172@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=ec941d6e24f633a59172
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf 2/3] Revert "bpf, sockmap: Prevent lock inversion deadlock in map delete elem"

[Jakub Sitnicki]

This reverts commit ff91059932401894e6c86341915615c5eb0eca48.

This check is no longer needed. BPF programs attached to tracepoints are
now rejected by the verifier when they attempt to delete from a
sockmap/sockhash maps.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 net/core/sock_map.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/core/sock_map.c b/net/core/sock_map.c
index 9402889840bf..63c016b4c169 100644
--- a/net/core/sock_map.c
+++ b/net/core/sock_map.c
@@ -423,9 +423,6 @@ static int __sock_map_delete(struct bpf_stab *stab, struct sock *sk_test,
 	struct sock *sk;
 	int err = 0;
 
-	if (irqs_disabled())
-		return -EOPNOTSUPP; /* locks here are hardirq-unsafe */
-
 	spin_lock_bh(&stab->lock);
 	sk = *psk;
 	if (!sk_test || sk_test == sk)
@@ -948,9 +945,6 @@ static long sock_hash_delete_elem(struct bpf_map *map, void *key)
 	struct bpf_shtab_elem *elem;
 	int ret = -ENOENT;
 
-	if (irqs_disabled())
-		return -EOPNOTSUPP; /* locks here are hardirq-unsafe */
-
 	hash = sock_hash_bucket_hash(key, key_size);
 	bucket = sock_hash_select_bucket(htab, hash);
 

-- 
2.40.1

RE: [PATCH bpf 2/3] Revert "bpf, sockmap: Prevent lock inversion deadlock in map delete elem"

[John Fastabend]

Jakub Sitnicki wrote:
> This reverts commit ff91059932401894e6c86341915615c5eb0eca48.
> 
> This check is no longer needed. BPF programs attached to tracepoints are
> now rejected by the verifier when they attempt to delete from a
> sockmap/sockhash maps.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---

Acked-by: John Fastabend <john.fastabend@gmail.com>

[PATCH bpf 3/3] selftests/bpf: Cover verifier checks for mutating sockmap/sockhash

[Jakub Sitnicki]

Verifier enforces that only certain program types can mutate sock{map,hash}
maps, that is update it or delete from it. Add test coverage for these
checks so we don't regress.

Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
 tools/testing/selftests/bpf/prog_tests/verifier.c  |   2 +
 .../selftests/bpf/progs/verifier_sockmap_mutate.c  | 187 +++++++++++++++++++++
 2 files changed, 189 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c
index c60db8beeb73..1c9c4ec1be11 100644
--- a/tools/testing/selftests/bpf/prog_tests/verifier.c
+++ b/tools/testing/selftests/bpf/prog_tests/verifier.c
@@ -67,6 +67,7 @@
 #include "verifier_search_pruning.skel.h"
 #include "verifier_sock.skel.h"
 #include "verifier_sock_addr.skel.h"
+#include "verifier_sockmap_mutate.skel.h"
 #include "verifier_spill_fill.skel.h"
 #include "verifier_spin_lock.skel.h"
 #include "verifier_stack_ptr.skel.h"
@@ -183,6 +184,7 @@ void test_verifier_sdiv(void)                 { RUN(verifier_sdiv); }
 void test_verifier_search_pruning(void)       { RUN(verifier_search_pruning); }
 void test_verifier_sock(void)                 { RUN(verifier_sock); }
 void test_verifier_sock_addr(void)            { RUN(verifier_sock_addr); }
+void test_verifier_sockmap_mutate(void)       { RUN(verifier_sockmap_mutate); }
 void test_verifier_spill_fill(void)           { RUN(verifier_spill_fill); }
 void test_verifier_spin_lock(void)            { RUN(verifier_spin_lock); }
 void test_verifier_stack_ptr(void)            { RUN(verifier_stack_ptr); }
diff --git a/tools/testing/selftests/bpf/progs/verifier_sockmap_mutate.c b/tools/testing/selftests/bpf/progs/verifier_sockmap_mutate.c
new file mode 100644
index 000000000000..fe4b123187b8
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/verifier_sockmap_mutate.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+#include "bpf_misc.h"
+
+#define __always_unused __attribute__((unused))
+
+char _license[] SEC("license") = "GPL";
+
+struct sock {
+} __attribute__((preserve_access_index));
+
+struct bpf_iter__sockmap {
+	union {
+		struct sock *sk;
+	};
+} __attribute__((preserve_access_index));
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKHASH);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, int);
+} sockhash SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_SOCKMAP);
+	__uint(max_entries, 1);
+	__type(key, int);
+	__type(value, int);
+} sockmap SEC(".maps");
+
+enum { CG_OK = 1 };
+
+int zero = 0;
+
+static __always_inline void test_sockmap_delete(void)
+{
+	bpf_map_delete_elem(&sockmap, &zero);
+	bpf_map_delete_elem(&sockhash, &zero);
+}
+
+static __always_inline void test_sockmap_update(void *sk)
+{
+	if (sk) {
+		bpf_map_update_elem(&sockmap, &zero, sk, BPF_ANY);
+		bpf_map_update_elem(&sockhash, &zero, sk, BPF_ANY);
+	}
+}
+
+static __always_inline void test_sockmap_lookup_and_update(void)
+{
+	struct bpf_sock *sk = bpf_map_lookup_elem(&sockmap, &zero);
+
+	if (sk) {
+		test_sockmap_update(sk);
+		bpf_sk_release(sk);
+	}
+}
+
+static __always_inline void test_sockmap_mutate(void *sk)
+{
+	test_sockmap_delete();
+	test_sockmap_update(sk);
+}
+
+static __always_inline void test_sockmap_lookup_and_mutate(void)
+{
+	test_sockmap_delete();
+	test_sockmap_lookup_and_update();
+}
+
+SEC("action")
+__success
+int test_sched_act(struct __sk_buff *skb)
+{
+	test_sockmap_mutate(skb->sk);
+	return 0;
+}
+
+SEC("classifier")
+__success
+int test_sched_cls(struct __sk_buff *skb)
+{
+	test_sockmap_mutate(skb->sk);
+	return 0;
+}
+
+SEC("flow_dissector")
+__success
+int test_flow_dissector_delete(struct __sk_buff *skb __always_unused)
+{
+	test_sockmap_delete();
+	return 0;
+}
+
+SEC("flow_dissector")
+__failure __msg("program of this type cannot use helper bpf_sk_release")
+int test_flow_dissector_update(struct __sk_buff *skb __always_unused)
+{
+	test_sockmap_lookup_and_update(); /* no access to skb->sk */
+	return 0;
+}
+
+SEC("iter/sockmap")
+__success
+int test_trace_iter(struct bpf_iter__sockmap *ctx)
+{
+	test_sockmap_mutate(ctx->sk);
+	return 0;
+}
+
+SEC("raw_tp/kfree")
+__failure __msg("cannot update sockmap in this context")
+int test_raw_tp_delete(const void *ctx __always_unused)
+{
+	test_sockmap_delete();
+	return 0;
+}
+
+SEC("raw_tp/kfree")
+__failure __msg("cannot update sockmap in this context")
+int test_raw_tp_update(const void *ctx __always_unused)
+{
+	test_sockmap_lookup_and_update();
+	return 0;
+}
+
+SEC("sk_lookup")
+__success
+int test_sk_lookup(struct bpf_sk_lookup *ctx)
+{
+	test_sockmap_mutate(ctx->sk);
+	return 0;
+}
+
+SEC("sk_reuseport")
+__success
+int test_sk_reuseport(struct sk_reuseport_md *ctx)
+{
+	test_sockmap_mutate(ctx->sk);
+	return 0;
+}
+
+SEC("socket")
+__success
+int test_socket_filter(struct __sk_buff *skb)
+{
+	test_sockmap_mutate(skb->sk);
+	return 0;
+}
+
+SEC("sockops")
+__success
+int test_sockops_delete(struct bpf_sock_ops *ctx __always_unused)
+{
+	test_sockmap_delete();
+	return CG_OK;
+}
+
+SEC("sockops")
+__failure __msg("cannot update sockmap in this context")
+int test_sockops_update(struct bpf_sock_ops *ctx)
+{
+	test_sockmap_update(ctx->sk);
+	return CG_OK;
+}
+
+SEC("sockops")
+__success
+int test_sockops_update_dedicated(struct bpf_sock_ops *ctx)
+{
+	bpf_sock_map_update(ctx, &sockmap, &zero, BPF_ANY);
+	bpf_sock_hash_update(ctx, &sockhash, &zero, BPF_ANY);
+	return CG_OK;
+}
+
+SEC("xdp")
+__success
+int test_xdp(struct xdp_md *ctx __always_unused)
+{
+	test_sockmap_lookup_and_mutate();
+	return XDP_PASS;
+}

-- 
2.40.1

RE: [PATCH bpf 3/3] selftests/bpf: Cover verifier checks for mutating sockmap/sockhash

[John Fastabend]

Jakub Sitnicki wrote:
> Verifier enforces that only certain program types can mutate sock{map,hash}
> maps, that is update it or delete from it. Add test coverage for these
> checks so we don't regress.
> 
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [PATCH bpf 0/3] Block deletes from sockmap for tracing programs

[John Fastabend]

Jakub Sitnicki wrote:
> We have seen a few syzkaller reports of locking violations triggered by
> map_delete from sockmap/sockhash from an unexpected code path, for instance
> when irqs were disabled, or during a kfree inside a map_update.
> 
> The consensus is [1] to block map_delete op in the verifier for programs
> which are not allowed to update sockmap/sockhash already today, instead of
> trying to make sockmap deletes lock-safe in every possible context.

+1 thanks Jakub. This makes sense to me I've never found a use case for
deleting socks from a tracing program.

Re: [PATCH bpf 0/3] Block deletes from sockmap for tracing programs

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Mon, 27 May 2024 13:20:06 +0200 you wrote:
> We have seen a few syzkaller reports of locking violations triggered by
> map_delete from sockmap/sockhash from an unexpected code path, for instance
> when irqs were disabled, or during a kfree inside a map_update.
> 
> The consensus is [1] to block map_delete op in the verifier for programs
> which are not allowed to update sockmap/sockhash already today, instead of
> trying to make sockmap deletes lock-safe in every possible context.
> 
> [...]

Here is the summary with links:
  - [bpf,1/3] bpf: Allow delete from sockmap/sockhash only if update is allowed
    https://git.kernel.org/bpf/bpf/c/98e948fb60d4
  - [bpf,2/3] Revert "bpf, sockmap: Prevent lock inversion deadlock in map delete elem"
    https://git.kernel.org/bpf/bpf/c/3b9ce0491a43
  - [bpf,3/3] selftests/bpf: Cover verifier checks for mutating sockmap/sockhash
    https://git.kernel.org/bpf/bpf/c/a63bf556160f

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