| Message ID | 20211110174442.619398-1-songliubraving@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: extend BTF_ID_LIST_GLOBAL with parameter for number of IDs | expand |
On Wed, Nov 10, 2021 at 9:45 AM Song Liu <songliubraving@fb.com> wrote: > > syzbot reported the following BUG w/o CONFIG_DEBUG_INFO_BTF > > 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 > ... > > This is caused by hard-coded name[1] in BTF_ID_LIST_GLOBAL (w/o > CONFIG_DEBUG_INFO_BTF). Fix this by adding a parameter n to > BTF_ID_LIST_GLOBAL. This avoids ifdef CONFIG_DEBUG_INFO_BTF in btf.c and > filter.c. > > Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma") > Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com > Suggested-by: Eric Dumazet <edumazet@google.com> > Reported-by: Eric Dumazet <edumazet@google.com> > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > include/linux/btf_ids.h | 6 +++--- > kernel/bpf/btf.c | 2 +- > net/core/filter.c | 6 +----- > 3 files changed, 5 insertions(+), 9 deletions(-) SGTM, thanks ! Reviewed-by: Eric Dumazet <edumazet@google.com>
On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote: > > -#ifdef CONFIG_DEBUG_INFO_BTF > -BTF_ID_LIST_GLOBAL(btf_sock_ids) > +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE) > #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type) > BTF_SOCK_TYPE_xxx > #undef BTF_SOCK_TYPE > -#else > -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE]; > -#endif If we're trying to future proof it I think it would be better to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro. (or have another macro that is tracing specific). That will help avoid cryptic btf_task_struct_ids[0|1|2] references in the code.
> On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote: >> >> -#ifdef CONFIG_DEBUG_INFO_BTF >> -BTF_ID_LIST_GLOBAL(btf_sock_ids) >> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE) >> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type) >> BTF_SOCK_TYPE_xxx >> #undef BTF_SOCK_TYPE >> -#else >> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE]; >> -#endif > > If we're trying to future proof it I think it would be better > to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro. > (or have another macro that is tracing specific). > That will help avoid cryptic btf_task_struct_ids[0|1|2] > references in the code. Yeah, this makes sense. I am taking time off for tomorrow and Friday, so I probably won't have time to implement this before 5.16-rc1. How about we ship this fix as-is, and improve it later? Thanks, Song
On Wed, Nov 10, 2021 at 2:11 PM Song Liu <songliubraving@fb.com> wrote: > > > > > On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote: > >> > >> -#ifdef CONFIG_DEBUG_INFO_BTF > >> -BTF_ID_LIST_GLOBAL(btf_sock_ids) > >> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE) > >> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type) > >> BTF_SOCK_TYPE_xxx > >> #undef BTF_SOCK_TYPE > >> -#else > >> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE]; > >> -#endif > > > > If we're trying to future proof it I think it would be better > > to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro. > > (or have another macro that is tracing specific). > > That will help avoid cryptic btf_task_struct_ids[0|1|2] > > references in the code. > > Yeah, this makes sense. > > I am taking time off for tomorrow and Friday, so I probably won't > have time to implement this before 5.16-rc1. How about we ship > this fix as-is, and improve it later? It's not rc1 material. It's in bpf-next only. There is no rush, I think.
> On Nov 10, 2021, at 2:13 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Nov 10, 2021 at 2:11 PM Song Liu <songliubraving@fb.com> wrote: >> >> >> >>> On Nov 10, 2021, at 2:02 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: >>> >>> On Wed, Nov 10, 2021 at 9:47 AM Song Liu <songliubraving@fb.com> wrote: >>>> >>>> -#ifdef CONFIG_DEBUG_INFO_BTF >>>> -BTF_ID_LIST_GLOBAL(btf_sock_ids) >>>> +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE) >>>> #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type) >>>> BTF_SOCK_TYPE_xxx >>>> #undef BTF_SOCK_TYPE >>>> -#else >>>> -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE]; >>>> -#endif >>> >>> If we're trying to future proof it I think it would be better >>> to combine it with MAX_BTF_SOCK_TYPE and BTF_SOCK_TYPE_xxx macro. >>> (or have another macro that is tracing specific). >>> That will help avoid cryptic btf_task_struct_ids[0|1|2] >>> references in the code. >> >> Yeah, this makes sense. >> >> I am taking time off for tomorrow and Friday, so I probably won't >> have time to implement this before 5.16-rc1. How about we ship >> this fix as-is, and improve it later? > > It's not rc1 material. It's in bpf-next only. There is no rush, I think. Aha, I guess I messed up the branches. Song
diff --git a/include/linux/btf_ids.h b/include/linux/btf_ids.h index 47d9abfbdb556..6bb42b785293a 100644 --- a/include/linux/btf_ids.h +++ b/include/linux/btf_ids.h @@ -73,7 +73,7 @@ asm( \ __BTF_ID_LIST(name, local) \ extern u32 name[]; -#define BTF_ID_LIST_GLOBAL(name) \ +#define BTF_ID_LIST_GLOBAL(name, n) \ __BTF_ID_LIST(name, globl) /* The BTF_ID_LIST_SINGLE macro defines a BTF_ID_LIST with @@ -83,7 +83,7 @@ __BTF_ID_LIST(name, globl) BTF_ID_LIST(name) \ BTF_ID(prefix, typename) #define BTF_ID_LIST_GLOBAL_SINGLE(name, prefix, typename) \ - BTF_ID_LIST_GLOBAL(name) \ + BTF_ID_LIST_GLOBAL(name, 1) \ BTF_ID(prefix, typename) /* @@ -149,7 +149,7 @@ extern struct btf_id_set name; #define BTF_ID_LIST(name) static u32 name[5]; #define BTF_ID(prefix, name) #define BTF_ID_UNUSED -#define BTF_ID_LIST_GLOBAL(name) u32 name[1]; +#define BTF_ID_LIST_GLOBAL(name, n) u32 name[n]; #define BTF_ID_LIST_SINGLE(name, prefix, typename) static u32 name[1]; #define BTF_ID_LIST_GLOBAL_SINGLE(name, prefix, typename) u32 name[1]; #define BTF_SET_START(name) static struct btf_id_set name = { 0 }; diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index cdb0fba656006..1cd2750b4c88e 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -6342,7 +6342,7 @@ const struct bpf_func_proto bpf_btf_find_by_name_kind_proto = { .arg4_type = ARG_ANYTHING, }; -BTF_ID_LIST_GLOBAL(btf_task_struct_ids) +BTF_ID_LIST_GLOBAL(btf_task_struct_ids, 3) BTF_ID(struct, task_struct) BTF_ID(struct, file) BTF_ID(struct, vm_area_struct) diff --git a/net/core/filter.c b/net/core/filter.c index 8e8d3b49c2976..cf3253a0e43af 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -10604,14 +10604,10 @@ void bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog) bpf_dispatcher_change_prog(BPF_DISPATCHER_PTR(xdp), prev_prog, prog); } -#ifdef CONFIG_DEBUG_INFO_BTF -BTF_ID_LIST_GLOBAL(btf_sock_ids) +BTF_ID_LIST_GLOBAL(btf_sock_ids, MAX_BTF_SOCK_TYPE) #define BTF_SOCK_TYPE(name, type) BTF_ID(struct, type) BTF_SOCK_TYPE_xxx #undef BTF_SOCK_TYPE -#else -u32 btf_sock_ids[MAX_BTF_SOCK_TYPE]; -#endif BPF_CALL_1(bpf_skc_to_tcp6_sock, struct sock *, sk) {
syzbot reported the following BUG w/o CONFIG_DEBUG_INFO_BTF 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> This is caused by hard-coded name[1] in BTF_ID_LIST_GLOBAL (w/o CONFIG_DEBUG_INFO_BTF). Fix this by adding a parameter n to BTF_ID_LIST_GLOBAL. This avoids ifdef CONFIG_DEBUG_INFO_BTF in btf.c and filter.c. Fixes: 7c7e3d31e785 ("bpf: Introduce helper bpf_find_vma") Reported-by: syzbot+e0d81ec552a21d9071aa@syzkaller.appspotmail.com Suggested-by: Eric Dumazet <edumazet@google.com> Reported-by: Eric Dumazet <edumazet@google.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- include/linux/btf_ids.h | 6 +++--- kernel/bpf/btf.c | 2 +- net/core/filter.c | 6 +----- 3 files changed, 5 insertions(+), 9 deletions(-)