On Thu, 2025-05-01 at 09:35 +0200, Luis Gerhorst wrote:
This is required to catch the errors later and fall back to a nospec if on a speculative path.
Eliminate the regs variable as it is only used once and insn_idx is not modified in-between the definition and usage.
Still pass insn simply to match the other check_*() functions. As Eduard points out [1], insn is assumed to correspond to env->insn_idx in many places (e.g, __check_reg_arg()).
Move code into do_check_insn(), replace
- "continue" with "return 0" after modifying insn_idx
- "goto process_bpf_exit" with "return PROCESS_BPF_EXIT"
- "do_print_state = " with "*do_print_state = "
[1] https://lore.kernel.org/all/293dbe3950a782b8eb3b87b71d7a967e120191fd.camel@g...
Signed-off-by: Luis Gerhorst luis.gerhorst@fau.de Acked-by: Henriette Herzog henriette.herzog@rub.de Cc: Maximilian Ott ott@cs.fau.de Cc: Milan Stephan milan.stephan@fau.de
Except two notes below, I think this patch looks good. Thank you, this is a good refactoring.
[...]
+static int do_check_insn(struct bpf_verifier_env *env, struct bpf_insn *insn,
bool *do_print_state)+{
[...]
- } else if (class == BPF_ST) {
enum bpf_reg_type dst_reg_type;if (BPF_MODE(insn->code) != BPF_MEM ||insn->src_reg != BPF_REG_0) {verbose(env, "BPF_ST uses reserved fields\n");return -EINVAL;}/* check src operand */err = check_reg_arg(env, insn->dst_reg, SRC_OP);if (err)return err;dst_reg_type = cur_regs(env)[insn->dst_reg].type;
Implicitly relying on `insn == &env->prog->insnsi[env->cur_idx]` is weird. Still think that `insn` parameter should be dropped and computed inside this function instead.
/* check that memory (dst_reg + off) is writeable */err = check_mem_access(env, env->insn_idx, insn->dst_reg,insn->off, BPF_SIZE(insn->code),BPF_WRITE, -1, false, false);if (err)return err;err = save_aux_ptr_type(env, dst_reg_type, false);if (err)return err;- } else if (class == BPF_JMP || class == BPF_JMP32) {
[...]
} else if (opcode == BPF_EXIT) {if (BPF_SRC(insn->code) != BPF_K ||insn->imm != 0 ||insn->src_reg != BPF_REG_0 ||insn->dst_reg != BPF_REG_0 ||class == BPF_JMP32) {verbose(env, "BPF_EXIT uses reserved fields\n");return -EINVAL;}+process_bpf_exit_full:
Nit: since we are refactoring I'd extract this as a function instead of goto.
/* We must do check_reference_leak here before* prepare_func_exit to handle the case when* state->curframe > 0, it may be a callback function,* for which reference_state must match caller reference* state when it exits.*/err = check_resource_leak(env, exception_exit, !env->cur_state->curframe,"BPF_EXIT instruction in main prog");if (err)return err;/* The side effect of the prepare_func_exit which is* being skipped is that it frees bpf_func_state.* Typically, process_bpf_exit will only be hit with* outermost exit. copy_verifier_state in pop_stack will* handle freeing of any extra bpf_func_state left over* from not processing all nested function exits. We* also skip return code checks as they are not needed* for exceptional exits.*/if (exception_exit)return PROCESS_BPF_EXIT;if (env->cur_state->curframe) {/* exit from nested function */err = prepare_func_exit(env, &env->insn_idx);if (err)return err;*do_print_state = true;return 0;}err = check_return_code(env, BPF_REG_0, "R0");if (err)return err;return PROCESS_BPF_EXIT;
[...]