On Tue, Jul 12, 2022 at 3:09 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Tue, Jul 5, 2022 at 5:26 PM Song Liu <song@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Reported-by: syzbot+87f65c75f4a72db05445@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Signed-off-by: Song Liu <song@xxxxxxxxxx>
> > ---
> > 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
