| Message ID | 20211109222447.3251621-1-songliubraving@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: fix btf_task_struct_ids w/o CONFIG_DEBUG_INFO_BTF | expand |
On Tue, Nov 9, 2021 at 2:25 PM Song Liu <songliubraving@fb.com> wrote: > > This fixes KASAN oops like > > BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 > Read of size 4 at addr ffffffff90297404 by task swapper/0/1 > > CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-syzkaller #0 > Hardware name: ... Google Compute Engine, BIOS Google 01/01/2011 > Call Trace: > <TASK> > __dump_stack lib/dump_stack.c:88 [inline] > dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 > print_address_description.constprop.0.cold+0xf/0x309 mm/kasan/report.c:256 > __kasan_report mm/kasan/report.c:442 [inline] > kasan_report.cold+0x83/0xdf mm/kasan/report.c:459 > task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 > do_one_initcall+0x103/0x650 init/main.c:1295 > do_initcall_level init/main.c:1368 [inline] > do_initcalls init/main.c:1384 [inline] > do_basic_setup init/main.c:1403 [inline] > kernel_init_freeable+0x6b1/0x73a init/main.c:1606 > kernel_init+0x1a/0x1d0 init/main.c:1497 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 > </TASK> > Please add a Fixes: tag Also you can add Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com > Reported-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > kernel/bpf/btf.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index cdb0fba656006..6db929a5826d4 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -6342,10 +6342,14 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = { > .arg4_type = ARG_ANYTHING, > }; > > +#ifdef CONFIG_DEBUG_INFO_BTF > BTF_ID_LIST_GLOBAL(btf_task_struct_ids) > BTF_ID(struct, task_struct) > BTF_ID(struct, file) > BTF_ID(struct, vm_area_struct) > +#else > +u32 btf_task_struct_ids[3]; > +#endif What about adding to BTF_ID_LIST_GLOBAL() another argument ? BTF_ID_LIST_GLOBAL(btf_task_struct_ids, 3) This would avoid this #ifdef I understand commit 079ef53673f2e3b3ee1728800311f20f28eed4f7 hardcoded a [5] value, maybe we can do slightly better with exact boundary checks for KASAN. > > /* BTF ID set registration API for modules */ > > -- > 2.30.2 >
> On Nov 9, 2021, at 2:38 PM, Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Nov 9, 2021 at 2:25 PM Song Liu <songliubraving@fb.com> wrote: >> >> This fixes KASAN oops like >> >> BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 >> Read of size 4 at addr ffffffff90297404 by task swapper/0/1 >> >> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-syzkaller #0 >> Hardware name: ... Google Compute Engine, BIOS Google 01/01/2011 >> Call Trace: >> <TASK> >> __dump_stack lib/dump_stack.c:88 [inline] >> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 >> print_address_description.constprop.0.cold+0xf/0x309 mm/kasan/report.c:256 >> __kasan_report mm/kasan/report.c:442 [inline] >> kasan_report.cold+0x83/0xdf mm/kasan/report.c:459 >> task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 >> do_one_initcall+0x103/0x650 init/main.c:1295 >> do_initcall_level init/main.c:1368 [inline] >> do_initcalls init/main.c:1384 [inline] >> do_basic_setup init/main.c:1403 [inline] >> kernel_init_freeable+0x6b1/0x73a init/main.c:1606 >> kernel_init+0x1a/0x1d0 init/main.c:1497 >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 >> </TASK> >> > > Please add a Fixes: tag Will add it in v2. > > Also you can add > > Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com > > >> Reported-by: Eric Dumazet <edumazet@google.com> >> Signed-off-by: Song Liu <songliubraving@fb.com> >> --- >> kernel/bpf/btf.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >> index cdb0fba656006..6db929a5826d4 100644 >> --- a/kernel/bpf/btf.c >> +++ b/kernel/bpf/btf.c >> @@ -6342,10 +6342,14 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = { >> .arg4_type = ARG_ANYTHING, >> }; >> >> +#ifdef CONFIG_DEBUG_INFO_BTF >> BTF_ID_LIST_GLOBAL(btf_task_struct_ids) >> BTF_ID(struct, task_struct) >> BTF_ID(struct, file) >> BTF_ID(struct, vm_area_struct) >> +#else >> +u32 btf_task_struct_ids[3]; >> +#endif > > What about adding to BTF_ID_LIST_GLOBAL() another argument ? > > BTF_ID_LIST_GLOBAL(btf_task_struct_ids, 3) > > This would avoid this #ifdef > > I understand commit 079ef53673f2e3b3ee1728800311f20f28eed4f7 > hardcoded a [5] value, maybe we can do slightly better with exact > boundary checks for KASAN. I like this idea. Yonghong, I believe you added BTF_ID_LIST_GLOBAL. What do you think about this proposal? Thanks, Song
On 11/9/21 2:56 PM, Song Liu wrote: > > >> On Nov 9, 2021, at 2:38 PM, Eric Dumazet <edumazet@google.com> wrote: >> >> On Tue, Nov 9, 2021 at 2:25 PM Song Liu <songliubraving@fb.com> wrote: >>> >>> This fixes KASAN oops like >>> >>> BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 >>> Read of size 4 at addr ffffffff90297404 by task swapper/0/1 >>> >>> CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-syzkaller #0 >>> Hardware name: ... Google Compute Engine, BIOS Google 01/01/2011 >>> Call Trace: >>> <TASK> >>> __dump_stack lib/dump_stack.c:88 [inline] >>> dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 >>> print_address_description.constprop.0.cold+0xf/0x309 mm/kasan/report.c:256 >>> __kasan_report mm/kasan/report.c:442 [inline] >>> kasan_report.cold+0x83/0xdf mm/kasan/report.c:459 >>> task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 >>> do_one_initcall+0x103/0x650 init/main.c:1295 >>> do_initcall_level init/main.c:1368 [inline] >>> do_initcalls init/main.c:1384 [inline] >>> do_basic_setup init/main.c:1403 [inline] >>> kernel_init_freeable+0x6b1/0x73a init/main.c:1606 >>> kernel_init+0x1a/0x1d0 init/main.c:1497 >>> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 >>> </TASK> >>> >> >> Please add a Fixes: tag > > Will add it in v2. > >> >> Also you can add >> >> Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com >> >> >>> Reported-by: Eric Dumazet <edumazet@google.com> >>> Signed-off-by: Song Liu <songliubraving@fb.com> >>> --- >>> kernel/bpf/btf.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c >>> index cdb0fba656006..6db929a5826d4 100644 >>> --- a/kernel/bpf/btf.c >>> +++ b/kernel/bpf/btf.c >>> @@ -6342,10 +6342,14 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = { >>> .arg4_type = ARG_ANYTHING, >>> }; >>> >>> +#ifdef CONFIG_DEBUG_INFO_BTF >>> BTF_ID_LIST_GLOBAL(btf_task_struct_ids) >>> BTF_ID(struct, task_struct) >>> BTF_ID(struct, file) >>> BTF_ID(struct, vm_area_struct) >>> +#else >>> +u32 btf_task_struct_ids[3]; >>> +#endif >> >> What about adding to BTF_ID_LIST_GLOBAL() another argument ? >> >> BTF_ID_LIST_GLOBAL(btf_task_struct_ids, 3) >> >> This would avoid this #ifdef >> >> I understand commit 079ef53673f2e3b3ee1728800311f20f28eed4f7 >> hardcoded a [5] value, maybe we can do slightly better with exact >> boundary checks for KASAN. > > I like this idea. > > Yonghong, I believe you added BTF_ID_LIST_GLOBAL. What do you think > about this proposal? I think this is better, so we can have a single place (btf_ids.h) to have #ifdef CONFIG_DEBUG_INFO_BTF. > > Thanks, > Song >
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index cdb0fba656006..6db929a5826d4 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6342,10 +6342,14 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = { .arg4_type = ARG_ANYTHING, }; +#ifdef CONFIG_DEBUG_INFO_BTF BTF_ID_LIST_GLOBAL(btf_task_struct_ids) BTF_ID(struct, task_struct) BTF_ID(struct, file) BTF_ID(struct, vm_area_struct) +#else +u32 btf_task_struct_ids[3]; +#endif /* BTF ID set registration API for modules */
This fixes KASAN oops like BUG: KASAN: global-out-of-bounds in task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 Read of size 4 at addr ffffffff90297404 by task swapper/0/1 CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.15.0-syzkaller #0 Hardware name: ... Google Compute Engine, BIOS Google 01/01/2011 Call Trace: <TASK> __dump_stack lib/dump_stack.c:88 [inline] dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106 print_address_description.constprop.0.cold+0xf/0x309 mm/kasan/report.c:256 __kasan_report mm/kasan/report.c:442 [inline] kasan_report.cold+0x83/0xdf mm/kasan/report.c:459 task_iter_init+0x212/0x2e7 kernel/bpf/task_iter.c:661 do_one_initcall+0x103/0x650 init/main.c:1295 do_initcall_level init/main.c:1368 [inline] do_initcalls init/main.c:1384 [inline] do_basic_setup init/main.c:1403 [inline] kernel_init_freeable+0x6b1/0x73a init/main.c:1606 kernel_init+0x1a/0x1d0 init/main.c:1497 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 </TASK> Reported-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- kernel/bpf/btf.c | 4 ++++ 1 file changed, 4 insertions(+)