From f6a95e490afbdcfa22f586d704c53a6a45172986 Mon Sep 17 00:00:00 2001 From: Andrii Nakryiko Date: Tue, 25 Apr 2023 16:49:08 -0700 Subject: [PATCH] bpf: fix mark_all_scalars_precise use in mark_chain_precision When precision backtracking bails out due to some unsupported sequence of instructions (e.g., stack access through register other than r10), we need to mark all SCALAR registers as precise to be safe. Currently, though, we mark SCALARs precise only starting from the state we detected unsupported condition, which could be one of the parent states of the actual current state. This will leave some registers potentially not marked as precise, even though they should. So make sure we start marking scalars as precise from current state (env->cur_state). Further, we don't currently detect a situation when we end up with some stack slots marked as needing precision, but we ran out of available states to find the instructions that populate those stack slots. This is akin the `i >= func->allocated_stack / BPF_REG_SIZE` check and should be handled similarly by falling back to marking all SCALARs precise. Add this check when we run out of states. Signed-off-by: Andrii Nakryiko --- kernel/bpf/verifier.c | 16 +++++++++++++--- tools/testing/selftests/bpf/verifier/precise.c | 9 +++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index f797b702ebe0..be7013d87390 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3823,7 +3823,7 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int frame, int r err = backtrack_insn(env, i, bt); } if (err == -ENOTSUPP) { - mark_all_scalars_precise(env, st); + mark_all_scalars_precise(env, env->cur_state); bt_reset(bt); return 0; } else if (err) { @@ -3885,7 +3885,7 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int frame, int r * fp-8 and it's "unallocated" stack space. * In such case fallback to conservative. */ - mark_all_scalars_precise(env, st); + mark_all_scalars_precise(env, env->cur_state); bt_reset(bt); return 0; } @@ -3914,11 +3914,21 @@ static int __mark_chain_precision(struct bpf_verifier_env *env, int frame, int r } if (bt_bitcnt(bt) == 0) - break; + return 0; last_idx = st->last_insn_idx; first_idx = st->first_insn_idx; } + + /* if we still have requested precise regs or slots, we missed + * something (e.g., stack access through non-r10 register), so + * fallback to marking all precise + */ + if (bt_bitcnt(bt) != 0) { + mark_all_scalars_precise(env, env->cur_state); + bt_reset(bt); + } + return 0; } diff --git a/tools/testing/selftests/bpf/verifier/precise.c b/tools/testing/selftests/bpf/verifier/precise.c index 9ef1b74044c9..5db30a2d134d 100644 --- a/tools/testing/selftests/bpf/verifier/precise.c +++ b/tools/testing/selftests/bpf/verifier/precise.c @@ -159,8 +159,9 @@ mark_precise: frame0: regs(0x10)=r4 stack(0x0)= before 3\ mark_precise: frame0: regs(0x0)= stack(0x1)=-8 before 2\ mark_precise: frame0: falling back to forcing all scalars precise\ + force_precise: frame0: forcing r0 to be precise\ mark_precise: frame0: last_idx 5 first_idx 5\ - mark_precise: frame0: parent state regs(0x1)=r0 stack(0x0)=:", + mark_precise: frame0: parent state regs(0x0)= stack(0x0)=:", .result = VERBOSE_ACCEPT, .retval = -1, }, @@ -187,10 +188,10 @@ mark_precise: frame0: falling back to forcing all scalars precise\ force_precise: frame0: forcing r0 to be precise\ force_precise: frame0: forcing r0 to be precise\ + force_precise: frame0: forcing r0 to be precise\ + force_precise: frame0: forcing r0 to be precise\ mark_precise: frame0: last_idx 6 first_idx 6\ - mark_precise: frame0: parent state regs(0x1)=r0 stack(0x0)=:\ - mark_precise: frame0: last_idx 5 first_idx 3\ - mark_precise: frame0: regs(0x1)=r0 stack(0x0)= before 5", + mark_precise: frame0: parent state regs(0x0)= stack(0x0)=:", .result = VERBOSE_ACCEPT, .retval = -1, },