[PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Yonghong Song]

When reviewing local percpu kptr support, Alexei discovered a bug
wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
locally allocated obj type do not match ([1]). Missed struct btf_id
comparison is the reason for the bug. This patch added such struct btf_id
comparison and will flag verification failure if types do not match.

  [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t

Reported-by: Alexei Starovoitov <ast@kernel.org>
Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/verifier.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 02a021c524ab..4e1ecd4b8497 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
 			verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
 			return -EFAULT;
 		}
-		/* Handled by helper specific checks */
+		if (meta->func_id == BPF_FUNC_kptr_xchg) {
+			struct btf_field *kptr_field = meta->kptr_field;
+
+			if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
+						  kptr_field->kptr.btf, kptr_field->kptr.btf_id,
+						  true)) {
+				verbose(env, "R%d is of type %s but %s is expected\n",
+					regno, btf_type_name(reg->btf, reg->btf_id),
+					btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
+				return -EACCES;
+			}
+		}
 		break;
 	case PTR_TO_BTF_ID | MEM_PERCPU:
 	case PTR_TO_BTF_ID | MEM_PERCPU | PTR_TRUSTED:
-- 
2.34.1

[PATCH bpf 2/2] selftests/bpf: Add a failure test for bpf_kptr_xchg() with local kptr

[Yonghong Song]

For a bpf_kptr_xchg() with local kptr, if the map value kptr type and
allocated local obj type does not match, with the previous patch,
the below verifier error message will be logged:
  R2 is of type <allocated local obj type> but <map value kptr type> is expected

Without the previous patch, the test will have unexpected success.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 .../bpf/prog_tests/local_kptr_stash.c         | 10 ++-
 .../bpf/progs/local_kptr_stash_fail.c         | 65 +++++++++++++++++++
 2 files changed, 74 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c

diff --git a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c
index 76f1da877f81..158616c94658 100644
--- a/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c
+++ b/tools/testing/selftests/bpf/prog_tests/local_kptr_stash.c
@@ -5,6 +5,7 @@
 #include <network_helpers.h>
 
 #include "local_kptr_stash.skel.h"
+#include "local_kptr_stash_fail.skel.h"
 static void test_local_kptr_stash_simple(void)
 {
 	LIBBPF_OPTS(bpf_test_run_opts, opts,
@@ -51,10 +52,17 @@ static void test_local_kptr_stash_unstash(void)
 	local_kptr_stash__destroy(skel);
 }
 
-void test_local_kptr_stash_success(void)
+static void test_local_kptr_stash_fail(void)
+{
+	RUN_TESTS(local_kptr_stash_fail);
+}
+
+void test_local_kptr_stash(void)
 {
 	if (test__start_subtest("local_kptr_stash_simple"))
 		test_local_kptr_stash_simple();
 	if (test__start_subtest("local_kptr_stash_unstash"))
 		test_local_kptr_stash_unstash();
+	if (test__start_subtest("local_kptr_stash_fail"))
+		test_local_kptr_stash_fail();
 }
diff --git a/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c b/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c
new file mode 100644
index 000000000000..ebb5f0208b41
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/local_kptr_stash_fail.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <bpf/bpf_core_read.h>
+#include "../bpf_experimental.h"
+#include "bpf_misc.h"
+
+struct node_data {
+	long key;
+	long data;
+	struct bpf_rb_node node;
+};
+
+struct map_value {
+	struct node_data __kptr *node;
+};
+
+struct node_data2 {
+	long key[4];
+};
+
+/* This is necessary so that LLVM generates BTF for node_data struct
+ * If it's not included, a fwd reference for node_data will be generated but
+ * no struct. Example BTF of "node" field in map_value when not included:
+ *
+ * [10] PTR '(anon)' type_id=35
+ * [34] FWD 'node_data' fwd_kind=struct
+ * [35] TYPE_TAG 'kptr_ref' type_id=34
+ */
+struct node_data *just_here_because_btf_bug;
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, struct map_value);
+	__uint(max_entries, 2);
+} some_nodes SEC(".maps");
+
+SEC("tc")
+__failure __msg("R2 is of type node_data2 but node_data is expected")
+long stash_rb_nodes(void *ctx)
+{
+	struct map_value *mapval;
+	struct node_data2 *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;
+	res->key[0] = 40;
+
+	res = bpf_kptr_xchg(&mapval->node, res);
+	if (res)
+		bpf_obj_drop(res);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
-- 
2.34.1

Re: [PATCH bpf 2/2] selftests/bpf: Add a failure test for bpf_kptr_xchg() with local kptr

[Kumar Kartikeya Dwivedi]

On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
>
> For a bpf_kptr_xchg() with local kptr, if the map value kptr type and
> allocated local obj type does not match, with the previous patch,
> the below verifier error message will be logged:
>   R2 is of type <allocated local obj type> but <map value kptr type> is expected
>
> Without the previous patch, the test will have unexpected success.
>
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Kumar Kartikeya Dwivedi]

[-- Attachment #1: Type: text/plain, Size: 3751 bytes --]

On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
>
> When reviewing local percpu kptr support, Alexei discovered a bug
> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
> locally allocated obj type do not match ([1]). Missed struct btf_id
> comparison is the reason for the bug. This patch added such struct btf_id
> comparison and will flag verification failure if types do not match.
>
>   [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> ---

Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

But some comments below...

>  kernel/bpf/verifier.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 02a021c524ab..4e1ecd4b8497 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>                         verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
>                         return -EFAULT;
>                 }
> -               /* Handled by helper specific checks */
> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
> +                       struct btf_field *kptr_field = meta->kptr_field;
> +
> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
> +                                                 true)) {
> +                               verbose(env, "R%d is of type %s but %s is expected\n",
> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
> +                               return -EACCES;
> +                       }
> +               }

The fix on its own looks ok to me, but any reason you'd not like to
delegate to map_kptr_match_type?
Just to collect kptr related type matching logic in its own place.  It
doesn't matter too much though.

While looking at the code, I noticed one more problem.

I don't think the current code is enforcing that 'reg->off is zero'
assumption when releasing MEM_ALLOC types. We are only saved because
you passed strict=true which makes passing non-zero reg->off a noop
and returns false.
The hunk was added to check_func_arg_reg_off in
6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
semantics")
which bypasses in case type is MEM_ALLOC and offset points to some
graph root or node.

I am not sure why this check exists, IIUC rbtree release helpers are
not tagged KF_RELEASE, and no release helper can type match on them
either. Dave, can you confirm? I think it might be an accidental
leftover of some refactoring.

In fact, it seems bpf_obj_drop is already broken because reg->off
assumption is violated.
For node_data type:

bpf_obj_drop(&res->data);
returns
R1 must have zero offset when passed to release func
No graph node or root found at R1 type:node_data off:8

but bpf_obj_drop(&res->node);
passes verifier.

15: (bf) r1 = r0                      ;
R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0)
R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3
16: (b7) r2 = 0                       ; R2_w=0 refs=3
17: (85) call bpf_obj_drop_impl#74867      ;
safe

I have attached a tentative fix and selftest to this patch, let me
know what you think.

[-- Attachment #2: 0001-bpf-Fix-check_func_arg_reg_off-bug-for-graph-root-no.patch --]
[-- Type: text/x-patch, Size: 2371 bytes --]

From a0419047c148d2e1b36764a5a7ca2d90923044f1 Mon Sep 17 00:00:00 2001
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date: Mon, 21 Aug 2023 13:03:43 +0530
Subject: [PATCH 1/2] bpf: Fix check_func_arg_reg_off bug for graph root/node

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 7fa46e92fe01..c0616c8b676d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7979,17 +7979,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


[-- Attachment #3: 0002-selftests-bpf-Add-test-for-bpf_obj_drop-with-bad-reg.patch --]
[-- Type: text/x-patch, Size: 1530 bytes --]

From 17838425c284cabbaa95df3305af76dd2b938751 Mon Sep 17 00:00:00 2001
From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
Date: Mon, 21 Aug 2023 13:10:42 +0530
Subject: [PATCH 2/2] selftests/bpf: Add test for bpf_obj_drop with bad
 reg->off

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 ebb5f0208b41..3e7c4a03ed98 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 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Yonghong Song]

On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote:
> On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> When reviewing local percpu kptr support, Alexei discovered a bug
>> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
>> locally allocated obj type do not match ([1]). Missed struct btf_id
>> comparison is the reason for the bug. This patch added such struct btf_id
>> comparison and will flag verification failure if types do not match.
>>
>>    [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>>
>> Reported-by: Alexei Starovoitov <ast@kernel.org>
>> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
> 
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 
> But some comments below...
> 
>>   kernel/bpf/verifier.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 02a021c524ab..4e1ecd4b8497 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>>                          verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
>>                          return -EFAULT;
>>                  }
>> -               /* Handled by helper specific checks */
>> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
>> +                       struct btf_field *kptr_field = meta->kptr_field;
>> +
>> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
>> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
>> +                                                 true)) {
>> +                               verbose(env, "R%d is of type %s but %s is expected\n",
>> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
>> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
>> +                               return -EACCES;
>> +                       }
>> +               }
> 
> The fix on its own looks ok to me, but any reason you'd not like to
> delegate to map_kptr_match_type?
> Just to collect kptr related type matching logic in its own place.  It
> doesn't matter too much though.

 From comments from Alexei in
 
https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t

=====
The map_kptr_match_type() should have been used for kptrs pointing to 
kernel objects only.
But you're calling it for MEM_ALLOC object with prog's BTF...
=====

So looks like map_kptr_match_type() is for kptrs pointing to
kernel objects only. So that is why I didn't use it.

> 
> While looking at the code, I noticed one more problem.
> 
> I don't think the current code is enforcing that 'reg->off is zero'
> assumption when releasing MEM_ALLOC types. We are only saved because
> you passed strict=true which makes passing non-zero reg->off a noop
> and returns false.
> The hunk was added to check_func_arg_reg_off in
> 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
> semantics")
> which bypasses in case type is MEM_ALLOC and offset points to some
> graph root or node.
> 
> I am not sure why this check exists, IIUC rbtree release helpers are
> not tagged KF_RELEASE, and no release helper can type match on them
> either. Dave, can you confirm? I think it might be an accidental
> leftover of some refactoring.

I am sure that Dave will look into this problem. I will take
a look as well to be sure my local percpu kptr won't have
issues with what you just discovered. Thanks!

> 
> In fact, it seems bpf_obj_drop is already broken because reg->off
> assumption is violated.
> For node_data type:
> 
> bpf_obj_drop(&res->data);
> returns
> R1 must have zero offset when passed to release func
> No graph node or root found at R1 type:node_data off:8
> 
> but bpf_obj_drop(&res->node);
> passes verifier.
> 
> 15: (bf) r1 = r0                      ;
> R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0)
> R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3
> 16: (b7) r2 = 0                       ; R2_w=0 refs=3
> 17: (85) call bpf_obj_drop_impl#74867      ;
> safe
> 
> I have attached a tentative fix and selftest to this patch, let me
> know what you think.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Kumar Kartikeya Dwivedi]

On Mon, 21 Aug 2023 at 19:52, Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
>
> On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
> >>
> >> When reviewing local percpu kptr support, Alexei discovered a bug
> >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
> >> locally allocated obj type do not match ([1]). Missed struct btf_id
> >> comparison is the reason for the bug. This patch added such struct btf_id
> >> comparison and will flag verification failure if types do not match.
> >>
> >>    [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
> >>
> >> Reported-by: Alexei Starovoitov <ast@kernel.org>
> >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >> ---
> >
> > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> >
> > But some comments below...
> >
> >>   kernel/bpf/verifier.c | 13 ++++++++++++-
> >>   1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 02a021c524ab..4e1ecd4b8497 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >>                          verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
> >>                          return -EFAULT;
> >>                  }
> >> -               /* Handled by helper specific checks */
> >> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
> >> +                       struct btf_field *kptr_field = meta->kptr_field;
> >> +
> >> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> >> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
> >> +                                                 true)) {
> >> +                               verbose(env, "R%d is of type %s but %s is expected\n",
> >> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
> >> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
> >> +                               return -EACCES;
> >> +                       }
> >> +               }
> >
> > The fix on its own looks ok to me, but any reason you'd not like to
> > delegate to map_kptr_match_type?
> > Just to collect kptr related type matching logic in its own place.  It
> > doesn't matter too much though.
>
>  From comments from Alexei in
>
> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>
> =====
> The map_kptr_match_type() should have been used for kptrs pointing to
> kernel objects only.
> But you're calling it for MEM_ALLOC object with prog's BTF...
> =====
>
> So looks like map_kptr_match_type() is for kptrs pointing to
> kernel objects only. So that is why I didn't use it.
>

That function was added by me. Back then I added this check as we were
discussing possibly supporting such local kptr and more thought would
be needed about the design before just doing type matching. Also it
was using kernel_type_name which was later renamed as btf_type_name,
so as a precaution I added the btf_is_kernel check. Apart from that I
remember no other reason, so I think it should be ok to drop it now
and use it.

But as I said, it is up to you, it will in effect do the same thing as
this patch, so it is ok as-is.

> >
> > While looking at the code, I noticed one more problem.
> >
> > I don't think the current code is enforcing that 'reg->off is zero'
> > assumption when releasing MEM_ALLOC types. We are only saved because
> > you passed strict=true which makes passing non-zero reg->off a noop
> > and returns false.
> > The hunk was added to check_func_arg_reg_off in
> > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
> > semantics")
> > which bypasses in case type is MEM_ALLOC and offset points to some
> > graph root or node.
> >
> > I am not sure why this check exists, IIUC rbtree release helpers are
> > not tagged KF_RELEASE, and no release helper can type match on them
> > either. Dave, can you confirm? I think it might be an accidental
> > leftover of some refactoring.
>
> I am sure that Dave will look into this problem. I will take
> a look as well to be sure my local percpu kptr won't have
> issues with what you just discovered. Thanks!
>

It should be a problem for anything that has the MEM_ALLOC flag set,
including percpu_kptr.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Yonghong Song]

On 8/21/23 9:03 AM, Kumar Kartikeya Dwivedi wrote:
> On Mon, 21 Aug 2023 at 19:52, Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>>
>>
>> On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote:
>>> On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
>>>>
>>>> When reviewing local percpu kptr support, Alexei discovered a bug
>>>> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
>>>> locally allocated obj type do not match ([1]). Missed struct btf_id
>>>> comparison is the reason for the bug. This patch added such struct btf_id
>>>> comparison and will flag verification failure if types do not match.
>>>>
>>>>     [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>>>>
>>>> Reported-by: Alexei Starovoitov <ast@kernel.org>
>>>> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
>>>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>>>> ---
>>>
>>> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>>>
>>> But some comments below...
>>>
>>>>    kernel/bpf/verifier.c | 13 ++++++++++++-
>>>>    1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>>>> index 02a021c524ab..4e1ecd4b8497 100644
>>>> --- a/kernel/bpf/verifier.c
>>>> +++ b/kernel/bpf/verifier.c
>>>> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>>>>                           verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
>>>>                           return -EFAULT;
>>>>                   }
>>>> -               /* Handled by helper specific checks */
>>>> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
>>>> +                       struct btf_field *kptr_field = meta->kptr_field;
>>>> +
>>>> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
>>>> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
>>>> +                                                 true)) {
>>>> +                               verbose(env, "R%d is of type %s but %s is expected\n",
>>>> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
>>>> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
>>>> +                               return -EACCES;
>>>> +                       }
>>>> +               }
>>>
>>> The fix on its own looks ok to me, but any reason you'd not like to
>>> delegate to map_kptr_match_type?
>>> Just to collect kptr related type matching logic in its own place.  It
>>> doesn't matter too much though.
>>
>>   From comments from Alexei in
>>
>> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>>
>> =====
>> The map_kptr_match_type() should have been used for kptrs pointing to
>> kernel objects only.
>> But you're calling it for MEM_ALLOC object with prog's BTF...
>> =====
>>
>> So looks like map_kptr_match_type() is for kptrs pointing to
>> kernel objects only. So that is why I didn't use it.
>>
> 
> That function was added by me. Back then I added this check as we were
> discussing possibly supporting such local kptr and more thought would
> be needed about the design before just doing type matching. Also it
> was using kernel_type_name which was later renamed as btf_type_name,
> so as a precaution I added the btf_is_kernel check. Apart from that I
> remember no other reason, so I think it should be ok to drop it now
> and use it.
> 
> But as I said, it is up to you, it will in effect do the same thing as
> this patch, so it is ok as-is.
> 
>>>
>>> While looking at the code, I noticed one more problem.
>>>
>>> I don't think the current code is enforcing that 'reg->off is zero'
>>> assumption when releasing MEM_ALLOC types. We are only saved because
>>> you passed strict=true which makes passing non-zero reg->off a noop
>>> and returns false.
>>> The hunk was added to check_func_arg_reg_off in
>>> 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
>>> semantics")
>>> which bypasses in case type is MEM_ALLOC and offset points to some
>>> graph root or node.
>>>
>>> I am not sure why this check exists, IIUC rbtree release helpers are
>>> not tagged KF_RELEASE, and no release helper can type match on them
>>> either. Dave, can you confirm? I think it might be an accidental
>>> leftover of some refactoring.
>>
>> I am sure that Dave will look into this problem. I will take
>> a look as well to be sure my local percpu kptr won't have
>> issues with what you just discovered. Thanks!
>>
> 
> It should be a problem for anything that has the MEM_ALLOC flag set,
> including percpu_kptr.

I suspect that below removed code

```
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7fa46e92fe01..c0616c8b676d 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -7979,17 +7979,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.
```

intends to check whether there is a graph node or root in a local
kptr. This is to ensure Dave's use case where you can locally
allocate a obj and one of obj's field is a graph node/root.

I agree with you the checking probably should not be here and
it should be somewhere else.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Alexei Starovoitov]

On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi
<memxor@gmail.com> wrote:
> > >
> > > The fix on its own looks ok to me, but any reason you'd not like to
> > > delegate to map_kptr_match_type?
> > > Just to collect kptr related type matching logic in its own place.  It
> > > doesn't matter too much though.
> >
> >  From comments from Alexei in
> >
> > https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
> >
> > =====
> > The map_kptr_match_type() should have been used for kptrs pointing to
> > kernel objects only.
> > But you're calling it for MEM_ALLOC object with prog's BTF...
> > =====
> >
> > So looks like map_kptr_match_type() is for kptrs pointing to
> > kernel objects only. So that is why I didn't use it.
> >
>
> That function was added by me. Back then I added this check as we were
> discussing possibly supporting such local kptr and more thought would
> be needed about the design before just doing type matching. Also it
> was using kernel_type_name which was later renamed as btf_type_name,
> so as a precaution I added the btf_is_kernel check. Apart from that I
> remember no other reason, so I think it should be ok to drop it now
> and use it.

Agree with Kumar.
When I said map_kptr_match_type() is only used with kernel BTF I meant
that as code stands it was the intent of that helper.
With MEM_ALLOC being kptr_xchg-ed it's better to share the code and
map_kptr_match_type() should probably be adopted to work with both kernel
and prog's BTFs.

And as Kumar noticed __check_ptr_off_reg() part of it is necessary for
MEM_ALLOC too.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Yonghong Song]

On 8/21/23 12:44 PM, Alexei Starovoitov wrote:
> On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
>>>>
>>>> The fix on its own looks ok to me, but any reason you'd not like to
>>>> delegate to map_kptr_match_type?
>>>> Just to collect kptr related type matching logic in its own place.  It
>>>> doesn't matter too much though.
>>>
>>>   From comments from Alexei in
>>>
>>> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>>>
>>> =====
>>> The map_kptr_match_type() should have been used for kptrs pointing to
>>> kernel objects only.
>>> But you're calling it for MEM_ALLOC object with prog's BTF...
>>> =====
>>>
>>> So looks like map_kptr_match_type() is for kptrs pointing to
>>> kernel objects only. So that is why I didn't use it.
>>>
>>
>> That function was added by me. Back then I added this check as we were
>> discussing possibly supporting such local kptr and more thought would
>> be needed about the design before just doing type matching. Also it
>> was using kernel_type_name which was later renamed as btf_type_name,
>> so as a precaution I added the btf_is_kernel check. Apart from that I
>> remember no other reason, so I think it should be ok to drop it now
>> and use it.
> 
> Agree with Kumar.
> When I said map_kptr_match_type() is only used with kernel BTF I meant
> that as code stands it was the intent of that helper.
> With MEM_ALLOC being kptr_xchg-ed it's better to share the code and
> map_kptr_match_type() should probably be adopted to work with both kernel
> and prog's BTFs.

Sounds good to me. Will use map_kptr_match_type() in v2.

> 
> And as Kumar noticed __check_ptr_off_reg() part of it is necessary for
> MEM_ALLOC too.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Alexei Starovoitov]

On Mon, Aug 21, 2023 at 3:13 PM Yonghong Song <yonghong.song@linux.dev> wrote:
>
>
>
> On 8/21/23 12:44 PM, Alexei Starovoitov wrote:
> > On Mon, Aug 21, 2023 at 9:03 AM Kumar Kartikeya Dwivedi
> > <memxor@gmail.com> wrote:
> >>>>
> >>>> The fix on its own looks ok to me, but any reason you'd not like to
> >>>> delegate to map_kptr_match_type?
> >>>> Just to collect kptr related type matching logic in its own place.  It
> >>>> doesn't matter too much though.
> >>>
> >>>   From comments from Alexei in
> >>>
> >>> https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
> >>>
> >>> =====
> >>> The map_kptr_match_type() should have been used for kptrs pointing to
> >>> kernel objects only.
> >>> But you're calling it for MEM_ALLOC object with prog's BTF...
> >>> =====
> >>>
> >>> So looks like map_kptr_match_type() is for kptrs pointing to
> >>> kernel objects only. So that is why I didn't use it.
> >>>
> >>
> >> That function was added by me. Back then I added this check as we were
> >> discussing possibly supporting such local kptr and more thought would
> >> be needed about the design before just doing type matching. Also it
> >> was using kernel_type_name which was later renamed as btf_type_name,
> >> so as a precaution I added the btf_is_kernel check. Apart from that I
> >> remember no other reason, so I think it should be ok to drop it now
> >> and use it.
> >
> > Agree with Kumar.
> > When I said map_kptr_match_type() is only used with kernel BTF I meant
> > that as code stands it was the intent of that helper.
> > With MEM_ALLOC being kptr_xchg-ed it's better to share the code and
> > map_kptr_match_type() should probably be adopted to work with both kernel
> > and prog's BTFs.
>
> Sounds good to me. Will use map_kptr_match_type() in v2.

btw it's a bit risky for bpf tree this late into rc-s. Pls target bpf-next.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[David Marchevsky]

On 8/21/23 4:36 AM, Kumar Kartikeya Dwivedi wrote:
> On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
>>
>> When reviewing local percpu kptr support, Alexei discovered a bug
>> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
>> locally allocated obj type do not match ([1]). Missed struct btf_id
>> comparison is the reason for the bug. This patch added such struct btf_id
>> comparison and will flag verification failure if types do not match.
>>
>>   [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
>>
>> Reported-by: Alexei Starovoitov <ast@kernel.org>
>> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
>> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
>> ---
> 
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 
> But some comments below...
> 
>>  kernel/bpf/verifier.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 02a021c524ab..4e1ecd4b8497 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
>>                         verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
>>                         return -EFAULT;
>>                 }
>> -               /* Handled by helper specific checks */
>> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
>> +                       struct btf_field *kptr_field = meta->kptr_field;
>> +
>> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
>> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
>> +                                                 true)) {
>> +                               verbose(env, "R%d is of type %s but %s is expected\n",
>> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
>> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
>> +                               return -EACCES;
>> +                       }
>> +               }
> 
> The fix on its own looks ok to me, but any reason you'd not like to
> delegate to map_kptr_match_type?
> Just to collect kptr related type matching logic in its own place.  It
> doesn't matter too much though.
> 
> While looking at the code, I noticed one more problem.
> 
> I don't think the current code is enforcing that 'reg->off is zero'
> assumption when releasing MEM_ALLOC types. We are only saved because
> you passed strict=true which makes passing non-zero reg->off a noop
> and returns false.
> The hunk was added to check_func_arg_reg_off in
> 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
> semantics")
> which bypasses in case type is MEM_ALLOC and offset points to some
> graph root or node.
> 
> I am not sure why this check exists, IIUC rbtree release helpers are
> not tagged KF_RELEASE, and no release helper can type match on them
> either. Dave, can you confirm? I think it might be an accidental
> leftover of some refactoring.

I think you're correct, that's probably leftover from when
helpers were tagged KF_RELEASE in earlier rbtree impl revisions.

I also think it's reasonable to delete the hunk as you've done
in attached patches.

> 
> In fact, it seems bpf_obj_drop is already broken because reg->off
> assumption is violated.
> For node_data type:
> 
> bpf_obj_drop(&res->data);
> returns
> R1 must have zero offset when passed to release func
> No graph node or root found at R1 type:node_data off:8
> 
> but bpf_obj_drop(&res->node);
> passes verifier.
> 
> 15: (bf) r1 = r0                      ;
> R0_w=ptr_node_data(ref_obj_id=3,off=16,imm=0)
> R1_w=ptr_node_data(ref_obj_id=3,off=16,imm=0) refs=3
> 16: (b7) r2 = 0                       ; R2_w=0 refs=3
> 17: (85) call bpf_obj_drop_impl#74867      ;
> safe
> 
> I have attached a tentative fix and selftest to this patch, let me
> know what you think.

Re: [PATCH bpf 1/2] bpf: Fix a bpf_kptr_xchg() issue with local kptr

[Kumar Kartikeya Dwivedi]

On Tue, 22 Aug 2023 at 10:45, David Marchevsky
<david.marchevsky@linux.dev> wrote:
>
> On 8/21/23 4:36 AM, Kumar Kartikeya Dwivedi wrote:
> > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@linux.dev> wrote:
> >>
> >> When reviewing local percpu kptr support, Alexei discovered a bug
> >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and
> >> locally allocated obj type do not match ([1]). Missed struct btf_id
> >> comparison is the reason for the bug. This patch added such struct btf_id
> >> comparison and will flag verification failure if types do not match.
> >>
> >>   [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@macbook-pro-8.dhcp.thefacebook.com/#t
> >>
> >> Reported-by: Alexei Starovoitov <ast@kernel.org>
> >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
> >> ---
> >
> > Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> >
> > But some comments below...
> >
> >>  kernel/bpf/verifier.c | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> >> index 02a021c524ab..4e1ecd4b8497 100644
> >> --- a/kernel/bpf/verifier.c
> >> +++ b/kernel/bpf/verifier.c
> >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno,
> >>                         verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n");
> >>                         return -EFAULT;
> >>                 }
> >> -               /* Handled by helper specific checks */
> >> +               if (meta->func_id == BPF_FUNC_kptr_xchg) {
> >> +                       struct btf_field *kptr_field = meta->kptr_field;
> >> +
> >> +                       if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off,
> >> +                                                 kptr_field->kptr.btf, kptr_field->kptr.btf_id,
> >> +                                                 true)) {
> >> +                               verbose(env, "R%d is of type %s but %s is expected\n",
> >> +                                       regno, btf_type_name(reg->btf, reg->btf_id),
> >> +                                       btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id));
> >> +                               return -EACCES;
> >> +                       }
> >> +               }
> >
> > The fix on its own looks ok to me, but any reason you'd not like to
> > delegate to map_kptr_match_type?
> > Just to collect kptr related type matching logic in its own place.  It
> > doesn't matter too much though.
> >
> > While looking at the code, I noticed one more problem.
> >
> > I don't think the current code is enforcing that 'reg->off is zero'
> > assumption when releasing MEM_ALLOC types. We are only saved because
> > you passed strict=true which makes passing non-zero reg->off a noop
> > and returns false.
> > The hunk was added to check_func_arg_reg_off in
> > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref
> > semantics")
> > which bypasses in case type is MEM_ALLOC and offset points to some
> > graph root or node.
> >
> > I am not sure why this check exists, IIUC rbtree release helpers are
> > not tagged KF_RELEASE, and no release helper can type match on them
> > either. Dave, can you confirm? I think it might be an accidental
> > leftover of some refactoring.
>
> I think you're correct, that's probably leftover from when
> helpers were tagged KF_RELEASE in earlier rbtree impl revisions.
>
> I also think it's reasonable to delete the hunk as you've done
> in attached patches.
>

Ok, then I'll submit it as a patch after Yonghong's patch is accepted
(since both will conflict trying to add local_kptr_stash_fail test
cases) and you can give your Acked-by on it.