Re: [PATCH v2 bpf-next 2/6] bpf: Use bpf_verifier_env buffers for reg_set_min_max

[Mykyta Yatsenko <mykyta.yatsenko5@gmail.com>]

Paul Chaignon <paul.chaignon@gmail.com> writes:

> In a subsequent patch, the regs_refine_cond_op and reg_bounds_sync
> functions will be called in is_branch_taken instead of reg_set_min_max,
> to simulate each branch's outcome. Since they will run before we branch
> out, these two functions will need to work on temporary registers for
> the two branches.
>
> This refactoring patch prepares for that change, by introducing the
> temporary registers on bpf_verifier_env and using them in
> reg_set_min_max.
>
> This change also allows us to save one fake_reg slot as we don't need to
> allocate an additional temporary buffer in case of a BPF_K condition.
>
> Finally, you may notice that this patch removes the check for
> "false_reg1 == false_reg2" in reg_set_min_max. That check was introduced
> in commit d43ad9da8052 ("bpf: Skip bounds adjustment for conditional
> jumps on same scalar register") to avoid an invariant violation. Given
> that "env->false_reg1 == env->false_reg2" doesn't make sense and
> invariant violations are addressed in a subsequent commit, this patch
> just removes the check.
>
> Suggested-by: Eduard Zingerman <eddyz87@gmail.com>
> Co-developed-by: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
> Signed-off-by: Harishankar Vishwanathan <harishankar.vishwanathan@gmail.com>
> Signed-off-by: Paul Chaignon <paul.chaignon@gmail.com>
> ---
>  include/linux/bpf_verifier.h |  4 ++-
>  kernel/bpf/verifier.c        | 64 +++++++++++++-----------------------
>  2 files changed, 26 insertions(+), 42 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 090aa26d1c98..b129e0aaee20 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -837,7 +837,9 @@ struct bpf_verifier_env {
>  	u64 scratched_stack_slots;
>  	u64 prev_log_pos, prev_insn_print_pos;
>  	/* buffer used to temporary hold constants as scalar registers */
> -	struct bpf_reg_state fake_reg[2];
> +	struct bpf_reg_state fake_reg[1];
> +	/* buffers used to save updated reg states while simulating branches */
> +	struct bpf_reg_state true_reg1, true_reg2, false_reg1, false_reg2;
I can see Eduard suggested to store this in env to reduce stack usage by
the verifier. The rest of the refactoring looks like a correct
alignment. The only difference with the base version is removal of the
if (false_reg1 == false_reg2) condition, which is explained in the
commit message.
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
>  	/* buffer used to generate temporary string representations,
>  	 * e.g., in reg_type_str() to generate reg_type string
>  	 */
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b638ab841c10..fbc29fb96a60 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -17184,10 +17184,6 @@ static void regs_refine_cond_op(struct bpf_reg_state *reg1, struct bpf_reg_state
>   * but we don't support that right now.
>   */
>  static int reg_set_min_max(struct bpf_verifier_env *env,
> -			   struct bpf_reg_state *true_reg1,
> -			   struct bpf_reg_state *true_reg2,
> -			   struct bpf_reg_state *false_reg1,
> -			   struct bpf_reg_state *false_reg2,
>  			   u8 opcode, bool is_jmp32)
>  {
>  	int err;
> @@ -17196,30 +17192,23 @@ static int reg_set_min_max(struct bpf_verifier_env *env,
>  	 * variable offset from the compare (unless they were a pointer into
>  	 * the same object, but we don't bother with that).
>  	 */
> -	if (false_reg1->type != SCALAR_VALUE || false_reg2->type != SCALAR_VALUE)
> -		return 0;
> -
> -	/* We compute branch direction for same SCALAR_VALUE registers in
> -	 * is_scalar_branch_taken(). For unknown branch directions (e.g., BPF_JSET)
> -	 * on the same registers, we don't need to adjust the min/max values.
> -	 */
> -	if (false_reg1 == false_reg2)
> +	if (env->false_reg1.type != SCALAR_VALUE || env->false_reg2.type != SCALAR_VALUE)
>  		return 0;
>  
>  	/* fallthrough (FALSE) branch */
> -	regs_refine_cond_op(false_reg1, false_reg2, rev_opcode(opcode), is_jmp32);
> -	reg_bounds_sync(false_reg1);
> -	reg_bounds_sync(false_reg2);
> +	regs_refine_cond_op(&env->false_reg1, &env->false_reg2, rev_opcode(opcode), is_jmp32);
> +	reg_bounds_sync(&env->false_reg1);
> +	reg_bounds_sync(&env->false_reg2);
>  
>  	/* jump (TRUE) branch */
> -	regs_refine_cond_op(true_reg1, true_reg2, opcode, is_jmp32);
> -	reg_bounds_sync(true_reg1);
> -	reg_bounds_sync(true_reg2);
> -
> -	err = reg_bounds_sanity_check(env, true_reg1, "true_reg1");
> -	err = err ?: reg_bounds_sanity_check(env, true_reg2, "true_reg2");
> -	err = err ?: reg_bounds_sanity_check(env, false_reg1, "false_reg1");
> -	err = err ?: reg_bounds_sanity_check(env, false_reg2, "false_reg2");
> +	regs_refine_cond_op(&env->true_reg1, &env->true_reg2, opcode, is_jmp32);
> +	reg_bounds_sync(&env->true_reg1);
> +	reg_bounds_sync(&env->true_reg2);
> +
> +	err = reg_bounds_sanity_check(env, &env->true_reg1, "true_reg1");
> +	err = err ?: reg_bounds_sanity_check(env, &env->true_reg2, "true_reg2");
> +	err = err ?: reg_bounds_sanity_check(env, &env->false_reg1, "false_reg1");
> +	err = err ?: reg_bounds_sanity_check(env, &env->false_reg2, "false_reg2");
>  	return err;
>  }
>  
> @@ -17597,6 +17586,10 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  	}
>  
>  	is_jmp32 = BPF_CLASS(insn->code) == BPF_JMP32;
> +	copy_register_state(&env->false_reg1, dst_reg);
> +	copy_register_state(&env->false_reg2, src_reg);
> +	copy_register_state(&env->true_reg1, dst_reg);
> +	copy_register_state(&env->true_reg2, src_reg);
>  	pred = is_branch_taken(dst_reg, src_reg, opcode, is_jmp32);
>  	if (pred >= 0) {
>  		/* If we get here with a dst_reg pointer type it is because
> @@ -17661,27 +17654,16 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env,
>  		return PTR_ERR(other_branch);
>  	other_branch_regs = other_branch->frame[other_branch->curframe]->regs;
>  
> -	if (BPF_SRC(insn->code) == BPF_X) {
> -		err = reg_set_min_max(env,
> -				      &other_branch_regs[insn->dst_reg],
> -				      &other_branch_regs[insn->src_reg],
> -				      dst_reg, src_reg, opcode, is_jmp32);
> -	} else /* BPF_SRC(insn->code) == BPF_K */ {
> -		/* reg_set_min_max() can mangle the fake_reg. Make a copy
> -		 * so that these are two different memory locations. The
> -		 * src_reg is not used beyond here in context of K.
> -		 */
> -		memcpy(&env->fake_reg[1], &env->fake_reg[0],
> -		       sizeof(env->fake_reg[0]));
> -		err = reg_set_min_max(env,
> -				      &other_branch_regs[insn->dst_reg],
> -				      &env->fake_reg[0],
> -				      dst_reg, &env->fake_reg[1],
> -				      opcode, is_jmp32);
> -	}
> +	err = reg_set_min_max(env, opcode, is_jmp32);
>  	if (err)
>  		return err;
>  
> +	copy_register_state(dst_reg, &env->false_reg1);
> +	copy_register_state(src_reg, &env->false_reg2);
> +	copy_register_state(&other_branch_regs[insn->dst_reg], &env->true_reg1);
> +	if (BPF_SRC(insn->code) == BPF_X)
> +		copy_register_state(&other_branch_regs[insn->src_reg], &env->true_reg2);
> +
>  	if (BPF_SRC(insn->code) == BPF_X &&
>  	    src_reg->type == SCALAR_VALUE && src_reg->id &&
>  	    !WARN_ON_ONCE(src_reg->id != other_branch_regs[insn->src_reg].id)) {
> -- 
> 2.43.0