Hi!
On Tue, Dec 14, 2021 at 2:11 PM Naveen N. Rao
<naveen.n.rao@xxxxxxxxxxxxxxxxxx> 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
