Re: [PATCH bpf 1/2] bpf: reject bpf-to-bpf call with large offset in interpreter

[Puranjay Mohan <puranjay@kernel.org>]

Yazhou Tang <tangyazhou@zju.edu.cn> writes:

> From: Yazhou Tang <tangyazhou518@outlook.com>
>
> Currently, the BPF instruction set allows bpf-to-bpf call (or internal call,
> pseudo call) to use a 32-bit `imm` field to represent the relative jump offset.
>
> However, when JIT is disabled or falls back to interpreter, the verifier
> invokes `bpf_patch_call_args()` to rewrite the call instruction. In this
> function, the 32-bit `imm` is downcasted to s16 and stored in the `off` field.
>
> ```c
> void bpf_patch_call_args(struct bpf_insn *insn, u32 stack_depth)
> {
> 	stack_depth = max_t(u32, stack_depth, 1);
> 	insn->off = (s16) insn->imm;
> 	insn->imm = interpreters_args[(round_up(stack_depth, 32) / 32) - 1] -
> 		__bpf_call_base_args;
> 	insn->code = BPF_JMP | BPF_CALL_ARGS;
> }
> ```
>
> If the original `imm` exceeds the s16 range (i.e., jump offset > 32KB),
> this downcast silently truncates the offset, resulting in an incorrect
> call target.
>
> Fix this by explicitly checking the offset boundary in `fixup_call_args()`.
> If the offset is out of range, reject the program with -EINVAL and emit
> a clear verifier log message.
>
> Co-developed-by: Tianci Cao <ziye@zju.edu.cn>
> Signed-off-by: Tianci Cao <ziye@zju.edu.cn>
> Co-developed-by: Shenghao Yuan <shenghaoyuan0928@163.com>
> Signed-off-by: Shenghao Yuan <shenghaoyuan0928@163.com>
> Signed-off-by: Yazhou Tang <tangyazhou518@outlook.com>
> ---
>
> We also evaluated a potentially more fundamental fix: patching the instruction
> stream in `do_misc_fixups()` to mov the 32-bit `imm` into an auxiliary register
> (`BPF_REG_AX`) prior to the call, and then utilizing `BPF_REG_AX` to resolve
> the jump target in the interpreter.
>
> To avoid degrading performance, this patching must be strictly confined to
> cases where JIT is disabled. However, reliably determining the final JIT
> execution status during the verification stage is not easy. Therefore, we
> opted for the simplest and safest fix for now.
>
>  kernel/bpf/verifier.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index df22bfc572e2..0ee8a3333193 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -23109,6 +23109,12 @@ static int fixup_call_args(struct bpf_verifier_env *env)
>  
>  		if (!bpf_pseudo_call(insn))
>  			continue;
> +
> +		if (insn->imm < S16_MIN || insn->imm > S16_MAX) {
> +			verbose(env, "bpf-to-bpf call offset out of range for interpreter\n");
> +			return -EINVAL;
> +		}
> +

Thanks for the patch. The interpreter truncation fix looks correct, but there's
a similar truncation bug in the JIT success path that should be addressed as
well.

In jit_subprogs(), the "make interpreter insns consistent for dump" fixup at
the end has the same s16 truncation problem:

insn->off = env->insn_aux_data[i].call_imm;
subprog = find_subprog(env, i + insn->off + 1);
insn->imm = subprog;

When call_imm exceeds s16 range, insn->off truncates it, and find_subprog()
looks up the wrong instruction index. In my testing with your selftest,
find_subprog() returns -ENOENT (-2), so insn->imm = -2. This causes bpftool to
compute the call target as __bpf_call_base + (-2), which shows a bogus address
in the xlated dump instead of the actual subprog symbol:

   1: (85) call pc+9#bpf_prog_115951674716e036_padding_subprog
   2: (85) call pc+3402#0xffff80008026d276

The 0xffff80008026d276 is __bpf_call_base - 2, not target_subprog.  This also
leaks the address of __bpf_call_base to userspace.

The fix is to use call_imm directly for the find_subprog() lookup instead of
reading back from the truncated insn->off:

insn->off = env->insn_aux_data[i].call_imm;
subprog = find_subprog(env, i + env->insn_aux_data[i].call_imm + 1);
insn->imm = subprog;

this still leaves the ->off to be wrong, but that is fine in my opinion.

Could you fold this into the series as well?