RE: [PATCH 1/4] bpf/arm64: fix zero-return branch in multi-exit programs

[Marat Khalili <marat.khalili@huawei.com>]

> If a JIT'd BPF program has more than one exit,
> the branch to the epilogue can be backwards.
> 
> The current code assumed it is always forward:
> emit_return_zero_if_src_zero() held the offset in an unsigned uint16_t,
> so a backward (negative) offset wrapped to a large positive value and
> branch off the end of the program, faulting at run time.
> 
> This was masked until now: the only test with this shape, test_ld_mbuf,
> needs BPF_ABS/BPF_IND which the arm64 JIT did not implement, so it never
> ran under the JIT.  The x86 JIT is unaffected because emit_epilog() keeps a
> single exit (st->exit.off) reached from later exits and the divide-by-zero
> check via a signed absolute jump (emit_abs_jcc), so direction does not
> matter.
> 
> Use a signed offset; emit_b() already sign-extends imm26 correctly.

This line is unclear to me, but that's not the main issue.

> Fixes: 111e2a747a4f ("bpf/arm: add basic arithmetic operations")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/bpf/bpf_jit_arm64.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/bpf/bpf_jit_arm64.c b/lib/bpf/bpf_jit_arm64.c
> index a04ef33a9c..099822e9f1 100644
> --- a/lib/bpf/bpf_jit_arm64.c
> +++ b/lib/bpf/bpf_jit_arm64.c
> @@ -957,7 +957,7 @@ static void
>  emit_return_zero_if_src_zero(struct a64_jit_ctx *ctx, bool is64, uint8_t src)
>  {
>  	uint8_t r0 = ebpf_to_a64_reg(ctx, EBPF_REG_0);
> -	uint16_t jump_to_epilogue;
> +	int32_t jump_to_epilogue;
> 
>  	emit_cbnz(ctx, is64, src, 3);
>  	emit_mov_imm(ctx, is64, r0, 0);
> --
> 2.53.0

In the very next line the offset is calculated as follows though:

    jump_to_epilogue = (ctx->program_start + ctx->program_sz) - ctx->idx;

From its appearance, it can never be negative. However, ctx->program_sz is set
in emit_epilogue which is issued on every BPF_EXIT, so mid-generation it
contains the location of the last issued epilogue (including possibly previous
pass), not the program size. With this interpretation the code technically
works, but I'd suggest either renaming program_sz to something like
epilogue_offset, or making sure it is only set to the actual program size (sans
prologue and epilogue). In the latter case the jump offset will never be
negative, although the change to int32_t is still helpful in extending the
range of supported programs from 2**16 instructions. Since only 26 signed bits
are actually available maybe some check or assert is also warranted.