[PATCH bpf-next 0/2] bpf: fix for use after free bug in inline_bpf_loop

[PATCH bpf-next 0/2] bpf: fix for use after free bug in inline_bpf_loop

[Eduard Zingerman]

These two patches fix the use after free bug in inline_bpf_loop()
reported by Dan Carpenter. The fix for verifier.c and the test case in
test_verifier.c are split into separate commits.

While the first patch is necessary, I'm not sure about the second. The
test case is somewhat fragile because of the following line:

	const int len = getpagesize() - 25;

Here 25 is a magical number that allows env->prog to fit in one page
before bpf_loop inlining and don't fit after the bpf_loop
inlining. I'd prefer to use sizeof(struct bpf_prog) instead of this
constant, but definition of the struct bpf_prog is not available in
test_verifier.c.

Eduard Zingerman (2):
  bpf: fix for use after free bug in inline_bpf_loop
  selftest/bpf: test for use after free bug fix in inline_bpf_loop

 kernel/bpf/verifier.c                         |  2 +-
 tools/testing/selftests/bpf/test_verifier.c   | 39 +++++++++++++++++++
 .../selftests/bpf/verifier/bpf_loop_inline.c  | 11 ++++++
 3 files changed, 51 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH bpf-next 1/2] bpf: fix for use after free bug in inline_bpf_loop

[Eduard Zingerman]

As reported by Dan Carpenter, the following statements in
inline_bpf_loop() might cause to the use after free bug:

	struct bpf_prog *new_prog;
        // ...
	new_prog = bpf_patch_insn_data(env, position, insn_buf, *cnt);
        // ...
	env->prog->insnsi[call_insn_offset].imm = callback_offset;

The bpf_patch_insn_data() might free the memory used by env->prog.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 kernel/bpf/verifier.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index a20d7736a5b2..24601d6b501a 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14417,7 +14417,7 @@ static struct bpf_prog *inline_bpf_loop(struct bpf_verifier_env *env,
 	/* Note: insn_buf[12] is an offset of BPF_CALL_REL instruction */
 	call_insn_offset = position + 12;
 	callback_offset = callback_start - call_insn_offset - 1;
-	env->prog->insnsi[call_insn_offset].imm = callback_offset;
+	new_prog->insnsi[call_insn_offset].imm = callback_offset;
 
 	return new_prog;
 }
-- 
2.25.1

[PATCH bpf-next 2/2] selftest/bpf: test for use after free bug fix in inline_bpf_loop

[Eduard Zingerman]

This test verifies that bpf_loop inlining works as expected when
address of `env->prog` is updated. This address is updated upon BPF
program reallocation.  Reallocation is handled by
core.c:bpf_prog_realloc, which reuses old memory if page boundary is
not crossed. The value of `len` in the test is chosen to cross this
boundary on bpf_loop patching.

Verifies that use after free bug in inline_bpf_loop() reported by Dan
Carpenter is fixed.

Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
 tools/testing/selftests/bpf/test_verifier.c   | 39 +++++++++++++++++++
 .../selftests/bpf/verifier/bpf_loop_inline.c  | 11 ++++++
 2 files changed, 50 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 7fe897c66d81..f9d553fbf68a 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -425,6 +425,45 @@ static void bpf_fill_torturous_jumps(struct bpf_test *self)
 	}
 }
 
+static void bpf_fill_big_prog_with_loop_1(struct bpf_test *self)
+{
+	struct bpf_insn *insn = self->fill_insns;
+	/* This test was added to catch a specific use after free
+	 * error, which happened upon BPF program reallocation.
+	 * Reallocation is handled by core.c:bpf_prog_realloc, which
+	 * reuses old memory if page boundary is not crossed. The
+	 * value of `len` is chosen to cross this boundary on bpf_loop
+	 * patching.
+	 */
+	const int len = getpagesize() - 25;
+	int callback_load_idx;
+	int callback_idx;
+	int i = 0;
+
+	insn[i++] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_1, 1);
+	callback_load_idx = i;
+	insn[i++] = BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW,
+				 BPF_REG_2, BPF_PSEUDO_FUNC, 0,
+				 777 /* filled below */);
+	insn[i++] = BPF_RAW_INSN(0, 0, 0, 0, 0);
+	insn[i++] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_3, 0);
+	insn[i++] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_4, 0);
+	insn[i++] = BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_loop);
+
+	while (i < len - 3)
+		insn[i++] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_0, 0);
+	insn[i++] = BPF_EXIT_INSN();
+
+	callback_idx = i;
+	insn[i++] = BPF_ALU64_IMM(BPF_MOV, BPF_REG_0, 0);
+	insn[i++] = BPF_EXIT_INSN();
+
+	insn[callback_load_idx].imm = callback_idx - callback_load_idx - 1;
+	self->func_info[1].insn_off = callback_idx;
+	self->prog_len = i;
+	assert(i == len);
+}
+
 /* BPF_SK_LOOKUP contains 13 instructions, if you need to fix up maps */
 #define BPF_SK_LOOKUP(func)						\
 	/* struct bpf_sock_tuple tuple = {} */				\
diff --git a/tools/testing/selftests/bpf/verifier/bpf_loop_inline.c b/tools/testing/selftests/bpf/verifier/bpf_loop_inline.c
index 232da07c93b5..2d0023659d88 100644
--- a/tools/testing/selftests/bpf/verifier/bpf_loop_inline.c
+++ b/tools/testing/selftests/bpf/verifier/bpf_loop_inline.c
@@ -244,6 +244,17 @@
 	.func_info_cnt = 3,
 	BTF_TYPES
 },
+{
+	"inline bpf_loop call in a big program",
+	.insns = {},
+	.fill_helper = bpf_fill_big_prog_with_loop_1,
+	.expected_insns = { PSEUDO_CALL_INSN() },
+	.unexpected_insns = { HELPER_CALL_INSN() },
+	.result = ACCEPT,
+	.func_info = { { 0, MAIN_TYPE }, { 16, CALLBACK_TYPE } },
+	.func_info_cnt = 2,
+	BTF_TYPES
+},
 
 #undef HELPER_CALL_INSN
 #undef PSEUDO_CALL_INSN
-- 
2.25.1

Re: [PATCH bpf-next 0/2] bpf: fix for use after free bug in inline_bpf_loop

[patchwork-bot+netdevbpf]

Hello:

This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 24 Jun 2022 05:06:11 +0300 you wrote:
> These two patches fix the use after free bug in inline_bpf_loop()
> reported by Dan Carpenter. The fix for verifier.c and the test case in
> test_verifier.c are split into separate commits.
> 
> While the first patch is necessary, I'm not sure about the second. The
> test case is somewhat fragile because of the following line:
> 
> [...]

Here is the summary with links:
  - [bpf-next,1/2] bpf: fix for use after free bug in inline_bpf_loop
    https://git.kernel.org/bpf/bpf-next/c/fb4e3b33e3e7
  - [bpf-next,2/2] selftest/bpf: test for use after free bug fix in inline_bpf_loop
    https://git.kernel.org/bpf/bpf-next/c/41188e9e9def

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