diff mbox series

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

Message ID 20240911110557.2759801-2-houtao@huaweicloud.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Check the remaining info_cnt before repeating btf fields | expand

Checks

Context Check Description
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-28 fail Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-VM_Test-33 success Logs for x86_64-llvm-18 / test
bpf/vmtest-bpf-VM_Test-29 success Logs for x86_64-llvm-17 / test
bpf/vmtest-bpf-VM_Test-27 fail Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-34 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-VM_Test-31 fail Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-32 fail Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-VM_Test-26 fail Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-PR fail PR summary
bpf/vmtest-bpf-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 16 this patch: 16
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers success CCed 14 of 14 maintainers
netdev/build_clang success Errors and warnings before: 16 this patch: 16
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 18 this patch: 18
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 12 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Hou Tao Sept. 11, 2024, 11:05 a.m. UTC 
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 12 is out of range for type 'btf_field_info [12]'
  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 before btf field repetition.

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 | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Eduard Zingerman Sept. 11, 2024, 5:37 p.m. UTC | #1 
On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:


[...]

> ---
>  kernel/bpf/btf.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index a4e4f8d43ecf..9a4a074d26f5 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
>  		info[i].off += off;
>  
>  	if (nelems > 1) {
> +		/* The type of struct size or variable size is u32,
> +		 * so the multiplication will not overflow.
> +		 */
> +		if (ret * nelems > info_cnt)
> +			return -E2BIG;
> +
>  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
>  		if (err == 0)
>  			ret *= nelems;


btf_repeat_fields(struct btf_field_info *info,
                  u32 field_cnt, u32 repeat_cnt, u32 elem_size)

copies field "field_cnt * repeat_cnt" times,
in this case field_cnt == ret, repeat_cnt == nelems - 1,
should the check be "ret * (nelems - 1) > info_cnt"?

I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
this check there. So that the check won't be forgotten again if
btf_repeat_fields() is used elsewhere. Wdyt?
Hou Tao Sept. 12, 2024, 1:20 a.m. UTC | #2 
Hi,

On 9/12/2024 1:37 AM, Eduard Zingerman wrote:
> On Wed, 2024-09-11 at 19:05 +0800, Hou Tao wrote:
>
>
> [...]
>
>> ---
>>  kernel/bpf/btf.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index a4e4f8d43ecf..9a4a074d26f5 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
>>  		info[i].off += off;
>>  
>>  	if (nelems > 1) {
>> +		/* The type of struct size or variable size is u32,
>> +		 * so the multiplication will not overflow.
>> +		 */
>> +		if (ret * nelems > info_cnt)
>> +			return -E2BIG;
>> +
>>  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
>>  		if (err == 0)
>>  			ret *= nelems;
>
> btf_repeat_fields(struct btf_field_info *info,
>                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
>
> copies field "field_cnt * repeat_cnt" times,
> in this case field_cnt == ret, repeat_cnt == nelems - 1,
> should the check be "ret * (nelems - 1) > info_cnt"?

No. The number of available btf_field_info is info_cnt,
btf_find_struct_field() has already used ret fields, and there are still
ret * (nelems - 1) fields waiting for repetition, so checking ret *
nelems against info_cnt is correct.
>
> I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
> this check there. So that the check won't be forgotten again if
> btf_repeat_fields() is used elsewhere. Wdyt?
Will do in v2.
Eduard Zingerman Sept. 12, 2024, 3:03 a.m. UTC | #3 
On Thu, 2024-09-12 at 09:20 +0800, Hou Tao wrote:

[...]

> > > @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
> > >  		info[i].off += off;
> > >  
> > >  	if (nelems > 1) {
> > > +		/* The type of struct size or variable size is u32,
> > > +		 * so the multiplication will not overflow.
> > > +		 */
> > > +		if (ret * nelems > info_cnt)
> > > +			return -E2BIG;
> > > +
> > >  		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
> > >  		if (err == 0)
> > >  			ret *= nelems;
> > 
> > btf_repeat_fields(struct btf_field_info *info,
> >                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
> > 
> > copies field "field_cnt * repeat_cnt" times,
> > in this case field_cnt == ret, repeat_cnt == nelems - 1,
> > should the check be "ret * (nelems - 1) > info_cnt"?
> 
> No. The number of available btf_field_info is info_cnt,
> btf_find_struct_field() has already used ret fields, and there are still
> ret * (nelems - 1) fields waiting for repetition, so checking ret *
> nelems against info_cnt is correct.

Please bear with me. Here is btf_repeat_fields():

    static int btf_repeat_fields(struct btf_field_info *info,
                     u32 field_cnt, u32 repeat_cnt, u32 elem_size)
    {
        u32 i, j;
        u32 cur;
        ...
        cur = field_cnt;
        for (i = 0; i < repeat_cnt; i++) {
            ...
            for (j = 0; j < field_cnt; j++)
                info[cur++].off += (i + 1) * elem_size;
        }
        ...
    }

The range for 'cur' is [field_cnt .. field_cnt * repeat_cnt].
Meaning that at-least 'field_cnt * repeat_cnt' entries are necessary
in the 'info' array.
Given parameters passed to the function, this is 'ret * (nelems - 1)'.
What do I miss?
Eduard Zingerman Sept. 12, 2024, 3:14 a.m. UTC | #4 
On Wed, 2024-09-11 at 20:03 -0700, Eduard Zingerman wrote:

[...]

> Please bear with me. Here is btf_repeat_fields():
> 
>     static int btf_repeat_fields(struct btf_field_info *info,
>                      u32 field_cnt, u32 repeat_cnt, u32 elem_size)
>     {
>         u32 i, j;
>         u32 cur;
>         ...
>         cur = field_cnt;
>         for (i = 0; i < repeat_cnt; i++) {
>             ...
>             for (j = 0; j < field_cnt; j++)
>                 info[cur++].off += (i + 1) * elem_size;
>         }
>         ...
>     }
> 
> The range for 'cur' is [field_cnt .. field_cnt * repeat_cnt].
> Meaning that at-least 'field_cnt * repeat_cnt' entries are necessary
> in the 'info' array.

Ok, I'm wrong.
The range for 'cur' is [field_cnt .. field_cnt * (repeat_cnt + 1)].
So with parameters passed maximal value of 'cur' is 'ret * nelems' indeed.
Sorry for the noise.
Eduard Zingerman Sept. 12, 2024, 7:45 p.m. UTC | #5 
On Thu, 2024-09-12 at 21:13 +0200, Thinker Li wrote:

[...]

> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index a4e4f8d43ecf..9a4a074d26f5 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -3592,6 +3592,12 @@ static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
> > >                info[i].off += off;
> > >   
> > >        if (nelems > 1) {
> > > +             /* The type of struct size or variable size is u32,
> > > +              * so the multiplication will not overflow.
> > > +              */
> > > +             if (ret * nelems > info_cnt)
> > > +                     return -E2BIG;
> > > +
> > >                err = btf_repeat_fields(info, ret, nelems - 1, t->size);
> > >                if (err == 0)
> > >                        ret *= nelems;
> > 
> > 
> > btf_repeat_fields(struct btf_field_info *info,
> >                   u32 field_cnt, u32 repeat_cnt, u32 elem_size)
> > 
> > copies field "field_cnt * repeat_cnt" times,
> > in this case field_cnt == ret, repeat_cnt == nelems - 1,
> > should the check be "ret * (nelems - 1) > info_cnt"?
> > 
> > I suggest to add info_cnt as a parameter of btf_repeat_fields() and do
> > this check there. So that the check won't be forgotten again if
> > btf_repeat_fields() is used elsewhere. Wdyt?
> > 
> 
> Should not this check be moved before the earlier for-loop?

Shouldn't the check for 'ret <= 0' be enough to make sure the for-loop
is fine?
diff mbox series

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index a4e4f8d43ecf..9a4a074d26f5 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -3592,6 +3592,12 @@  static int btf_find_nested_struct(const struct btf *btf, const struct btf_type *
 		info[i].off += off;
 
 	if (nelems > 1) {
+		/* The type of struct size or variable size is u32,
+		 * so the multiplication will not overflow.
+		 */
+		if (ret * nelems > info_cnt)
+			return -E2BIG;
+
 		err = btf_repeat_fields(info, ret, nelems - 1, t->size);
 		if (err == 0)
 			ret *= nelems;