[PATCH bpf 1/2] bpf: Fix may_goto with negative offset.

[PATCH bpf 1/2] bpf: Fix may_goto with negative offset.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Zac's syzbot crafted a bpf prog that exposed two bugs in may_goto.
The 1st bug is the way may_goto is patched. When offset is negative
it should be patched differently.
The 2nd bug is in the verifier:
when current state may_goto_depth is equal to visited state may_goto_depth
it means there is an actual infinite loop. It's not correct to prune
exploration of the program at this point.
Note, that this check doesn't limit the program to only one may_goto insn,
since 2nd and any further may_goto will increment may_goto_depth only
in the queued state pushed for future exploration. The current state
will have may_goto_depth == 0 regardless of number of may_goto insns
and the verifier has to explore the program until bpf_exit.

Reported-by: Zac Ecob <zacecob@protonmail.com>
Closes: https://lore.kernel.org/bpf/CAADnVQL-15aNp04-cyHRn47Yv61NXfYyhopyZtUyxNojUZUXpA@mail.gmail.com/
Fixes: 011832b97b31 ("bpf: Introduce may_goto instruction")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 kernel/bpf/verifier.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 5586a571bf55..214a9fa8c6fb 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -17460,11 +17460,11 @@ static int is_state_visited(struct bpf_verifier_env *env, int insn_idx)
 				goto skip_inf_loop_check;
 			}
 			if (is_may_goto_insn_at(env, insn_idx)) {
-				if (states_equal(env, &sl->state, cur, RANGE_WITHIN)) {
+				if (sl->state.may_goto_depth != cur->may_goto_depth &&
+				    states_equal(env, &sl->state, cur, RANGE_WITHIN)) {
 					update_loop_entry(cur, &sl->state);
 					goto hit;
 				}
-				goto skip_inf_loop_check;
 			}
 			if (calls_callback(env, insn_idx)) {
 				if (states_equal(env, &sl->state, cur, RANGE_WITHIN))
@@ -20049,7 +20049,10 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 
 			stack_depth_extra = 8;
 			insn_buf[0] = BPF_LDX_MEM(BPF_DW, BPF_REG_AX, BPF_REG_10, stack_off);
-			insn_buf[1] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, insn->off + 2);
+			if (insn->off >= 0)
+				insn_buf[1] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, insn->off + 2);
+			else
+				insn_buf[1] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_AX, 0, insn->off - 1);
 			insn_buf[2] = BPF_ALU64_IMM(BPF_SUB, BPF_REG_AX, 1);
 			insn_buf[3] = BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_AX, stack_off);
 			cnt = 4;
-- 
2.43.0

[PATCH bpf 2/2] selftests/bpf: Add tests for may_goto with negative offset.

[Alexei Starovoitov]

From: Alexei Starovoitov <ast@kernel.org>

Add few tests with may_goto and negative offset.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 .../bpf/progs/verifier_iterating_callbacks.c  | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
index 8885e5239d6b..80c737b6d340 100644
--- a/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
+++ b/tools/testing/selftests/bpf/progs/verifier_iterating_callbacks.c
@@ -274,6 +274,58 @@ static __naked void iter_limit_bug_cb(void)
 	);
 }
 
+int tmp_var;
+SEC("socket")
+__failure __msg("infinite loop detected at insn 2")
+__naked void jgt_imm64_and_may_goto(void)
+{
+	asm volatile ("			\
+	r0 = %[tmp_var] ll;		\
+l0_%=:	.byte 0xe5; /* may_goto */	\
+	.byte 0; /* regs */		\
+	.short -3; /* off -3 */		\
+	.long 0; /* imm */		\
+	if r0 > 10 goto l0_%=;		\
+	r0 = 0;				\
+	exit;				\
+"	:: __imm_addr(tmp_var)
+	: __clobber_all);
+}
+
+SEC("socket")
+__failure __msg("infinite loop detected at insn 1")
+__naked void may_goto_self(void)
+{
+	asm volatile ("			\
+	r0 = *(u32 *)(r10 - 4);		\
+l0_%=:	.byte 0xe5; /* may_goto */	\
+	.byte 0; /* regs */		\
+	.short -1; /* off -1 */		\
+	.long 0; /* imm */		\
+	if r0 > 10 goto l0_%=;		\
+	r0 = 0;				\
+	exit;				\
+"	::: __clobber_all);
+}
+
+SEC("socket")
+__success __retval(0)
+__naked void may_goto_neg_off(void)
+{
+	asm volatile ("			\
+	r0 = *(u32 *)(r10 - 4);		\
+	goto l0_%=;			\
+	goto l1_%=;			\
+l0_%=:	.byte 0xe5; /* may_goto */	\
+	.byte 0; /* regs */		\
+	.short -2; /* off -2 */		\
+	.long 0; /* imm */		\
+	if r0 > 10 goto l0_%=;		\
+l1_%=:	r0 = 0;				\
+	exit;				\
+"	::: __clobber_all);
+}
+
 SEC("tc")
 __failure
 __flag(BPF_F_TEST_STATE_FREQ)
-- 
2.43.0

Re: [PATCH bpf 1/2] bpf: Fix may_goto with negative offset.

[Eduard Zingerman]

On Wed, 2024-06-19 at 16:53 -0700, Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Zac's syzbot crafted a bpf prog that exposed two bugs in may_goto.
> The 1st bug is the way may_goto is patched. When offset is negative
> it should be patched differently.
> The 2nd bug is in the verifier:
> when current state may_goto_depth is equal to visited state may_goto_depth
> it means there is an actual infinite loop. It's not correct to prune
> exploration of the program at this point.
> Note, that this check doesn't limit the program to only one may_goto insn,
> since 2nd and any further may_goto will increment may_goto_depth only
> in the queued state pushed for future exploration. The current state
> will have may_goto_depth == 0 regardless of number of may_goto insns
> and the verifier has to explore the program until bpf_exit.
> 
> Reported-by: Zac Ecob <zacecob@protonmail.com>
> Closes: https://lore.kernel.org/bpf/CAADnVQL-15aNp04-cyHRn47Yv61NXfYyhopyZtUyxNojUZUXpA@mail.gmail.com/
> Fixes: 011832b97b31 ("bpf: Introduce may_goto instruction")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---

Took me a while to figure out why we don't need this for iterators.

Acked-by: Eduard Zingerman <eddyz87@gmail.com>

Re: [PATCH bpf 1/2] bpf: Fix may_goto with negative offset.

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf.git (master)
by Andrii Nakryiko <andrii@kernel.org>:

On Wed, 19 Jun 2024 16:53:54 -0700 you wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Zac's syzbot crafted a bpf prog that exposed two bugs in may_goto.
> The 1st bug is the way may_goto is patched. When offset is negative
> it should be patched differently.
> The 2nd bug is in the verifier:
> when current state may_goto_depth is equal to visited state may_goto_depth
> it means there is an actual infinite loop. It's not correct to prune
> exploration of the program at this point.
> Note, that this check doesn't limit the program to only one may_goto insn,
> since 2nd and any further may_goto will increment may_goto_depth only
> in the queued state pushed for future exploration. The current state
> will have may_goto_depth == 0 regardless of number of may_goto insns
> and the verifier has to explore the program until bpf_exit.
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Fix may_goto with negative offset.
    https://git.kernel.org/bpf/bpf/c/635c7a3cb3bf
  - [bpf,2/2] selftests/bpf: Add tests for may_goto with negative offset.
    https://git.kernel.org/bpf/bpf/c/d3a2cbf6e4e5

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html