Re: [RFC PATCH bpf-next v1 04/11] bpf: Give the inlined bpf_loop a pad

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: inline_bpf_loop() emits pad to unseen target deleted by dead code removal
- [High] bpf: inline_bpf_loop() leaves orphaned unwind_goto after LDX_MEM when !pad
--

commit f87a8cc4d4bc638d4f9d4773f7657f7aec4d7868
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Give the inlined bpf_loop a pad

This commit updates inline_bpf_loop() to emit an unwind landing pad when the
callback subprogram can unwind and an unwind_goto instruction follows the
bpf_loop() call. The inlined pad restores callee-saved registers R6-R8, jumps
to the caller's landing pad, and rewrites the original unwind_goto after the
bpf_loop() call into a no-op jump.

> diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
> index f062ef7dd943d..94a379c94c675 100644
> --- a/kernel/bpf/fixups.c
> +++ b/kernel/bpf/fixups.c
> @@ -2836,6 +2836,15 @@ int bpf_do_misc_fixups(struct bpf_verifier_env *env)
>  	return 0;
>  }
>  
> +/*
> + * When the callback can unwind and unwind_goto follows the call of bpf_loop,
> + * unwind_goto follows the call of the callback, and the pad restores r6-r8 and
> + * jumps to the pad after the call of bpf_loop with the r0 of the callback, which
> + * the verifier has checked with the state of that r0. The unwind_goto that
> + * followed the call of bpf_loop doesn't follow a call anymore and becomes a
> + * no-op jump. A loop of no rounds never runs its callback, so the verifier asked
> + * for no unwind_goto there, and the insn after the call is some other insn.
> + */
>  static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  					int position,
>  					s32 stack_base,
> @@ -2848,6 +2857,8 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	int reg_loop_max = BPF_REG_6;
>  	int reg_loop_cnt = BPF_REG_7;
>  	int reg_loop_ctx = BPF_REG_8;
> +	bool pad = env->subprog_info[callback_subprogno].can_unwind &&
> +		   bpf_is_unwind_goto(&env->prog->insnsi[position + 1]);

[Severity: High]
Can this leave a live BPF_JMP32_A instruction targeting a dead caller landing
pad that gets removed by bpf_opt_remove_dead_code(), leading to an
out-of-bounds read in addrs[] or ctx->offset[] during JIT compilation?

In inline_bpf_loop(), pad is enabled without checking whether the target of
the unwind_goto at position + 1 was actually visited during verification:

    caller_pad = position + 2 + env->prog->insnsi[position + 1].off;
    env->insn_aux_data[caller_pad].seen

The caller's landing pad can remain unseen while can_unwind and
bpf_is_unwind_goto(&env->prog->insnsi[position + 1]) are both true, for
example when bpf_loop() is called with 0 iterations (r1 == 0, which skips
push_callback_call() in check_helper_call() while still marking the call site
fit_for_inline in update_loop_inline_state()) or when unwind_exit in the
callback lies on a branch not taken for this call site.

In bpf_check(), bpf_optimize_bpf_loop() runs before
bpf_opt_hard_wire_dead_code_branches() and bpf_opt_remove_dead_code():

bpf_check()
  bpf_optimize_bpf_loop()
    inline_bpf_loop()
      bpf_patch_insn_data()
        adjust_insn_aux_data()
  bpf_opt_hard_wire_dead_code_branches()
  bpf_opt_remove_dead_code()
    verifier_remove_insns()
      bpf_remove_insns()
        bpf_adj_branches()
          bpf_adj_delta_to_off()

When bpf_patch_insn_data() patches the inlined loop, adjust_insn_aux_data()
marks all patched instructions in insn_buf as seen, including the inlined
pad and its trailing BPF_JMP32_A instruction, while the original unwind_goto
is overwritten with BPF_JMP_A(0).

When bpf_opt_hard_wire_dead_code_branches() runs next, it preserves the
inlined BPF_UNWIND_GOTO because its target (the inlined pad) is marked seen,
and ignores the BPF_JMP32_A instruction because it is an unconditional jump.

When bpf_opt_remove_dead_code() then removes the unseen caller landing pad
range [pos, end_old), bpf_adj_delta_to_off() only adjusts forward jumps when
curr + off + 1 >= end_old. Because BPF_JMP32_A targets caller_pad inside the
removed range [pos, end_old), its imm offset is not adjusted. If caller_pad
is at the end of a subprogram and preceded by other dead instructions (or
followed by a subprogram where bpf_do_misc_fixups() calls adjust_jmp_off()
for may_goto), BPF_JMP32_A jumps past the end of the subprogram, causing
do_jit() on x86-64 to read past addrs[] and bpf2a64_offset() on arm64 to read
past ctx->offset[].

Should pad also require env->insn_aux_data[position + 2 +
env->prog->insnsi[position + 1].off].seen to be true?

>  
>  	struct bpf_insn *insn_buf = env->insn_buf;
>  	struct bpf_prog *new_prog;
> @@ -2865,7 +2876,7 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	 */
>  	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JLE, BPF_REG_1, BPF_MAX_LOOPS, 2);
>  	insn_buf[cnt++] = BPF_MOV32_IMM(BPF_REG_0, -E2BIG);
> -	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 16);
> +	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, pad ? 22 : 16);
>  	/* spill R6, R7, R8 to use these as loop vars */
>  	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, r6_offset);
>  	insn_buf[cnt++] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_7, r7_offset);
> @@ -2877,17 +2888,20 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	/* loop header,
>  	 * if reg_loop_cnt >= reg_loop_max skip the loop body
>  	 */
> -	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, 5);
> +	insn_buf[cnt++] = BPF_JMP_REG(BPF_JGE, reg_loop_cnt, reg_loop_max, pad ? 6 : 5);
>  	/* callback call,
>  	 * correct callback offset would be set after patching
>  	 */
>  	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_1, reg_loop_cnt);
>  	insn_buf[cnt++] = BPF_MOV64_REG(BPF_REG_2, reg_loop_ctx);
>  	insn_buf[cnt++] = BPF_CALL_REL(0);
> +	if (pad)
> +		/* to the pad below when the callback left with unwind_exit */
> +		insn_buf[cnt++] = BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, BPF_UNWIND_GOTO, 7, 0);
>  	/* increment loop counter */
>  	insn_buf[cnt++] = BPF_ALU64_IMM(BPF_ADD, reg_loop_cnt, 1);
>  	/* jump to loop header if callback returned 0 */
> -	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, -6);
> +	insn_buf[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, pad ? -7 : -6);
>  	/* return value of bpf_loop,
>  	 * set R0 to the number of iterations
>  	 */
> @@ -2896,12 +2910,35 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
>  	insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
> +	if (pad) {
> +		/* over the pad */
> +		insn_buf[cnt++] = BPF_JMP_IMM(BPF_JA, 0, 0, 4);
> +		/* the pad: r0 is what the callback unwound with */
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_6, BPF_REG_10, r6_offset);
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, r7_offset);
> +		insn_buf[cnt++] = BPF_LDX_MEM(BPF_DW, BPF_REG_8, BPF_REG_10, r8_offset);
> +		/*
> +		 * To the pad of the caller, where the unwind_goto that follows
> +		 * the patch goes. Its offset is known once the patch is in and
> +		 * the jumps across it are adjusted, see below.
> +		 */
> +		insn_buf[cnt++] = BPF_JMP32_A(0);
> +	}
>  
>  	*total_cnt = cnt;
>  	new_prog = bpf_patch_insn_data(env, position, insn_buf, cnt);
>  	if (!new_prog)
>  		return new_prog;
>  
> +	if (pad) {
> +		struct bpf_insn *unwind_goto = &new_prog->insnsi[position + cnt];
> +
> +		/* the jump of the pad, one insn before, takes the target of the unwind_goto */
> +		new_prog->insnsi[position + cnt - 1].imm = unwind_goto->off + 1;
> +		/* and the unwind_goto after the call of bpf_loop follows no call anymore */
> +		*unwind_goto = BPF_JMP_A(0);
> +	}

[Severity: High]
What happens if bpf_is_unwind_goto(&new_prog->insnsi[position + cnt]) is true,
but pad is false (for example, when
env->subprog_info[callback_subprogno].can_unwind is false, or when the
caller's landing pad was not reached from this call site)?

In check_unwind_insns() and check_cb_unwind(), an unwind_goto instruction is
permitted after a bpf_loop() call even when the callback subprogram has
can_unwind == false:

kernel/bpf/verifier.c:check_cb_unwind() {
    if (!env->subprog_info[subprog].can_unwind)
        return 0;
    ...
}

When inline_bpf_loop() runs with pad == false, it replaces the bpf_loop()
call with 19 instructions ending in BPF_LDX_MEM(BPF_DW, BPF_REG_8,
BPF_REG_10, r8_offset), and the error branch BPF_JMP_IMM(BPF_JA, 0, 0, 16)
jumps directly to new_prog->insnsi[position + cnt].

Because the rewrite of *unwind_goto = BPF_JMP_A(0) is guarded by if (pad),
an unwind_goto following the bpf_loop() call is left in place when !pad. If
the landing pad targeted by that unwind_goto was marked seen via another
unwinding call in the same subprogram, bpf_opt_hard_wire_dead_code_branches()
sees:

    if (bpf_is_unwind_goto(insn) && aux_data[i + 1 + insn->off].seen)

and preserves the orphaned unwind_goto instruction. That leaves
BPF_UNWIND_GOTO immediately following BPF_LDX_MEM instead of a call
instruction, causing the x86-64 and arm64 JITs (which assume insn[i - 1] is a
call instruction) to emit bogus EX_TYPE_BPF exception table entries and
unwind stubs keyed inside the BPF_LDX_MEM instruction.

Should new_prog->insnsi[position + cnt] be rewritten to BPF_JMP_A(0) whenever
bpf_is_unwind_goto(&new_prog->insnsi[position + cnt]) is true, regardless of
whether pad is true?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793147364.git.memxor@gmail.com?part=4