On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t
>
> Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg")
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> ---
Acked-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
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.
From a0419047c148d2e1b36764a5a7ca2d90923044f1 Mon Sep 17 00:00:00 2001
From: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
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@xxxxxxxxx>
---
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
From 17838425c284cabbaa95df3305af76dd2b938751 Mon Sep 17 00:00:00 2001
From: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
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@xxxxxxxxx>
---
.../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
