Re: PPC jit and pseudo_btf_id

[Yauheni Kaliuta <ykaliuta@redhat.com>]

Hi!

On Tue, Dec 14, 2021 at 2:11 PM Naveen N. Rao
<naveen.n.rao@linux.vnet.ibm.com> wrote:
>
> Hi Yauheni,
>
>
> Yauheni Kaliuta wrote:
> > Hi!
> >
> > I get kernel oops on my setup due to unresolved pseudo_btf_id for
> > ld_imm64 (see 4976b718c355 ("bpf: Introduce pseudo_btf_id")) for
> > example for `test_progs -t for_each/hash_map` where callback
> > address is passed to a bpf helper:
> >
> >
> > [  425.853991] kernel tried to execute user page (100000014) - exploit attempt? (uid: 0)
> > [  425.854173] BUG: Unable to handle kernel instruction fetch
> > [  425.854255] Faulting instruction address: 0x100000014

[...]

>
> Thanks for the problem report. I noticed this recently and have prepared
> a fix as part of a larger patchset.
>

Ah, cool! That was  actually my main question, do I miss the fix somewhere :)

> >
> > The simple patch fixes it for me:
> >
> > diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> > index 90ce75f0f1e2..554c26480387 100644
> > --- a/arch/powerpc/net/bpf_jit_comp.c
> > +++ b/arch/powerpc/net/bpf_jit_comp.c
> > @@ -201,8 +201,7 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
> >                */
> >               bpf_jit_fixup_subprog_calls(fp, code_base, &cgctx, addrs);
> >
> > -             /* There is no need to perform the usual passes. */
> > -             goto skip_codegen_passes;
> > +             /* Due to pseudo_btf_id resolving, regenerate */
> >       }
> >
> >       /* Code generation passes 1-2 */
> > @@ -222,7 +221,6 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
> >                               proglen - (cgctx.idx * 4), cgctx.seen);
> >       }
> >
> > -skip_codegen_passes:
> >       if (bpf_jit_enable > 1)
> >               /*
> >                * Note that we output the base address of the code_base
> >
> >
> >
> > Do I miss something?
>
> The problem with the above approach is that we generate variable number
> of instructions for certain BPF instructions and so, unless we reserve
> space for maximum number of powerpc instructions beforehand, we risk
> writing past the end of the allocated buffer.

Yes, agree.

> Can you please check if the below patch fixes the issue for you?

It does, thanks!

I was actually thinking later about something similar and I wonder
about naming. Should the function be renamed to more generic, and is
it really for func_addr only or can be any generic value?

>
>
> Thanks,
> Naveen
>
>
> ---
> diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
> index 463f99ecaa459e..e16d421ce22a65 100644
> --- a/arch/powerpc/net/bpf_jit_comp.c
> +++ b/arch/powerpc/net/bpf_jit_comp.c
> @@ -66,7 +66,15 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
>                          * of the JITed sequence remains unchanged.
>                          */
>                         ctx->idx = tmp_idx;
> +               } else if (insn[i].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> +                       func_addr = ((u64)(u32) insn[i].imm) | (((u64)(u32) insn[i+1].imm) << 32);
> +                       tmp_idx = ctx->idx;
> +                       ctx->idx = addrs[i] / 4;
> +                       PPC_LI64(b2p[insn[i].dst_reg], func_addr);
> +                       ctx->idx = tmp_idx;
> +                       i++;
>                 }
> +
>         }
>
>         return 0;
> diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
> index 74b465cc7a84d0..4d3973cd78b46f 100644
> --- a/arch/powerpc/net/bpf_jit_comp64.c
> +++ b/arch/powerpc/net/bpf_jit_comp64.c
> @@ -324,6 +324,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>                 u64 imm64;
>                 u32 true_cond;
>                 u32 tmp_idx;
> +               int j;
>
>                 /*
>                  * addrs[] maps a BPF bytecode address into a real offset from
> @@ -858,7 +859,11 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
>                                     (((u64)(u32) insn[i+1].imm) << 32);
>                         /* Adjust for two bpf instructions */
>                         addrs[++i] = ctx->idx * 4;
> +                       tmp_idx = ctx->idx;
>                         PPC_LI64(dst_reg, imm64);
> +                       /* padding to allow full 5 instructions for later patching */
> +                       for (j = ctx->idx - tmp_idx; j < 5; j++)
> +                               EMIT(PPC_RAW_NOP());
>                         break;
>
>                 /*
>


-- 
WBR, Yauheni