On Fri, Oct 20, 2023 at 3:02 PM Tao Lyu <tao.lyu@xxxxxxx> wrote:
>
> There is another issue about the backtracking.
> When uploading the following program under privilege mode,
> the verifier reports a "verifier backtracking bug".
>
> 0: R1=ctx(off=0,imm=0) R10=fp0
> 0: (85) call pc+2
> caller:
> R10=fp0
> callee:
> frame1: R1=ctx(off=0,imm=0) R10=fp0
> 3: frame1:
> 3: (bf) r3 = r10 ; frame1: R3_w=fp0 R10=fp0
> 4: (bc) w0 = w10 ; frame1: R0_w=scalar(umax=4294967295,var_off=(0x0; 0xffffffff)) R10=fp0
> 5: (0f) r3 += r0
> mark_precise: frame1: last_idx 5 first_idx 0 subseq_idx -1
> mark_precise: frame1: regs=r0 stack= before 4: (bc) w0 = w10
> mark_precise: frame1: regs=r10 stack= before 3: (bf) r3 = r10
> mark_precise: frame1: regs=r10 stack= before 0: (85) call pc+2
> BUG regs 400
>
> This bug is manifested by the following check:
>
> if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS) {
> verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> WARN_ONCE(1, "verifier backtracking bug");
> return -EFAULT;
> }
>
> Since the verifier allows add operation on stack pointers,
> it shouldn't show this WARNING and reject the program.
>
> I fixed it by skipping the warning if it's privilege mode and only r10 is marked as precise.
>
See my reply to your other email. It would be nice if you can rewrite
your tests in inline assembly, it would be easier to follow and debug.
I think your fix is papering over the fact that we don't recognize
non-r10 stack access. Once we fix that, we shouldn't need extra hacks.
So let's solve the underlying problem first.
> Signed-off-by: Tao Lyu <tao.lyu@xxxxxxx>
> ---
> kernel/bpf/verifier.c | 4 +++-
> .../bpf/verifier/ret-without-checing-r10.c | 16 ++++++++++++++++
> 2 files changed, 19 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/bpf/verifier/ret-without-checing-r10.c
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index e777f50401b6..1ce80cdc4f1d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3495,6 +3495,7 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
> u32 dreg = insn->dst_reg;
> u32 sreg = insn->src_reg;
> u32 spi, i;
> + u32 reg_mask;
>
> if (insn->code == 0)
> return 0;
> @@ -3621,7 +3622,8 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
> * precise, r0 and r6-r10 or any stack slot in
> * the current frame should be zero by now
> */
> - if (bt_reg_mask(bt) & ~BPF_REGMASK_ARGS) {
> + reg_mask = bt_reg_mask(bt) & ~BPF_REGMASK_ARGS;
> + if (reg_mask && !((reg_mask == 1 << BPF_REG_10) && env->allow_ptr_leaks)) {
> verbose(env, "BUG regs %x\n", bt_reg_mask(bt));
> WARN_ONCE(1, "verifier backtracking bug");
> return -EFAULT;
> diff --git a/tools/testing/selftests/bpf/verifier/ret-without-checing-r10.c b/tools/testing/selftests/bpf/verifier/ret-without-checing-r10.c
> new file mode 100644
> index 000000000000..56e529cf922b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/verifier/ret-without-checing-r10.c
> @@ -0,0 +1,16 @@
> +{
> + "pointer arithmetic: when returning from subprog in priv, do not checking r10",
> + .insns = {
> + BPF_CALL_REL(2),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + BPF_MOV64_REG(BPF_REG_3, BPF_REG_10),
> + BPF_MOV32_REG(BPF_REG_0, BPF_REG_10),
> + BPF_ALU64_REG(BPF_ADD, BPF_REG_3, BPF_REG_0),
> + BPF_MOV64_IMM(BPF_REG_0, 0),
> + BPF_EXIT_INSN(),
> + },
> + .result = ACCEPT,
> + .result_unpriv = REJECT,
> + .errstr_unpriv = "loading/calling other bpf or kernel functions are allowed for CAP_BPF and CAP_SYS_ADMIN",
> +},
> --
> 2.25.1
>
