| Message ID | 20220706002612.4013790-1-song@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1d5f82d9dd477d5c66e0214a68c3e4f308eadd6d |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf, x86: fix freeing of not-finalized bpf_prog_pack | expand |
On Tue, Jul 5, 2022 at 5:26 PM Song Liu <song@kernel.org> wrote: > > syzbot reported a few issues with bpf_prog_pack [1], [2]. These are > triggered when the program passed initial JIT in jit_subprogs(), but > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is > freed. > > Fix this with a custom bpf_jit_free() for x86_64, which calls > bpf_jit_binary_pack_finalize() if necessary. Also, with custom > bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more, > remove it. > > Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") > [1] https://syzkaller.appspot.com/bug?extid=2f649ec6d2eea1495a8f > [2] https://syzkaller.appspot.com/bug?extid=87f65c75f4a72db05445 > Reported-by: syzbot+2f649ec6d2eea1495a8f@syzkaller.appspotmail.com > Reported-by: syzbot+87f65c75f4a72db05445@syzkaller.appspotmail.com > Signed-off-by: Song Liu <song@kernel.org> > --- > arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++++ > include/linux/bpf.h | 1 - > include/linux/filter.h | 8 ++++++++ > kernel/bpf/core.c | 29 ++++++++++++----------------- > 4 files changed, 45 insertions(+), 18 deletions(-) > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > index c98b8c0ed3b8..c3dca4c97e48 100644 > --- a/arch/x86/net/bpf_jit_comp.c > +++ b/arch/x86/net/bpf_jit_comp.c > @@ -2492,3 +2492,28 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len) > return ERR_PTR(-EINVAL); > return dst; > } > + > +void bpf_jit_free(struct bpf_prog *prog) > +{ > + if (prog->jited) { > + struct x64_jit_data *jit_data = prog->aux->jit_data; > + struct bpf_binary_header *hdr; > + > + /* > + * If we fail the final pass of JIT (from jit_subprogs), > + * the program may not be finalized yet. Call finalize here > + * before freeing it. > + */ > + if (jit_data) { > + bpf_jit_binary_pack_finalize(prog, jit_data->header, > + jit_data->rw_header); > + kvfree(jit_data->addrs); > + kfree(jit_data); > + } It looks like a workaround for missed cleanup on the JIT side. When bpf_int_jit_compile() fails it is supposed to free jit_data immediately. > passed initial JIT in jit_subprogs(), but > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > called before bpf_jit_binary_pack_finalize() It feels that bpf_int_jit_compile() should call bpf_jit_binary_pack_finalize() instead in the path where it's failing. I could be missing details on what exactly "failed final pass of JIT" means.
On Tue, Jul 12, 2022 at 3:09 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Jul 5, 2022 at 5:26 PM Song Liu <song@kernel.org> wrote: > > > > syzbot reported a few issues with bpf_prog_pack [1], [2]. These are > > triggered when the program passed initial JIT in jit_subprogs(), but > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > > called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is > > freed. > > > > Fix this with a custom bpf_jit_free() for x86_64, which calls > > bpf_jit_binary_pack_finalize() if necessary. Also, with custom > > bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more, > > remove it. > > > > Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") > > [1] https://syzkaller.appspot.com/bug?extid=2f649ec6d2eea1495a8f > > [2] https://syzkaller.appspot.com/bug?extid=87f65c75f4a72db05445 > > Reported-by: syzbot+2f649ec6d2eea1495a8f@syzkaller.appspotmail.com > > Reported-by: syzbot+87f65c75f4a72db05445@syzkaller.appspotmail.com > > Signed-off-by: Song Liu <song@kernel.org> > > --- > > arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++++ > > include/linux/bpf.h | 1 - > > include/linux/filter.h | 8 ++++++++ > > kernel/bpf/core.c | 29 ++++++++++++----------------- > > 4 files changed, 45 insertions(+), 18 deletions(-) > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > index c98b8c0ed3b8..c3dca4c97e48 100644 > > --- a/arch/x86/net/bpf_jit_comp.c > > +++ b/arch/x86/net/bpf_jit_comp.c > > @@ -2492,3 +2492,28 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len) > > return ERR_PTR(-EINVAL); > > return dst; > > } > > + > > +void bpf_jit_free(struct bpf_prog *prog) > > +{ > > + if (prog->jited) { > > + struct x64_jit_data *jit_data = prog->aux->jit_data; > > + struct bpf_binary_header *hdr; > > + > > + /* > > + * If we fail the final pass of JIT (from jit_subprogs), > > + * the program may not be finalized yet. Call finalize here > > + * before freeing it. > > + */ > > + if (jit_data) { > > + bpf_jit_binary_pack_finalize(prog, jit_data->header, > > + jit_data->rw_header); > > + kvfree(jit_data->addrs); > > + kfree(jit_data); > > + } > > It looks like a workaround for missed cleanup on the JIT side. > When bpf_int_jit_compile() fails it is supposed to free jit_data > immediately. > > > passed initial JIT in jit_subprogs(), but > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > > called before bpf_jit_binary_pack_finalize() > > It feels that bpf_int_jit_compile() should call > bpf_jit_binary_pack_finalize() instead in the path where > it's failing. > I could be missing details on what exactly > "failed final pass of JIT" means. This only happens with multiple subprogs. In jit_subprogs(), we first call bpf_int_jit_compile() on each sub program. And then, we call it on each sub program again. jit_data is not freed in the first call of bpf_int_jit_compile(). Similarly we don't call bpf_jit_binary_pack_finalize() in the first call of bpf_int_jit_compile(). If bpf_int_jit_compile() failed for one sub program, we will call bpf_jit_binary_pack_finalize() for this sub program. However, we don't have a chance to call it for other sub programs. Then we will hit "goto out_free" in jit_subprogs(), and call bpf_jit_free on some subprograms that haven't got bpf_jit_binary_pack_finalize() yet. So, I think bpf_jit_free is the best place we can add the extra check and call bpf_jit_binary_pack_finalize(). Does this make sense? Thanks, Song
On Tue, Jul 12, 2022 at 4:02 PM Song Liu <song@kernel.org> wrote: > > On Tue, Jul 12, 2022 at 3:09 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Tue, Jul 5, 2022 at 5:26 PM Song Liu <song@kernel.org> wrote: > > > > > > syzbot reported a few issues with bpf_prog_pack [1], [2]. These are > > > triggered when the program passed initial JIT in jit_subprogs(), but > > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > > > called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is > > > freed. > > > > > > Fix this with a custom bpf_jit_free() for x86_64, which calls > > > bpf_jit_binary_pack_finalize() if necessary. Also, with custom > > > bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more, > > > remove it. > > > > > > Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") > > > [1] https://syzkaller.appspot.com/bug?extid=2f649ec6d2eea1495a8f > > > [2] https://syzkaller.appspot.com/bug?extid=87f65c75f4a72db05445 > > > Reported-by: syzbot+2f649ec6d2eea1495a8f@syzkaller.appspotmail.com > > > Reported-by: syzbot+87f65c75f4a72db05445@syzkaller.appspotmail.com > > > Signed-off-by: Song Liu <song@kernel.org> > > > --- > > > arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++++ > > > include/linux/bpf.h | 1 - > > > include/linux/filter.h | 8 ++++++++ > > > kernel/bpf/core.c | 29 ++++++++++++----------------- > > > 4 files changed, 45 insertions(+), 18 deletions(-) > > > > > > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c > > > index c98b8c0ed3b8..c3dca4c97e48 100644 > > > --- a/arch/x86/net/bpf_jit_comp.c > > > +++ b/arch/x86/net/bpf_jit_comp.c > > > @@ -2492,3 +2492,28 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len) > > > return ERR_PTR(-EINVAL); > > > return dst; > > > } > > > + > > > +void bpf_jit_free(struct bpf_prog *prog) > > > +{ > > > + if (prog->jited) { > > > + struct x64_jit_data *jit_data = prog->aux->jit_data; > > > + struct bpf_binary_header *hdr; > > > + > > > + /* > > > + * If we fail the final pass of JIT (from jit_subprogs), > > > + * the program may not be finalized yet. Call finalize here > > > + * before freeing it. > > > + */ > > > + if (jit_data) { > > > + bpf_jit_binary_pack_finalize(prog, jit_data->header, > > > + jit_data->rw_header); > > > + kvfree(jit_data->addrs); > > > + kfree(jit_data); > > > + } > > > > It looks like a workaround for missed cleanup on the JIT side. > > When bpf_int_jit_compile() fails it is supposed to free jit_data > > immediately. > > > > > passed initial JIT in jit_subprogs(), but > > > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > > > called before bpf_jit_binary_pack_finalize() > > > > It feels that bpf_int_jit_compile() should call > > bpf_jit_binary_pack_finalize() instead in the path where > > it's failing. > > I could be missing details on what exactly > > "failed final pass of JIT" means. > > This only happens with multiple subprogs. In jit_subprogs(), we > first call bpf_int_jit_compile() on each sub program. And then, > we call it on each sub program again. jit_data is not freed in the > first call of bpf_int_jit_compile(). Similarly we don't call > bpf_jit_binary_pack_finalize() in the first call of bpf_int_jit_compile(). > > If bpf_int_jit_compile() failed for one sub program, we will call > bpf_jit_binary_pack_finalize() for this sub program. However, > we don't have a chance to call it for other sub programs. Then > we will hit "goto out_free" in jit_subprogs(), and call bpf_jit_free > on some subprograms that haven't got bpf_jit_binary_pack_finalize() > yet. So, I think bpf_jit_free is the best place we can add the extra > check and call bpf_jit_binary_pack_finalize(). > > Does this make sense? Got it. I've amended the commit log with the above details. Considering that we're at rc6 and the amount of conflicts this patch would cause between bpf and bpf-next trees I pushed it to bpf-next after applying manually. Thanks.
Hello: This patch was applied to bpf/bpf-next.git (master) by Alexei Starovoitov <ast@kernel.org>: On Tue, 5 Jul 2022 17:26:12 -0700 you wrote: > syzbot reported a few issues with bpf_prog_pack [1], [2]. These are > triggered when the program passed initial JIT in jit_subprogs(), but > failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is > called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is > freed. > > Fix this with a custom bpf_jit_free() for x86_64, which calls > bpf_jit_binary_pack_finalize() if necessary. Also, with custom > bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more, > remove it. > > [...] Here is the summary with links: - [bpf] bpf, x86: fix freeing of not-finalized bpf_prog_pack https://git.kernel.org/bpf/bpf-next/c/1d5f82d9dd47 You are awesome, thank you!
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c index c98b8c0ed3b8..c3dca4c97e48 100644 --- a/arch/x86/net/bpf_jit_comp.c +++ b/arch/x86/net/bpf_jit_comp.c @@ -2492,3 +2492,28 @@ void *bpf_arch_text_copy(void *dst, void *src, size_t len) return ERR_PTR(-EINVAL); return dst; } + +void bpf_jit_free(struct bpf_prog *prog) +{ + if (prog->jited) { + struct x64_jit_data *jit_data = prog->aux->jit_data; + struct bpf_binary_header *hdr; + + /* + * If we fail the final pass of JIT (from jit_subprogs), + * the program may not be finalized yet. Call finalize here + * before freeing it. + */ + if (jit_data) { + bpf_jit_binary_pack_finalize(prog, jit_data->header, + jit_data->rw_header); + kvfree(jit_data->addrs); + kfree(jit_data); + } + hdr = bpf_jit_binary_pack_hdr(prog); + bpf_jit_binary_pack_free(hdr, NULL); + WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(prog)); + } + + bpf_prog_unlock_free(prog); +} diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 2b914a56a2c5..7424cf234ae0 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1025,7 +1025,6 @@ struct bpf_prog_aux { bool sleepable; bool tail_call_reachable; bool xdp_has_frags; - bool use_bpf_prog_pack; /* BTF_KIND_FUNC_PROTO for valid attach_btf_id */ const struct btf_type *attach_func_proto; /* function name for valid attach_btf_id */ diff --git a/include/linux/filter.h b/include/linux/filter.h index ed0c0ff42ad5..5005bf2d30bd 100644 --- a/include/linux/filter.h +++ b/include/linux/filter.h @@ -1060,6 +1060,14 @@ u64 bpf_jit_alloc_exec_limit(void); void *bpf_jit_alloc_exec(unsigned long size); void bpf_jit_free_exec(void *addr); void bpf_jit_free(struct bpf_prog *fp); +struct bpf_binary_header * +bpf_jit_binary_pack_hdr(const struct bpf_prog *fp); + +static inline bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) +{ + return list_empty(&fp->aux->ksym.lnode) || + fp->aux->ksym.lnode.prev == LIST_POISON2; +} struct bpf_binary_header * bpf_jit_binary_pack_alloc(unsigned int proglen, u8 **ro_image, diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c index 5f6f3f829b36..360ceb817639 100644 --- a/kernel/bpf/core.c +++ b/kernel/bpf/core.c @@ -647,12 +647,6 @@ static bool bpf_prog_kallsyms_candidate(const struct bpf_prog *fp) return fp->jited && !bpf_prog_was_classic(fp); } -static bool bpf_prog_kallsyms_verify_off(const struct bpf_prog *fp) -{ - return list_empty(&fp->aux->ksym.lnode) || - fp->aux->ksym.lnode.prev == LIST_POISON2; -} - void bpf_prog_kallsyms_add(struct bpf_prog *fp) { if (!bpf_prog_kallsyms_candidate(fp) || @@ -1150,7 +1144,6 @@ int bpf_jit_binary_pack_finalize(struct bpf_prog *prog, bpf_prog_pack_free(ro_header); return PTR_ERR(ptr); } - prog->aux->use_bpf_prog_pack = true; return 0; } @@ -1174,17 +1167,23 @@ void bpf_jit_binary_pack_free(struct bpf_binary_header *ro_header, bpf_jit_uncharge_modmem(size); } +struct bpf_binary_header * +bpf_jit_binary_pack_hdr(const struct bpf_prog *fp) +{ + unsigned long real_start = (unsigned long)fp->bpf_func; + unsigned long addr; + + addr = real_start & BPF_PROG_CHUNK_MASK; + return (void *)addr; +} + static inline struct bpf_binary_header * bpf_jit_binary_hdr(const struct bpf_prog *fp) { unsigned long real_start = (unsigned long)fp->bpf_func; unsigned long addr; - if (fp->aux->use_bpf_prog_pack) - addr = real_start & BPF_PROG_CHUNK_MASK; - else - addr = real_start & PAGE_MASK; - + addr = real_start & PAGE_MASK; return (void *)addr; } @@ -1197,11 +1196,7 @@ void __weak bpf_jit_free(struct bpf_prog *fp) if (fp->jited) { struct bpf_binary_header *hdr = bpf_jit_binary_hdr(fp); - if (fp->aux->use_bpf_prog_pack) - bpf_jit_binary_pack_free(hdr, NULL /* rw_buffer */); - else - bpf_jit_binary_free(hdr); - + bpf_jit_binary_free(hdr); WARN_ON_ONCE(!bpf_prog_kallsyms_verify_off(fp)); }
syzbot reported a few issues with bpf_prog_pack [1], [2]. These are triggered when the program passed initial JIT in jit_subprogs(), but failed final pass of JIT. At this point, bpf_jit_binary_pack_free() is called before bpf_jit_binary_pack_finalize(), and the whole 2MB page is freed. Fix this with a custom bpf_jit_free() for x86_64, which calls bpf_jit_binary_pack_finalize() if necessary. Also, with custom bpf_jit_free(), bpf_prog_aux->use_bpf_prog_pack is not needed any more, remove it. Fixes: 1022a5498f6f ("bpf, x86_64: Use bpf_jit_binary_pack_alloc") [1] https://syzkaller.appspot.com/bug?extid=2f649ec6d2eea1495a8f [2] https://syzkaller.appspot.com/bug?extid=87f65c75f4a72db05445 Reported-by: syzbot+2f649ec6d2eea1495a8f@syzkaller.appspotmail.com Reported-by: syzbot+87f65c75f4a72db05445@syzkaller.appspotmail.com Signed-off-by: Song Liu <song@kernel.org> --- arch/x86/net/bpf_jit_comp.c | 25 +++++++++++++++++++++++++ include/linux/bpf.h | 1 - include/linux/filter.h | 8 ++++++++ kernel/bpf/core.c | 29 ++++++++++++----------------- 4 files changed, 45 insertions(+), 18 deletions(-)