[PATCH bpf-next v3 0/2] bpf: Reject MEM_ALLOC BTF accesses past bounds

[PATCH bpf-next v3 0/2] bpf: Reject MEM_ALLOC BTF accesses past bounds

[Yiyang Chen]

BTF struct walks can relax the top-level struct-size check for trailing
flexible arrays. That relaxation must not let a PTR_TO_BTF_ID | MEM_ALLOC
access escape the bytes allocated by bpf_obj_new() or bpf_percpu_obj_new().

Patch 1 rejects MEM_ALLOC BTF walks whose access range reaches past the
current struct size before applying the flexible-array relaxation. This now
also applies to struct ID matching used by kfunc and kptr type checks.
Patch 2 adds a linked_list negative loader case for this path.

Changes in v3:
- Pass the flexible-array walk policy through btf_struct_ids_match() callers,
  so MEM_ALLOC kfunc/kptr type checks use the same bounds rule.
- Rename the btf_struct_walk() parameter to walk_flex_arrays.
- Rebase onto current bpf-next.

v2:
https://lore.kernel.org/bpf/cover.1782197377.git.chenyy23@mails.tsinghua.edu.cn/

v1:
https://lore.kernel.org/bpf/cover.1782100805.git.chenyy23@mails.tsinghua.edu.cn/

Yiyang Chen (2):
  bpf: Reject MEM_ALLOC BTF accesses past object bounds
  selftests/bpf: Cover MEM_ALLOC access past object bounds

 include/linux/bpf.h                           |  2 +-
 kernel/bpf/btf.c                              | 17 +++++++++-----
 kernel/bpf/verifier.c                         | 11 +++++----
 .../selftests/bpf/prog_tests/linked_list.c    |  1 +
 .../selftests/bpf/progs/linked_list_fail.c    | 23 +++++++++++++++++++
 5 files changed, 43 insertions(+), 11 deletions(-)


base-commit: 53435562a725962e4de0c29653223129ba11643a
-- 
2.34.1

[PATCH bpf-next v3 1/2] bpf: Reject MEM_ALLOC BTF accesses past object bounds

[Yiyang Chen]

BTF struct walks relax the struct-size check for accesses through a
trailing flexible array. That is valid for ordinary BTF type walking, but
PTR_TO_BTF_ID | MEM_ALLOC values point to objects allocated with the static
BTF type size.

When walking a MEM_ALLOC object, reject the access before applying the
flexible-array relaxation if the access range extends past the struct size.
Apply the same policy to struct ID matching so kfunc and kptr type checks
do not walk past the allocated object bounds either.

Fixes: 958cf2e273f0 ("bpf: Introduce bpf_obj_new")
Fixes: 36d8bdf75a93 ("bpf: Add alloc/xchg/direct_access support for local percpu kptr")
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
 include/linux/bpf.h   |  2 +-
 kernel/bpf/btf.c      | 17 +++++++++++------
 kernel/bpf/verifier.c | 11 +++++++----
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 7719f6528..314d606dc 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3146,7 +3146,7 @@ int btf_struct_access(struct bpf_verifier_log *log,
 bool btf_struct_ids_match(struct bpf_verifier_log *log,
 			  const struct btf *btf, u32 id, int off,
 			  const struct btf *need_btf, u32 need_type_id,
-			  bool strict);
+			  bool strict, bool walk_flex_arrays);
 
 int btf_distill_func_proto(struct bpf_verifier_log *log,
 			   struct btf *btf,
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 64572f85e..dff5c0d91 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -7108,7 +7108,7 @@ enum bpf_struct_walk_result {
 static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 			   const struct btf_type *t, int off, int size,
 			   u32 *next_btf_id, enum bpf_type_flag *flag,
-			   const char **field_name)
+			   const char **field_name, bool walk_flex_arrays)
 {
 	u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
 	const struct btf_type *mtype, *elem_type = NULL;
@@ -7135,11 +7135,14 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
 		*flag |= PTR_UNTRUSTED;
 
 	if (off + size > t->size) {
+		struct btf_array *array_elem;
+
+		if (!walk_flex_arrays)
+			goto error;
+
 		/* If the last element is a variable size array, we may
 		 * need to relax the rule.
 		 */
-		struct btf_array *array_elem;
-
 		if (vlen == 0)
 			goto error;
 
@@ -7404,7 +7407,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
 
 	t = btf_type_by_id(btf, id);
 	do {
-		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
+		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag,
+				      field_name, !type_is_alloc(reg->type));
 
 		switch (err) {
 		case WALK_PTR:
@@ -7463,7 +7467,7 @@ bool btf_types_are_same(const struct btf *btf1, u32 id1,
 bool btf_struct_ids_match(struct bpf_verifier_log *log,
 			  const struct btf *btf, u32 id, int off,
 			  const struct btf *need_btf, u32 need_type_id,
-			  bool strict)
+			  bool strict, bool walk_flex_arrays)
 {
 	const struct btf_type *type;
 	enum bpf_type_flag flag = 0;
@@ -7482,7 +7486,8 @@ bool btf_struct_ids_match(struct bpf_verifier_log *log,
 	type = btf_type_by_id(btf, id);
 	if (!type)
 		return false;
-	err = btf_struct_walk(log, btf, type, off, 1, &id, &flag, NULL);
+	err = btf_struct_walk(log, btf, type, off, 1, &id, &flag, NULL,
+			      walk_flex_arrays);
 	if (err != WALK_STRUCT)
 		return false;
 
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 25aea4271..4d3bb7156 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4379,7 +4379,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
 	 */
 	if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->var_off.value,
 				  kptr_field->kptr.btf, kptr_field->kptr.btf_id,
-				  kptr_field->type != BPF_KPTR_UNREF))
+				  kptr_field->type != BPF_KPTR_UNREF,
+				  !type_is_alloc(reg->type)))
 		goto bad_type;
 	return 0;
 bad_type:
@@ -7970,7 +7971,7 @@ static int check_reg_type(struct bpf_verifier_env *env, struct bpf_reg_state *re
 
 			if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id,
 						  reg->var_off.value, btf_vmlinux, *arg_btf_id,
-						  strict_type_match)) {
+						  strict_type_match, !type_is_alloc(reg->type))) {
 				verbose(env, "%s is of type %s but %s is expected\n",
 					reg_arg_name(env, argno),
 					btf_type_name(reg->btf, reg->btf_id),
@@ -11429,7 +11430,8 @@ static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
 	reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id, &reg_ref_id);
 	reg_ref_tname = btf_name_by_offset(reg_btf, reg_ref_t->name_off);
 	struct_same = btf_struct_ids_match(&env->log, reg_btf, reg_ref_id, reg->var_off.value,
-					   meta->btf, ref_id, strict_type_match);
+					   meta->btf, ref_id, strict_type_match,
+					   !type_is_alloc(reg->type));
 	/* If kfunc is accepting a projection type (ie. __sk_buff), it cannot
 	 * actually use it -- it must cast to the underlying type. So we allow
 	 * caller to pass in the underlying type.
@@ -11876,7 +11878,8 @@ __process_kf_arg_ptr_to_graph_node(struct bpf_verifier_env *env,
 	et = btf_type_by_id(field->graph_root.btf, field->graph_root.value_btf_id);
 	t = btf_type_by_id(reg->btf, reg->btf_id);
 	if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, 0, field->graph_root.btf,
-				  field->graph_root.value_btf_id, true)) {
+				  field->graph_root.value_btf_id, true,
+				  !type_is_alloc(reg->type))) {
 		verbose(env, "operation on %s expects arg#1 %s at offset=%d "
 			"in struct %s, but arg is at offset=%d in struct %s\n",
 			btf_field_type_name(head_field_type),
-- 
2.34.1

Re: [PATCH bpf-next v3 1/2] bpf: Reject MEM_ALLOC BTF accesses past object bounds

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for out-of-bounds flex-array access on locally allocated objects. An attacker can bypass the new `!type_is_alloc(reg->type)` check by stripping the `MEM_ALLOC` flag from the pointer, re-enabling the flex-array relaxation and allowing out-of-bounds kernel memory reads.
--

commit 58fdae86cafcb299054bb852079f413faa582d91
Author: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>

bpf: Reject MEM_ALLOC BTF accesses past object bounds

This patch aims to prevent out-of-bounds accesses past object bounds when
accessing objects with a trailing flexible array. It restricts the
flexible-array bounds relaxation when the register points to a locally
allocated object.

> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 64572f85edc83..dff5c0d916412 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
[ ... ]
> @@ -7404,7 +7407,8 @@ int btf_struct_access(struct bpf_verifier_log *log,
>  
>  	t = btf_type_by_id(btf, id);
>  	do {
> -		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag, field_name);
> +		err = btf_struct_walk(log, btf, t, off, size, &id, &tmp_flag,
> +				      field_name, !type_is_alloc(reg->type));

[Severity: High]
This is a pre-existing issue, but does this check completely prevent the
out-of-bounds flex-array access?

If a BPF program allocates a local object via bpf_obj_new(), the memory is
allocated strictly based on t->size with no extra room for a flexible array.

However, if the MEM_ALLOC flag is later stripped from the pointer, the check
!type_is_alloc(reg->type) will evaluate to true. This flag stripping can happen
if the pointer is cast using bpf_rdonly_cast(), or if it is stored as an
unreferenced kptr in a map and loaded outside an RCU critical section.

Could this allow a program to bypass this new fix, re-enable the flex-array
bounds relaxation in btf_struct_walk(), and perform an out-of-bounds read of
adjacent SLUB allocator objects using PROBE_MEM semantics?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782807039.git.chenyy23@mails.tsinghua.edu.cn?part=1

[PATCH bpf-next v3 2/2] selftests/bpf: Cover MEM_ALLOC access past object bounds

[Yiyang Chen]

Add a linked_list negative loader case for a program-BTF type whose last
member is a zero-length flexible array. The program writes through the
first flexible-array element of an object allocated by bpf_obj_new().

The verifier should reject the access when the BTF walk reaches beyond the
static size of the allocated object.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Signed-off-by: Yiyang Chen <chenyy23@mails.tsinghua.edu.cn>
---
 .../selftests/bpf/prog_tests/linked_list.c    |  1 +
 .../selftests/bpf/progs/linked_list_fail.c    | 23 +++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/linked_list.c b/tools/testing/selftests/bpf/prog_tests/linked_list.c
index 8defea025..c3d133c6a 100644
--- a/tools/testing/selftests/bpf/prog_tests/linked_list.c
+++ b/tools/testing/selftests/bpf/prog_tests/linked_list.c
@@ -68,6 +68,7 @@ static struct {
 	{ "obj_type_id_oor", "local type ID argument must be in range [0, U32_MAX]" },
 	{ "obj_new_no_composite", "bpf_obj_new/bpf_percpu_obj_new type ID argument must be of a struct" },
 	{ "obj_new_no_struct", "bpf_obj_new/bpf_percpu_obj_new type ID argument must be of a struct" },
+	{ "obj_new_flex_array", "access beyond struct obj_new_flex" },
 	{ "obj_drop_non_zero_off", "R1 must have zero offset when passed to release func" },
 	{ "new_null_ret", "R0 invalid mem access 'ptr_or_null_'" },
 	{ "obj_new_acq", "Unreleased reference id=" },
diff --git a/tools/testing/selftests/bpf/progs/linked_list_fail.c b/tools/testing/selftests/bpf/progs/linked_list_fail.c
index ddd26d1a0..031e77a28 100644
--- a/tools/testing/selftests/bpf/progs/linked_list_fail.c
+++ b/tools/testing/selftests/bpf/progs/linked_list_fail.c
@@ -167,6 +167,16 @@ CHECK_OP(push_back);
 #undef CHECK_OP
 #undef INIT
 
+struct obj_new_flex_elem {
+	int lo;
+	int hi;
+};
+
+struct obj_new_flex {
+	int hdr;
+	struct obj_new_flex_elem cells[];
+};
+
 SEC("?kprobe/xyz")
 int map_compat_kprobe(void *ctx)
 {
@@ -230,6 +240,19 @@ int obj_new_no_struct(void *ctx)
 	return 0;
 }
 
+SEC("?tc")
+int obj_new_flex_array(void *ctx)
+{
+	struct obj_new_flex *p;
+
+	p = bpf_obj_new_impl(bpf_core_type_id_local(struct obj_new_flex), NULL);
+	if (!p)
+		return 0;
+	p->cells[0].hi = 42;
+	bpf_obj_drop_impl(p, NULL);
+	return 0;
+}
+
 SEC("?tc")
 int obj_drop_non_zero_off(void *ctx)
 {
-- 
2.34.1