Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux

bpf: fix precision backtracking instruction iteration

Fix an edge case in __mark_chain_precision() which prematurely stops
backtracking instructions in a state if it happens that state's first
and last instruction indexes are the same. This situations doesn't
necessarily mean that there were no instructions simulated in a state,
but rather that we starting from the instruction, jumped around a bit,
and then ended up at the same instruction before checkpointing or
marking precision.

To distinguish between these two possible situations, we need to consult
jump history. If it's empty or contain a single record "bridging" parent
state and first instruction of processed state, then we indeed
backtracked all instructions in this state. But if history is not empty,
we are definitely not done yet.

Move this logic inside get_prev_insn_idx() to contain it more nicely.
Use -ENOENT return code to denote "we are out of instructions"
situation.

This bug was exposed by verifier_loop1.c's bounded_recursion subtest, once
the next fix in this patch set is applied.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>
Fixes: b5dc0163d8fd ("bpf: precise scalar_value tracking")
Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
Link: https://lore.kernel.org/r/20231110002638.4168352-3-andrii@kernel.org
Signed-off-by: Alexei Starovoitov <ast@kernel.org>

authored by

Andrii Nakryiko and committed by
Alexei Starovoitov
4bb7ea94 3feb263b

+19 -2
+19 -2
kernel/bpf/verifier.c
··· 3516 3516 3517 3517 /* Backtrack one insn at a time. If idx is not at the top of recorded 3518 3518 * history then previous instruction came from straight line execution. 3519 + * Return -ENOENT if we exhausted all instructions within given state. 3520 + * 3521 + * It's legal to have a bit of a looping with the same starting and ending 3522 + * insn index within the same state, e.g.: 3->4->5->3, so just because current 3523 + * instruction index is the same as state's first_idx doesn't mean we are 3524 + * done. If there is still some jump history left, we should keep going. We 3525 + * need to take into account that we might have a jump history between given 3526 + * state's parent and itself, due to checkpointing. In this case, we'll have 3527 + * history entry recording a jump from last instruction of parent state and 3528 + * first instruction of given state. 3519 3529 */ 3520 3530 static int get_prev_insn_idx(struct bpf_verifier_state *st, int i, 3521 3531 u32 *history) 3522 3532 { 3523 3533 u32 cnt = *history; 3534 + 3535 + if (i == st->first_insn_idx) { 3536 + if (cnt == 0) 3537 + return -ENOENT; 3538 + if (cnt == 1 && st->jmp_history[0].idx == i) 3539 + return -ENOENT; 3540 + } 3524 3541 3525 3542 if (cnt && st->jmp_history[cnt - 1].idx == i) { 3526 3543 i = st->jmp_history[cnt - 1].prev_idx; ··· 4418 4401 * Nothing to be tracked further in the parent state. 4419 4402 */ 4420 4403 return 0; 4421 - if (i == first_idx) 4422 - break; 4423 4404 subseq_idx = i; 4424 4405 i = get_prev_insn_idx(st, i, &history); 4406 + if (i == -ENOENT) 4407 + break; 4425 4408 if (i >= env->prog->len) { 4426 4409 /* This can happen if backtracking reached insn 0 4427 4410 * and there are still reg_mask or stack_mask