On Wed, Aug 21, 2024 at 5:30 PM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Wed, 2024-08-21 at 17:22 -0700, Alexei Starovoitov wrote:
>
> [...]
>
> > > + if (ops->gen_epilogue) {
> > > + epilogue_cnt = ops->gen_epilogue(epilogue_buf, env->prog,
> > > + -(subprogs[0].stack_depth + 8));
> > > + if (epilogue_cnt >= ARRAY_SIZE(epilogue_buf)) {
> > > + verbose(env, "bpf verifier is misconfigured\n");
> > > + return -EINVAL;
> > > + } else if (epilogue_cnt) {
> > > + /* Save the ARG_PTR_TO_CTX for the epilogue to use */
> > > + cnt = 0;
> > > + subprogs[0].stack_depth += 8;
> > > + insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_FP, BPF_REG_1,
> > > + -subprogs[0].stack_depth);
> > > + insn_buf[cnt++] = env->prog->insnsi[0];
> > > + new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
> > > + if (!new_prog)
> > > + return -ENOMEM;
> > > + env->prog = new_prog;
> > > + delta += cnt - 1;
> >
> > I suspect this is buggy.
> > See commit 5337ac4c9b80 ("bpf: Fix the corner case with may_goto and
> > jump to the 1st insn.")
>
> Actually, I was unable to figure out a counter example for this case,
> patching math seems to be correct, jump targets are just moved down.
> But let's see, maybe Martin can come up with something.
Something like
insn_0
...
r1 = 0
if rX == .. goto insn_0
this jmp will be rewritten to point to newly added *(u64*)(r10 - ..) = r1
so at run time it will overwrite that slot with zero and
epilogue will read zero from it instead of ctx.
