[PATCH bpf-next v1 0/2] Fix for check_func_arg_reg_off

[PATCH bpf-next v1 0/2] Fix for check_func_arg_reg_off

[Kumar Kartikeya Dwivedi]

Remove a leftover hunk in check_func_arg_reg_off that incorrectly
bypasses reg->off == 0 requirement for release kfuncs and helpers.

Kumar Kartikeya Dwivedi (2):
  bpf: Fix check_func_arg_reg_off bug for graph root/node
  selftests/bpf: Add test for bpf_obj_drop with bad reg->off

 kernel/bpf/verifier.c                         | 11 ----------
 .../bpf/progs/local_kptr_stash_fail.c         | 20 +++++++++++++++++++
 2 files changed, 20 insertions(+), 11 deletions(-)


base-commit: fb30159426439bfe9a1435c0555f67201198988c
-- 
2.41.0

[PATCH bpf-next v1 1/2] bpf: Fix check_func_arg_reg_off bug for graph root/node

[Kumar Kartikeya Dwivedi]

The commit being fixed introduced a hunk into check_func_arg_reg_off
that bypasses reg->off == 0 enforcement when offset points to a graph
node or root. This might possibly be done for treating bpf_rbtree_remove
and others as KF_RELEASE and then later check correct reg->off in helper
argument checks.

But this is not the case, those helpers are already not KF_RELEASE and
permit non-zero reg->off and verify it later to match the subobject in
BTF type.

However, this logic leads to bpf_obj_drop permitting free of register
arguments with non-zero offset when they point to a graph root or node
within them, which is not ok.

For instance:

struct foo {
	int i;
	int j;
	struct bpf_rb_node node;
};

struct foo *f = bpf_obj_new(typeof(*f));
if (!f) ...
bpf_obj_drop(f); // OK
bpf_obj_drop(&f->i); // still ok from verifier PoV
bpf_obj_drop(&f->node); // Not OK, but permitted right now

Fix this by dropping the whole part of code altogether.

Fixes: 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref semantics")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 kernel/bpf/verifier.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3a91bfd7b9cc..3d51c737a034 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7973,17 +7973,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env,
 		if (arg_type_is_dynptr(arg_type) && type == PTR_TO_STACK)
 			return 0;
 
-		if ((type_is_ptr_alloc_obj(type) || type_is_non_owning_ref(type)) && reg->off) {
-			if (reg_find_field_offset(reg, reg->off, BPF_GRAPH_NODE_OR_ROOT))
-				return __check_ptr_off_reg(env, reg, regno, true);
-
-			verbose(env, "R%d must have zero offset when passed to release func\n",
-				regno);
-			verbose(env, "No graph node or root found at R%d type:%s off:%d\n", regno,
-				btf_type_name(reg->btf, reg->btf_id), reg->off);
-			return -EINVAL;
-		}
-
 		/* Doing check_ptr_off_reg check for the offset will catch this
 		 * because fixed_off_ok is false, but checking here allows us
 		 * to give the user a better error message.
-- 
2.41.0

[PATCH bpf-next v1 2/2] selftests/bpf: Add test for bpf_obj_drop with bad reg->off

[Kumar Kartikeya Dwivedi]

Add a selftest for the fix provided in the previous commit. Without the
fix, the selftest passes the verifier while it should fail. The special
logic for detecting graph root or node for reg->off and bypassing
reg->off == 0 guarantee for release helpers/kfuncs has been dropped.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 .../bpf/progs/local_kptr_stash_fail.c         | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c b/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c
index 5484d1e9801d..fcf7a7567da2 100644
--- a/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c
+++ b/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c
@@ -62,4 +62,24 @@ long stash_rb_nodes(void *ctx)
 	return 0;
 }
 
+SEC("tc")
+__failure __msg("R1 must have zero offset when passed to release func")
+long drop_rb_node_off(void *ctx)
+{
+	struct map_value *mapval;
+	struct node_data *res;
+	int idx = 0;
+
+	mapval = bpf_map_lookup_elem(&some_nodes, &idx);
+	if (!mapval)
+		return 1;
+
+	res = bpf_obj_new(typeof(*res));
+	if (!res)
+		return 1;
+	/* Try releasing with graph node offset */
+	bpf_obj_drop(&res->node);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
-- 
2.41.0

Re: [PATCH bpf-next v1 0/2] Fix for check_func_arg_reg_off

[patchwork-bot+netdevbpf]

Hello:

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

On Tue, 22 Aug 2023 23:21:38 +0530 you wrote:
> Remove a leftover hunk in check_func_arg_reg_off that incorrectly
> bypasses reg->off == 0 requirement for release kfuncs and helpers.
> 
> Kumar Kartikeya Dwivedi (2):
>   bpf: Fix check_func_arg_reg_off bug for graph root/node
>   selftests/bpf: Add test for bpf_obj_drop with bad reg->off
> 
> [...]

Here is the summary with links:
  - [bpf-next,v1,1/2] bpf: Fix check_func_arg_reg_off bug for graph root/node
    https://git.kernel.org/bpf/bpf-next/c/6785b2edf48c
  - [bpf-next,v1,2/2] selftests/bpf: Add test for bpf_obj_drop with bad reg->off
    https://git.kernel.org/bpf/bpf-next/c/fbc5bc4c8e6c

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