arm64 jit ctx.offset[-1] access

arm64 jit ctx.offset[-1] access

[Yauheni Kaliuta]

Hi!

Looks like my first message did not reach the list, resending:

I have a question about arm64 bpf jit implementation.

The problem I observe is "taken loop with back jump to 1st insn"
verifier test, the subprogram is:

BPF_ALU64_REG(BPF_ADD, BPF_REG_2, BPF_REG_1),
BPF_ALU64_IMM(BPF_SUB, BPF_REG_1, 1),
BPF_JMP_IMM(BPF_JNE, BPF_REG_1, 0, -3),
BPF_MOV64_REG(BPF_REG_0, BPF_REG_2),
BPF_EXIT_INSN(),

Jitting the program causes invocation of bpf2a64_offset(-1, 2, ctx)
from
        jmp_offset = bpf2a64_offset(i + off, i, ctx);

which does ctx->offset[-1] then (and works by accident when it
returns 0).

As far as I see, the offset[] keeps actually offsets of the next
instruction:

                ret = build_insn(insn, ctx, extra_pass);
                if (ret > 0) {
                        i++;
                        if (ctx->image == NULL)
                                ctx->offset[i] = ctx->idx;
                        continue;
                }
                if (ctx->image == NULL)
                        ctx->offset[i] = ctx->idx;


ctx->idx is updated by build_insn() already.

How is that supposed to work?

-- 
WBR, Yauheni

Re: arm64 jit ctx.offset[-1] access

[Luke Nelson]

Hi Yauheni,

Thanks for the report!

>
> Jitting the program causes invocation of bpf2a64_offset(-1, 2, ctx)
> from
>         jmp_offset = bpf2a64_offset(i + off, i, ctx);
>
> which does ctx->offset[-1] then (and works by accident when it
> returns 0).
>

This definitely looks like a bug to me, I ran your test program and
printed out the values of bpf_to in bpf2a64_offset and it is being called
with bpf_to = -1.

One way to fix this is to do something similar to what the RISC-V JITs
do here, by checking for the < 0 case explicitly:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/arch/riscv/net/bpf_jit.h?h=v5.8.8#n145

I imagine it would look something like the following:

--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -146,11 +146,11 @@ static inline void emit_addr_mov_i64(const int
reg, const u64 val,
 static inline int bpf2a64_offset(int bpf_to, int bpf_from,
                                 const struct jit_ctx *ctx)
 {
-       int to = ctx->offset[bpf_to];
-       /* -1 to account for the Branch instruction */
-       int from = ctx->offset[bpf_from] - 1;
+       int to = (bpf_to >= 0) ? ctx->offset[bpf_to] : 0;
+       int from = (bpf_from >= 0) ? ctx->offset[bpf_from] : 0;

-       return to - from;
+       /* -1 to account for the Branch instruction. */
+       return to - (from - 1);
 }

Anybody else have any thoughts? I can turn around and submit this as an
actual patch if it seems reasonable to others.

- Luke