| From 973c7a0d8a38e675570c3336c664f5610bd4eb19 Mon Sep 17 00:00:00 2001 |
| From: Andrii Nakryiko <andrii@kernel.org> |
| Date: Thu, 9 Nov 2023 16:26:37 -0800 |
| Subject: bpf: fix precision backtracking instruction iteration |
| |
| From: Andrii Nakryiko <andrii@kernel.org> |
| |
| commit 4bb7ea946a370707315ab774432963ce47291946 upstream. |
| |
| 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> |
| Signed-off-by: Aaron Lu <ziqianlu@bytedance.com> |
| Reported-by: Wei Wei <weiwei.danny@bytedance.com> |
| Closes: https://lore.kernel.org/all/20250605070921.GA3795@bytedance/ |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| kernel/bpf/verifier.c | 21 +++++++++++++++++++-- |
| 1 file changed, 19 insertions(+), 2 deletions(-) |
| |
| --- a/kernel/bpf/verifier.c |
| +++ b/kernel/bpf/verifier.c |
| @@ -2134,12 +2134,29 @@ static int push_jmp_history(struct bpf_v |
| |
| /* Backtrack one insn at a time. If idx is not at the top of recorded |
| * history then previous instruction came from straight line execution. |
| + * Return -ENOENT if we exhausted all instructions within given state. |
| + * |
| + * It's legal to have a bit of a looping with the same starting and ending |
| + * insn index within the same state, e.g.: 3->4->5->3, so just because current |
| + * instruction index is the same as state's first_idx doesn't mean we are |
| + * done. If there is still some jump history left, we should keep going. We |
| + * need to take into account that we might have a jump history between given |
| + * state's parent and itself, due to checkpointing. In this case, we'll have |
| + * history entry recording a jump from last instruction of parent state and |
| + * first instruction of given state. |
| */ |
| static int get_prev_insn_idx(struct bpf_verifier_state *st, int i, |
| u32 *history) |
| { |
| u32 cnt = *history; |
| |
| + if (i == st->first_insn_idx) { |
| + if (cnt == 0) |
| + return -ENOENT; |
| + if (cnt == 1 && st->jmp_history[0].idx == i) |
| + return -ENOENT; |
| + } |
| + |
| if (cnt && st->jmp_history[cnt - 1].idx == i) { |
| i = st->jmp_history[cnt - 1].prev_idx; |
| (*history)--; |
| @@ -2630,9 +2647,9 @@ static int __mark_chain_precision(struct |
| * Nothing to be tracked further in the parent state. |
| */ |
| return 0; |
| - if (i == first_idx) |
| - break; |
| i = get_prev_insn_idx(st, i, &history); |
| + if (i == -ENOENT) |
| + break; |
| if (i >= env->prog->len) { |
| /* This can happen if backtracking reached insn 0 |
| * and there are still reg_mask or stack_mask |
