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.ca...@gmail.com/
>
> Signed-off-by: Luis Gerhorst
> Acked-by: Henriette Herzog
> Cc: Maximilian Ott
> Cc: Milan Stephan
> ---
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;
[...]
