bpf: Use bpf_verifier_env buffers for reg_set_min_max

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>
Acked-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
Acked-by: Mykyta Yatsenko <yatsenko@meta.com>
Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Link: https://lore.kernel.org/r/260b0270052944a420e1c56e6a92df4d43cadf03.1775142354.git.paul.chaignon@gmail.com
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 090aa26d1c9848d0a9fbfa6461c579877da4355b..b129e0aaee2049ccadf2539ea2772fd1011296c5 100644 (file)
--- 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;
        /* 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 760cd137bf3fc4993af4a6efa95db267cb04b34a..15defae1d7edd3b18615bd179aca4fa3e69d3d9e 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17253,10 +17253,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;
@@ -17265,30 +17261,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;
 }
 
@@ -17672,6 +17661,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
@@ -17736,27 +17729,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)) {