bpf: verifier: MOV64 don't mark dst reg unbounded

When check_alu_op() handles a BPF_MOV64 between two registers,
it calls check_reg_arg(DST_OP) on the dst register, marking it
as unbounded. If the src and dst register are the same, this
marks the src as unbounded, which can lead to unexpected errors
for further checks that rely on bounds info. For example:

	BPF_MOV64_IMM(BPF_REG_2, 0),
	BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
	BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
	BPF_MOV64_IMM(BPF_REG_0, 0),
	BPF_EXIT_INSN(),

Results in:

	"math between ctx pointer and register with unbounded
	min value is not allowed"

check_alu_op() now uses check_reg_arg(DST_OP_NO_MARK), and MOVs
that need to mark the dst register (MOVIMM, MOV32) do so.

Added a test case for MOV64 dst == src, and dst != src.

Signed-off-by: Arthur Fabre <afabre@cloudflare.com>
Acked-by: Edward Cree <ecree@solarflare.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 25e47c1958745caf9e547f18572c2d4ef51c2cfe..e948303a0ea8d3bdcafc43957cca469ce412f6de 100644 (file)
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3238,8 +3238,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
                        }
                }
 
-               /* check dest operand */
-               err = check_reg_arg(env, insn->dst_reg, DST_OP);
+               /* check dest operand, mark as required later */
+               err = check_reg_arg(env, insn->dst_reg, DST_OP_NO_MARK);
                if (err)
                        return err;
 
@@ -3265,6 +3265,8 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
                        /* case: R = imm
                         * remember the value we stored into this reg
                         */
+                       /* clear any state __mark_reg_known doesn't set */
+                       mark_reg_unknown(env, regs, insn->dst_reg);
                        regs[insn->dst_reg].type = SCALAR_VALUE;
                        if (BPF_CLASS(insn->code) == BPF_ALU64) {
                                __mark_reg_known(regs + insn->dst_reg,

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index f5f7bcc960465bd94367b135063152345b9a40bd..c582afba9d1f8eb2cfb02d7d00f1ecec95e3e506 100644 (file)
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -12332,6 +12332,32 @@ static struct bpf_test tests[] = {
                .result = REJECT,
                .errstr = "variable ctx access var_off=(0x0; 0x4)",
        },
+       {
+               "mov64 src == dst",
+               .insns = {
+                       BPF_MOV64_IMM(BPF_REG_2, 0),
+                       BPF_MOV64_REG(BPF_REG_2, BPF_REG_2),
+                       // Check bounds are OK
+                       BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
+                       BPF_MOV64_IMM(BPF_REG_0, 0),
+                       BPF_EXIT_INSN(),
+               },
+               .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+               .result = ACCEPT,
+       },
+       {
+               "mov64 src != dst",
+               .insns = {
+                       BPF_MOV64_IMM(BPF_REG_3, 0),
+                       BPF_MOV64_REG(BPF_REG_2, BPF_REG_3),
+                       // Check bounds are OK
+                       BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_2),
+                       BPF_MOV64_IMM(BPF_REG_0, 0),
+                       BPF_EXIT_INSN(),
+               },
+               .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+               .result = ACCEPT,
+       },
 };
 
 static int probe_filter_length(const struct bpf_insn *fp)