[PATCH bpf RESEND 0/2] Check the remaining info_cnt before repeating btf fields

[PATCH bpf RESEND 0/2] Check the remaining info_cnt before repeating btf fields

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Hi,

The patch set adds the missed check again info_cnt when flattening the
array of nested struct. The problem was spotted when developing dynptr
key support for hash map. Patch #1 adds the missed check and patch #2
adds three success test cases and one failure test case for the problem.

Comments are always welcome.

Change Log:
v2:
 * patch #1: check info_cnt in btf_repeat_fields()
 * patch #2: use a hard-coded number instead of BTF_FIELDS_MAX, because
             BTF_FIELDS_MAX is not always available in vmlinux.h (e.g.,
	     for llvm 17/18)

v1: https://lore.kernel.org/bpf/20240911110557.2759801-1-houtao@huaweicloud.com/T/#t

Hou Tao (2):
  bpf: Check the remaining info_cnt before repeating btf fields
  selftests/bpf: Add more test case for field flattening

 kernel/bpf/btf.c                              | 14 +++-
 .../selftests/bpf/prog_tests/cpumask.c        |  1 +
 .../selftests/bpf/progs/cpumask_common.h      |  5 ++
 .../selftests/bpf/progs/cpumask_failure.c     | 35 +++++++++
 .../selftests/bpf/progs/cpumask_success.c     | 78 ++++++++++++++++++-
 5 files changed, 127 insertions(+), 6 deletions(-)

-- 
2.29.2

[PATCH bpf RESEND 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

When trying to repeat the btf fields for array of nested struct, it
doesn't check the remaining info_cnt. The following splat will be
reported when the value of ret * nelems is greater than BTF_FIELDS_MAX:

  ------------[ cut here ]------------
  UBSAN: array-index-out-of-bounds in ../kernel/bpf/btf.c:3951:49
  index 11 is out of range for type 'btf_field_info [11]'
  CPU: 6 UID: 0 PID: 411 Comm: test_progs ...... 6.11.0-rc4+ #1
  Tainted: [O]=OOT_MODULE
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ...
  Call Trace:
   <TASK>
   dump_stack_lvl+0x57/0x70
   dump_stack+0x10/0x20
   ubsan_epilogue+0x9/0x40
   __ubsan_handle_out_of_bounds+0x6f/0x80
   ? kallsyms_lookup_name+0x48/0xb0
   btf_parse_fields+0x992/0xce0
   map_create+0x591/0x770
   __sys_bpf+0x229/0x2410
   __x64_sys_bpf+0x1f/0x30
   x64_sys_call+0x199/0x9f0
   do_syscall_64+0x3b/0xc0
   entry_SYSCALL_64_after_hwframe+0x4b/0x53
  RIP: 0033:0x7fea56f2cc5d
  ......
   </TASK>
  ---[ end trace ]---

Fix it by checking the remaining info_cnt in btf_repeat_fields() before
repeating the btf fields.

Fixes: 64e8ee814819 ("bpf: look into the types of the fields of a struct type recursively.")
Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 kernel/bpf/btf.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 75e4fe83c509..9cf0ec2e7cb6 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3523,7 +3523,7 @@ static int btf_get_field_type(const struct btf *btf, const struct btf_type *var_
  *   (i + 1) * elem_size
  * where i is the repeat index and elem_size is the size of an element.
  */
-static int btf_repeat_fields(struct btf_field_info *info,
+static int btf_repeat_fields(struct btf_field_info *info, int info_cnt,
 			     u32 field_cnt, u32 repeat_cnt, u32 elem_size)
 {
 	u32 i, j;
@@ -3543,6 +3543,12 @@ static int btf_repeat_fields(struct btf_field_info *info,
 		}
 	}
 
+	/* The type of struct size or variable size is u32,
+	 * so the multiplication will not overflow.
+	 */
+	if (field_cnt * (repeat_cnt + 1) > info_cnt)
+		return -E2BIG;
+
 	cur = field_cnt;
 	for (i = 0; i < repeat_cnt; i++) {
 		memcpy(&info[cur], &info[0], field_cnt * sizeof(info[0]));
@@ -3587,7 +3593,7 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
 		info[i].off += off;
 
 	if (nelems > 1) {
-		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
+		err = btf_repeat_fields(info, info_cnt, ret, nelems - 1, t->size);
 		if (err == 0)
 			ret *= nelems;
 		else
@@ -3681,10 +3687,10 @@ static int btf_find_field_one(const struct btf *btf,
 
 	if (ret == BTF_FIELD_IGNORE)
 		return 0;
-	if (nelems > info_cnt)
+	if (!info_cnt)
 		return -E2BIG;
 	if (nelems > 1) {
-		ret = btf_repeat_fields(info, 1, nelems - 1, sz);
+		ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.29.2

Re: [PATCH bpf RESEND 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Tue, 2024-10-08 at 15:11 +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> When trying to repeat the btf fields for array of nested struct, it
> doesn't check the remaining info_cnt. The following splat will be
> reported when the value of ret * nelems is greater than BTF_FIELDS_MAX:

[...]

> Fix it by checking the remaining info_cnt in btf_repeat_fields() before
> repeating the btf fields.
> 
> Fixes: 64e8ee814819 ("bpf: look into the types of the fields of a struct type recursively.")
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

> @@ -3681,10 +3687,10 @@ static int btf_find_field_one(const struct btf *btf,
>  
>  	if (ret == BTF_FIELD_IGNORE)
>  		return 0;
> -	if (nelems > info_cnt)
> +	if (!info_cnt)
>  		return -E2BIG;
>  	if (nelems > 1) {
> -		ret = btf_repeat_fields(info, 1, nelems - 1, sz);
> +		ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
>  		if (ret < 0)
>  			return ret;
>  	}


I think the change like below (on top of yours) would work the same
(because nelems is >= 1 at this point):

-       if (!info_cnt)
-               return -E2BIG;
-       if (nelems > 1) {
-               ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
-               if (ret < 0)
-                       return ret;
-       }
+
+       ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
+       if (ret < 0)
+               return ret;

wdyt?

Re: [PATCH bpf RESEND 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Hou Tao]

Hi,

On 10/9/2024 2:42 PM, Eduard Zingerman wrote:
> On Tue, 2024-10-08 at 15:11 +0800, Hou Tao wrote:
>> From: Hou Tao <houtao1@huawei.com>
>>
>> When trying to repeat the btf fields for array of nested struct, it
>> doesn't check the remaining info_cnt. The following splat will be
>> reported when the value of ret * nelems is greater than BTF_FIELDS_MAX:
> [...]
>
>> Fix it by checking the remaining info_cnt in btf_repeat_fields() before
>> repeating the btf fields.
>>
>> Fixes: 64e8ee814819 ("bpf: look into the types of the fields of a struct type recursively.")
>> Signed-off-by: Hou Tao <houtao1@huawei.com>
>> ---
> Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Thanks for the ack.
>
>> @@ -3681,10 +3687,10 @@ static int btf_find_field_one(const struct btf *btf,
>>  
>>  	if (ret == BTF_FIELD_IGNORE)
>>  		return 0;
>> -	if (nelems > info_cnt)
>> +	if (!info_cnt)
>>  		return -E2BIG;
>>  	if (nelems > 1) {
>> -		ret = btf_repeat_fields(info, 1, nelems - 1, sz);
>> +		ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
>>  		if (ret < 0)
>>  			return ret;
>>  	}
>
> I think the change like below (on top of yours) would work the same
> (because nelems is >= 1 at this point):
>
> -       if (!info_cnt)
> -               return -E2BIG;
> -       if (nelems > 1) {
> -               ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
> -               if (ret < 0)
> -                       return ret;
> -       }
> +
> +       ret = btf_repeat_fields(info, info_cnt, 1, nelems - 1, sz);
> +       if (ret < 0)
> +               return ret;
>
> wdyt?

I don't think they are the same. The main reason is due to the check in
the beginning of btf_repeat_field():

        /* Ensure not repeating fields that should not be repeated. */
        for (i = 0; i < field_cnt; i++) {
                switch (info[i].type) {

There are two cases here:
1) info_cnt == 0
Because info_cnt is 0, the found record isn't saved in info[0], the
check will be incorrect

2) nelements ==1 && info_cnt > 0
If the found record is bpf_timer or similar, btf_repeat_fields() will
return -EINVAL instead of 0.

Re: [PATCH bpf RESEND 1/2] bpf: Check the remaining info_cnt before repeating btf fields

[Eduard Zingerman]

On Wed, 2024-10-09 at 15:12 +0800, Hou Tao wrote:

[...]

> I don't think they are the same. The main reason is due to the check in
> the beginning of btf_repeat_field():
> 
>         /* Ensure not repeating fields that should not be repeated. */
>         for (i = 0; i < field_cnt; i++) {
>                 switch (info[i].type) {
> 
> There are two cases here:
> 1) info_cnt == 0
> Because info_cnt is 0, the found record isn't saved in info[0], the
> check will be incorrect
> 
> 2) nelements ==1 && info_cnt > 0
> If the found record is bpf_timer or similar, btf_repeat_fields() will
> return -EINVAL instead of 0.

Oh, right, there is a loop accessing 'info' at the start.
Sorry for the noise.

[PATCH bpf RESEND 2/2] selftests/bpf: Add more test case for field flattening

[Hou Tao]

From: Hou Tao <houtao1@huawei.com>

Add three success test cases to test the flattening of array of nested
struct. For these three tests, the number of special fields in map is
BTF_FIELDS_MAX, but the array is defined in structs with different
nested level.

Add one failure test case for the flattening as well. In the test case,
the number of special fields in map is BTF_FIELDS_MAX + 1. It will make
btf_parse_fields() in map_create() return -E2BIG, the creation of map
will succeed, but the load of program will fail because the btf_record
is invalid for the map.

Signed-off-by: Hou Tao <houtao1@huawei.com>
---
 .../selftests/bpf/prog_tests/cpumask.c        |  1 +
 .../selftests/bpf/progs/cpumask_common.h      |  5 ++
 .../selftests/bpf/progs/cpumask_failure.c     | 35 +++++++++
 .../selftests/bpf/progs/cpumask_success.c     | 78 ++++++++++++++++++-
 4 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/cpumask.c b/tools/testing/selftests/bpf/prog_tests/cpumask.c
index 2570bd4b0cb2..e58a04654238 100644
--- a/tools/testing/selftests/bpf/prog_tests/cpumask.c
+++ b/tools/testing/selftests/bpf/prog_tests/cpumask.c
@@ -23,6 +23,7 @@ static const char * const cpumask_success_testcases[] = {
 	"test_global_mask_array_l2_rcu",
 	"test_global_mask_nested_rcu",
 	"test_global_mask_nested_deep_rcu",
+	"test_global_mask_nested_deep_array_rcu",
 	"test_cpumask_weight",
 };
 
diff --git a/tools/testing/selftests/bpf/progs/cpumask_common.h b/tools/testing/selftests/bpf/progs/cpumask_common.h
index b979e91f55f0..4ece7873ba60 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_common.h
+++ b/tools/testing/selftests/bpf/progs/cpumask_common.h
@@ -7,6 +7,11 @@
 #include "errno.h"
 #include <stdbool.h>
 
+/* Should use BTF_FIELDS_MAX, but it is not always available in vmlinux.h,
+ * so use the hard-coded number as a workaround.
+ */
+#define CPUMASK_KPTR_FIELDS_MAX 11
+
 int err;
 
 #define private(name) SEC(".bss." #name) __attribute__((aligned(8)))
diff --git a/tools/testing/selftests/bpf/progs/cpumask_failure.c b/tools/testing/selftests/bpf/progs/cpumask_failure.c
index a988d2823b52..b40b52548ffb 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_failure.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_failure.c
@@ -10,6 +10,21 @@
 
 char _license[] SEC("license") = "GPL";
 
+struct kptr_nested_array_2 {
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_array_1 {
+	/* Make btf_parse_fields() in map_create() return -E2BIG */
+	struct kptr_nested_array_2 d_2[CPUMASK_KPTR_FIELDS_MAX + 1];
+};
+
+struct kptr_nested_array {
+	struct kptr_nested_array_1 d_1;
+};
+
+private(MASK_NESTED) static struct kptr_nested_array global_mask_nested_arr;
+
 /* Prototype for all of the program trace events below:
  *
  * TRACE_EVENT(task_newtask,
@@ -187,3 +202,23 @@ int BPF_PROG(test_global_mask_rcu_no_null_check, struct task_struct *task, u64 c
 
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__failure __msg("has no valid kptr")
+int BPF_PROG(test_invalid_nested_array, struct task_struct *task, u64 clone_flags)
+{
+	struct bpf_cpumask *local, *prev;
+
+	local = create_cpumask();
+	if (!local)
+		return 0;
+
+	prev = bpf_kptr_xchg(&global_mask_nested_arr.d_1.d_2[CPUMASK_KPTR_FIELDS_MAX].mask, local);
+	if (prev) {
+		bpf_cpumask_release(prev);
+		err = 3;
+		return 0;
+	}
+
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/progs/cpumask_success.c b/tools/testing/selftests/bpf/progs/cpumask_success.c
index fd8106831c32..80ee469b0b60 100644
--- a/tools/testing/selftests/bpf/progs/cpumask_success.c
+++ b/tools/testing/selftests/bpf/progs/cpumask_success.c
@@ -31,11 +31,59 @@ struct kptr_nested_deep {
 	struct kptr_nested_pair ptr_pairs[3];
 };
 
+struct kptr_nested_deep_array_1_2 {
+	int dummy;
+	struct bpf_cpumask __kptr * mask[CPUMASK_KPTR_FIELDS_MAX];
+};
+
+struct kptr_nested_deep_array_1_1 {
+	int dummy;
+	struct kptr_nested_deep_array_1_2 d_2;
+};
+
+struct kptr_nested_deep_array_1 {
+	long dummy;
+	struct kptr_nested_deep_array_1_1 d_1;
+};
+
+struct kptr_nested_deep_array_2_2 {
+	long dummy[2];
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_deep_array_2_1 {
+	int dummy;
+	struct kptr_nested_deep_array_2_2 d_2[CPUMASK_KPTR_FIELDS_MAX];
+};
+
+struct kptr_nested_deep_array_2 {
+	long dummy;
+	struct kptr_nested_deep_array_2_1 d_1;
+};
+
+struct kptr_nested_deep_array_3_2 {
+	long dummy[2];
+	struct bpf_cpumask __kptr * mask;
+};
+
+struct kptr_nested_deep_array_3_1 {
+	int dummy;
+	struct kptr_nested_deep_array_3_2 d_2;
+};
+
+struct kptr_nested_deep_array_3 {
+	long dummy;
+	struct kptr_nested_deep_array_3_1 d_1[CPUMASK_KPTR_FIELDS_MAX];
+};
+
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array[2];
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array_l2[2][1];
 private(MASK) static struct bpf_cpumask __kptr * global_mask_array_one[1];
 private(MASK) static struct kptr_nested global_mask_nested[2];
 private(MASK_DEEP) static struct kptr_nested_deep global_mask_nested_deep;
+private(MASK_1) static struct kptr_nested_deep_array_1 global_mask_nested_deep_array_1;
+private(MASK_2) static struct kptr_nested_deep_array_2 global_mask_nested_deep_array_2;
+private(MASK_3) static struct kptr_nested_deep_array_3 global_mask_nested_deep_array_3;
 
 static bool is_test_task(void)
 {
@@ -543,12 +591,21 @@ static int _global_mask_array_rcu(struct bpf_cpumask **mask0,
 		goto err_exit;
 	}
 
-	/* [<mask 0>, NULL] */
-	if (!*mask0 || *mask1) {
+	/* [<mask 0>, *] */
+	if (!*mask0) {
 		err = 2;
 		goto err_exit;
 	}
 
+	if (!mask1)
+		goto err_exit;
+
+	/* [*, NULL] */
+	if (*mask1) {
+		err = 3;
+		goto err_exit;
+	}
+
 	local = create_cpumask();
 	if (!local) {
 		err = 9;
@@ -631,6 +688,23 @@ int BPF_PROG(test_global_mask_nested_deep_rcu, struct task_struct *task, u64 clo
 	return 0;
 }
 
+SEC("tp_btf/task_newtask")
+int BPF_PROG(test_global_mask_nested_deep_array_rcu, struct task_struct *task, u64 clone_flags)
+{
+	int i;
+
+	for (i = 0; i < CPUMASK_KPTR_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_1.d_1.d_2.mask[i], NULL);
+
+	for (i = 0; i < CPUMASK_KPTR_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_2.d_1.d_2[i].mask, NULL);
+
+	for (i = 0; i < CPUMASK_KPTR_FIELDS_MAX; i++)
+		_global_mask_array_rcu(&global_mask_nested_deep_array_3.d_1[i].d_2.mask, NULL);
+
+	return 0;
+}
+
 SEC("tp_btf/task_newtask")
 int BPF_PROG(test_cpumask_weight, struct task_struct *task, u64 clone_flags)
 {
-- 
2.29.2

Re: [PATCH bpf RESEND 2/2] selftests/bpf: Add more test case for field flattening

[Eduard Zingerman]

On Tue, 2024-10-08 at 15:11 +0800, Hou Tao wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Add three success test cases to test the flattening of array of nested
> struct. For these three tests, the number of special fields in map is
> BTF_FIELDS_MAX, but the array is defined in structs with different
> nested level.
> 
> Add one failure test case for the flattening as well. In the test case,
> the number of special fields in map is BTF_FIELDS_MAX + 1. It will make
> btf_parse_fields() in map_create() return -E2BIG, the creation of map
> will succeed, but the load of program will fail because the btf_record
> is invalid for the map.
> 
> Signed-off-by: Hou Tao <houtao1@huawei.com>
> ---

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

[...]

Re: [PATCH bpf RESEND 0/2] Check the remaining info_cnt before repeating btf fields

[patchwork-bot+netdevbpf]

Hello:

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

On Tue,  8 Oct 2024 15:11:12 +0800 you wrote:
> From: Hou Tao <houtao1@huawei.com>
> 
> Hi,
> 
> The patch set adds the missed check again info_cnt when flattening the
> array of nested struct. The problem was spotted when developing dynptr
> key support for hash map. Patch #1 adds the missed check and patch #2
> adds three success test cases and one failure test case for the problem.
> 
> [...]

Here is the summary with links:
  - [bpf,RESEND,1/2] bpf: Check the remaining info_cnt before repeating btf fields
    https://git.kernel.org/bpf/bpf/c/797d73ee232d
  - [bpf,RESEND,2/2] selftests/bpf: Add more test case for field flattening
    https://git.kernel.org/bpf/bpf/c/c456f0804058

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