[PATCH bpf-next 0/4] fixes for bpf_jit_harden race

Hi,

Now bpf_jit_harden will be tested twice for each subprog if there are
subprogs in bpf program and constant blinding may increase the length of
program, so when running "./test_progs -t subprogs" and toggling
bpf_jit_harden between 0 and 2, extra pass in bpf_int_jit_compile() may
fail because constant blinding increases the length of subprog and
the length is mismatched with the first pass.

The failure uncovers several issues in error handling of jit_subprogs()
and bpf_int_jit_compile():
(1) jit_subprogs() continues even when extra pass for one subprogs fails
It may leads to oops during to UAF. Fixed in patch #1.

(2) jit_subprogs() doesn't do proper cleanup for other subprogs which
    have not went through the extra pass.
It will lead to oops and memory leak. Fixed in patch #2. Other arch JIT
may have the same problem, and will fix later if the proposed fix for
x86-64 is accepted.

(3) bpf_int_jit_compile() may fail due to inconsistent twice read values
    from bpf_jit_harden
Fixed in patch #3 by caching the value of bpf_jit_blinding_enabled().

Patch #4 just adds a test to ensure these problem are fixed.

Comments and suggestions are welcome.

Regards,
Tao

Hou Tao (4):
  bpf, x86: Fall back to interpreter mode when extra pass fails
  bpf: Introduce bpf_int_jit_abort()
  bpf: Fix net.core.bpf_jit_harden race
  selftests/bpf: Test subprog jit when toggle bpf_jit_harden repeatedly

 arch/x86/net/bpf_jit_comp.c                   | 35 ++++++++-
 include/linux/filter.h                        |  2 +
 kernel/bpf/core.c                             | 12 ++-
 kernel/bpf/verifier.c                         |  8 +-
 .../selftests/bpf/prog_tests/subprogs.c       | 77 ++++++++++++++++---
 5 files changed, 120 insertions(+), 14 deletions(-)

-- 
2.29.2

Extra pass for subprog jit may fail (e.g. due to bpf_jit_harden race),
but bpf_func is not cleared for the subprog and jit_subprogs will
succeed. The running of the bpf program may lead to oops because the
memory for the jited subprog image has already been freed.

So fall back to interpreter mode by clearing bpf_func/jited/jited_len
when extra pass fails.

Signed-off-by: Hou Tao <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index e6ff8f4f9ea4..ec3f00be2ac5 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c

@@ -2335,7 +2335,13 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 						   sizeof(rw_header->size));
 				bpf_jit_binary_pack_free(header, rw_header);
 			}
+			/* Fall back to interpreter mode */
 			prog = orig_prog;
+			if (extra_pass) {
+				prog->bpf_func = NULL;
+				prog->jited = 0;
+				prog->jited_len = 0;
+			}
 			goto out_addrs;
 		}
 		if (image) {

@@ -2384,8 +2390,9 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog)
 			 * Both cases are serious bugs and justify WARN_ON.
 			 */
 			if (WARN_ON(bpf_jit_binary_pack_finalize(prog, header, rw_header))) {
-				prog = orig_prog;
-				goto out_addrs;
+				/* header has been freed */
+				header = NULL;
+				goto out_image;
 			}
 
 			bpf_tail_call_direct_fixup(prog);

-- 
2.29.2

It will be used to do cleanup for subprog which has been jited in first
pass but extra pass has not been done. The scenario is possible when
extra pass for subprog in the middle fails. The failure may lead to
oops due to inconsistent status for pack allocator (e.g. ro_hdr->size
and use_bpf_prog_pack) and memory leak in aux->jit_data.

For x86-64, bpf_int_jit_abort() will free allocated memories saved in
aux->jit_data and fall back to interpreter mode to bypass the calling
of bpf_jit_binary_pack_free() in bpf_jit_free().

Signed-off-by: Hou Tao <redacted>
---
 arch/x86/net/bpf_jit_comp.c | 24 ++++++++++++++++++++++++
 include/linux/filter.h      |  1 +
 kernel/bpf/core.c           |  9 +++++++++
 kernel/bpf/verifier.c       |  3 +++
 4 files changed, 37 insertions(+)

diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index ec3f00be2ac5..49bc0ddd55ae 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c

@@ -2244,6 +2244,30 @@ struct x64_jit_data {
 	struct jit_context ctx;
 };
 
+void bpf_int_jit_abort(struct bpf_prog *prog)
+{
+	struct x64_jit_data *jit_data = prog->aux->jit_data;
+	struct bpf_binary_header *header, *rw_header;
+
+	if (!jit_data)
+		return;
+
+	prog->bpf_func = NULL;
+	prog->jited = 0;
+	prog->jited_len = 0;
+
+	header = jit_data->header;
+	rw_header = jit_data->rw_header;
+	bpf_arch_text_copy(&header->size, &rw_header->size,
+			   sizeof(rw_header->size));
+	bpf_jit_binary_pack_free(header, rw_header);
+
+	kvfree(jit_data->addrs);
+	kfree(jit_data);
+
+	prog->aux->jit_data = NULL;
+}
+
 #define MAX_PASSES 20
 #define PADDING_PASSES (MAX_PASSES - 5)
 

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 9bf26307247f..f3a913229edd 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h

@@ -945,6 +945,7 @@ u64 __bpf_call_base(u64 r1, u64 r2, u64 r3, u64 r4, u64 r5);
 	 (void *)__bpf_call_base)
 
 struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *prog);
+void bpf_int_jit_abort(struct bpf_prog *prog);
 void bpf_jit_compile(struct bpf_prog *prog);
 bool bpf_jit_needs_zext(void);
 bool bpf_jit_supports_kfunc_call(void);

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index ab630f773ec1..a1841e11524c 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -2636,6 +2636,15 @@ struct bpf_prog * __weak bpf_int_jit_compile(struct bpf_prog *prog)
 	return prog;
 }
 
+/*
+ * If arch JIT uses aux->jit_data to save temporary allocated status and
+ * supports subprog, it needs to override the function to free allocated
+ * memories and fall back to interpreter mode for passed prog.
+ */
+void __weak bpf_int_jit_abort(struct bpf_prog *prog)
+{
+}
+
 /* Stub for JITs that support eBPF. All cBPF code gets transformed into
  * eBPF by the kernel and is later compiled by bpf_int_jit_compile().
  */

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index e34264200e09..885e515cf83f 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -13086,6 +13086,9 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		if (tmp != func[i] || func[i]->bpf_func != old_bpf_func) {
 			verbose(env, "JIT doesn't support bpf-to-bpf calls\n");
 			err = -ENOTSUPP;
+			/* Abort extra pass for the remaining subprogs */
+			while (++i < env->subprog_cnt)
+				bpf_int_jit_abort(func[i]);
 			goto out_free;
 		}
 		cond_resched();

-- 
2.29.2

It is the bpf_jit_harden counterpart to commit 60b58afc96c9 ("bpf: fix
net.core.bpf_jit_enable race"). bpf_jit_harden will be tested twice
for each subprog if there are subprogs in bpf program and constant
blinding may increase the length of program, so when running
"./test_progs -t subprogs" and toggling bpf_jit_harden between 0 and 2,
jit_subprogs may fail because constant blinding increases the length
of subprog instructions during extra passs.

So cache the value of bpf_jit_blinding_enabled() during program
allocation, and use the cached value during constant blinding, subprog
JITing and args tracking of tail call.

Signed-off-by: Hou Tao <redacted>
---
 include/linux/filter.h | 1 +
 kernel/bpf/core.c      | 3 ++-
 kernel/bpf/verifier.c  | 5 +++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index f3a913229edd..605c8456467b 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h

@@ -566,6 +566,7 @@ struct bpf_prog {
 				gpl_compatible:1, /* Is filter GPL compatible? */
 				cb_access:1,	/* Is control block accessed? */
 				dst_needed:1,	/* Do we need dst entry? */
+				blinding_requested:1, /* needs constant blinding */
 				blinded:1,	/* Was blinded */
 				is_func:1,	/* program is a bpf function */
 				kprobe_override:1, /* Do we override a kprobe? */

diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index a1841e11524c..a4b5fb095261 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c

@@ -105,6 +105,7 @@ struct bpf_prog *bpf_prog_alloc_no_stats(unsigned int size, gfp_t gfp_extra_flag
 	fp->aux = aux;
 	fp->aux->prog = fp;
 	fp->jit_requested = ebpf_jit_enabled();
+	fp->blinding_requested = bpf_jit_blinding_enabled(fp);
 
 	INIT_LIST_HEAD_RCU(&fp->aux->ksym.lnode);
 	mutex_init(&fp->aux->used_maps_mutex);

@@ -1382,7 +1383,7 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_prog *prog)
 	struct bpf_insn *insn;
 	int i, rewritten;
 
-	if (!bpf_jit_blinding_enabled(prog) || prog->blinded)
+	if (!prog->blinding_requested || prog->blinded)
 		return prog;
 
 	clone = bpf_prog_clone_create(prog, GFP_USER);

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 885e515cf83f..ec4780bd44f1 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c

@@ -13024,6 +13024,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 		func[i]->aux->name[0] = 'F';
 		func[i]->aux->stack_depth = env->subprog_info[i].stack_depth;
 		func[i]->jit_requested = 1;
+		func[i]->blinding_requested = prog->blinding_requested;
 		func[i]->aux->kfunc_tab = prog->aux->kfunc_tab;
 		func[i]->aux->kfunc_btf_tab = prog->aux->kfunc_btf_tab;
 		func[i]->aux->linfo = prog->aux->linfo;

@@ -13150,6 +13151,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
 out_undo_insn:
 	/* cleanup main prog to be interpreted */
 	prog->jit_requested = 0;
+	prog->blinding_requested = 0;
 	for (i = 0, insn = prog->insnsi; i < prog->len; i++, insn++) {
 		if (!bpf_pseudo_call(insn))
 			continue;

@@ -13243,7 +13245,6 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 {
 	struct bpf_prog *prog = env->prog;
 	enum bpf_attach_type eatype = prog->expected_attach_type;
-	bool expect_blinding = bpf_jit_blinding_enabled(prog);
 	enum bpf_prog_type prog_type = resolve_prog_type(prog);
 	struct bpf_insn *insn = prog->insnsi;
 	const struct bpf_func_proto *fn;

@@ -13407,7 +13408,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
 			insn->code = BPF_JMP | BPF_TAIL_CALL;
 
 			aux = &env->insn_aux_data[i + delta];
-			if (env->bpf_capable && !expect_blinding &&
+			if (env->bpf_capable && !prog->blinding_requested &&
 			    prog->jit_requested &&
 			    !bpf_map_key_poisoned(aux) &&
 			    !bpf_map_ptr_poisoned(aux) &&

-- 
2.29.2

When bpf_jit_harden is toggled between 0 and 2, subprog jit may fail
due to inconsistent twice read values of bpf_jit_harden during jit. So
add a test to ensure the problem is fixed.

Signed-off-by: Hou Tao <redacted>
---
 .../selftests/bpf/prog_tests/subprogs.c       | 77 ++++++++++++++++---
 1 file changed, 68 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/subprogs.c b/tools/testing/selftests/bpf/prog_tests/subprogs.c
index 3f3d2ac4dd57..903f35a9e62e 100644
--- a/tools/testing/selftests/bpf/prog_tests/subprogs.c
+++ b/tools/testing/selftests/bpf/prog_tests/subprogs.c

@@ -1,32 +1,83 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (c) 2020 Facebook */
 #include <test_progs.h>
-#include <time.h>
 #include "test_subprogs.skel.h"
 #include "test_subprogs_unused.skel.h"
 
-static int duration;
+struct toggler_ctx {
+	int fd;
+	bool stop;
+};
 
-void test_subprogs(void)
+static void *toggle_jit_harden(void *arg)
+{
+	struct toggler_ctx *ctx = arg;
+	char two = '2';
+	char zero = '0';
+
+	while (!ctx->stop) {
+		lseek(ctx->fd, SEEK_SET, 0);
+		write(ctx->fd, &two, sizeof(two));
+		lseek(ctx->fd, SEEK_SET, 0);
+		write(ctx->fd, &zero, sizeof(zero));
+	}
+
+	return NULL;
+}
+
+static void test_subprogs_with_jit_harden_toggling(void)
+{
+	struct toggler_ctx ctx;
+	pthread_t toggler;
+	int err;
+	unsigned int i, loop = 10;
+
+	ctx.fd = open("/proc/sys/net/core/bpf_jit_harden", O_RDWR);
+	if (!ASSERT_GE(ctx.fd, 0, "open bpf_jit_harden"))
+		return;
+
+	ctx.stop = false;
+	err = pthread_create(&toggler, NULL, toggle_jit_harden, &ctx);
+	if (!ASSERT_OK(err, "new toggler"))
+		goto out;
+
+	/* Make toggler thread to run */
+	usleep(1);
+
+	for (i = 0; i < loop; i++) {
+		struct test_subprogs *skel = test_subprogs__open_and_load();
+
+		if (!ASSERT_OK_PTR(skel, "skel open"))
+			break;
+		test_subprogs__destroy(skel);
+	}
+
+	ctx.stop = true;
+	pthread_join(toggler, NULL);
+out:
+	close(ctx.fd);
+}
+
+static void test_subprogs_alone(void)
 {
 	struct test_subprogs *skel;
 	struct test_subprogs_unused *skel2;
 	int err;
 
 	skel = test_subprogs__open_and_load();
-	if (CHECK(!skel, "skel_open", "failed to open skeleton\n"))
+	if (!ASSERT_OK_PTR(skel, "skel_open"))
 		return;
 
 	err = test_subprogs__attach(skel);
-	if (CHECK(err, "skel_attach", "failed to attach skeleton: %d\n", err))
+	if (!ASSERT_OK(err, "skel attach"))
 		goto cleanup;
 
 	usleep(1);
 
-	CHECK(skel->bss->res1 != 12, "res1", "got %d, exp %d\n", skel->bss->res1, 12);
-	CHECK(skel->bss->res2 != 17, "res2", "got %d, exp %d\n", skel->bss->res2, 17);
-	CHECK(skel->bss->res3 != 19, "res3", "got %d, exp %d\n", skel->bss->res3, 19);
-	CHECK(skel->bss->res4 != 36, "res4", "got %d, exp %d\n", skel->bss->res4, 36);
+	ASSERT_EQ(skel->bss->res1, 12, "res1");
+	ASSERT_EQ(skel->bss->res2, 17, "res2");
+	ASSERT_EQ(skel->bss->res3, 19, "res3");
+	ASSERT_EQ(skel->bss->res4, 36, "res4");
 
 	skel2 = test_subprogs_unused__open_and_load();
 	ASSERT_OK_PTR(skel2, "unused_progs_skel");

@@ -35,3 +86,11 @@ void test_subprogs(void)
 cleanup:
 	test_subprogs__destroy(skel);
 }
+
+void test_subprogs(void)
+{
+	if (test__start_subtest("subprogs_alone"))
+		test_subprogs_alone();
+	if (test__start_subtest("subprogs_and_jit_harden"))
+		test_subprogs_with_jit_harden_toggling();
+}

-- 
2.29.2

On Wed, Mar 09, 2022 at 08:33:20PM +0800, Hou Tao wrote:

It is the bpf_jit_harden counterpart to commit 60b58afc96c9 ("bpf: fix
net.core.bpf_jit_enable race"). bpf_jit_harden will be tested twice
for each subprog if there are subprogs in bpf program and constant
blinding may increase the length of program, so when running
"./test_progs -t subprogs" and toggling bpf_jit_harden between 0 and 2,
jit_subprogs may fail because constant blinding increases the length
of subprog instructions during extra passs.

So cache the value of bpf_jit_blinding_enabled() during program
allocation, and use the cached value during constant blinding, subprog
JITing and args tracking of tail call.

Looks like this patch alone is enough.
With race fixed. Patches 1 and 2 are no longer necessary, right?

Hi,

On 3/10/2022 7:22 AM, Alexei Starovoitov wrote:

On Wed, Mar 09, 2022 at 08:33:20PM +0800, Hou Tao wrote:

quoted

It is the bpf_jit_harden counterpart to commit 60b58afc96c9 ("bpf: fix
net.core.bpf_jit_enable race"). bpf_jit_harden will be tested twice
for each subprog if there are subprogs in bpf program and constant
blinding may increase the length of program, so when running
"./test_progs -t subprogs" and toggling bpf_jit_harden between 0 and 2,
jit_subprogs may fail because constant blinding increases the length
of subprog instructions during extra passs.

So cache the value of bpf_jit_blinding_enabled() during program
allocation, and use the cached value during constant blinding, subprog
JITing and args tracking of tail call.

Looks like this patch alone is enough.
With race fixed. Patches 1 and 2 are no longer necessary, right?

Yes and no. With patch 3 applied, the problems described in patch 1 and patch 2
are gone, but it may recur due to other issue in JIT. So I post these two patch
together and hope these fixes can also be merged.

.

On Wed, Mar 9, 2022 at 5:01 PM Hou Tao [off-list ref] wrote:

Hi,

On 3/10/2022 7:22 AM, Alexei Starovoitov wrote:

quoted

On Wed, Mar 09, 2022 at 08:33:20PM +0800, Hou Tao wrote:

quoted

It is the bpf_jit_harden counterpart to commit 60b58afc96c9 ("bpf: fix
net.core.bpf_jit_enable race"). bpf_jit_harden will be tested twice
for each subprog if there are subprogs in bpf program and constant
blinding may increase the length of program, so when running
"./test_progs -t subprogs" and toggling bpf_jit_harden between 0 and 2,
jit_subprogs may fail because constant blinding increases the length
of subprog instructions during extra passs.

So cache the value of bpf_jit_blinding_enabled() during program
allocation, and use the cached value during constant blinding, subprog
JITing and args tracking of tail call.

Looks like this patch alone is enough.
With race fixed. Patches 1 and 2 are no longer necessary, right?

Yes and no. With patch 3 applied, the problems described in patch 1 and patch 2
are gone, but it may recur due to other issue in JIT. So I post these two patch
together and hope these fixes can also be merged.

What kind of 'issues in JIT'?
I'd rather fix them than do defensive programming.
patch 2 is a hack that should not happen in a correct JIT.

Hi,

On 3/10/2022 11:29 AM, Alexei Starovoitov wrote:

On Wed, Mar 9, 2022 at 5:01 PM Hou Tao [off-list ref] wrote:

quoted

Hi,

On 3/10/2022 7:22 AM, Alexei Starovoitov wrote:

quoted

On Wed, Mar 09, 2022 at 08:33:20PM +0800, Hou Tao wrote:

quoted

It is the bpf_jit_harden counterpart to commit 60b58afc96c9 ("bpf: fix
net.core.bpf_jit_enable race"). bpf_jit_harden will be tested twice
for each subprog if there are subprogs in bpf program and constant
blinding may increase the length of program, so when running
"./test_progs -t subprogs" and toggling bpf_jit_harden between 0 and 2,
jit_subprogs may fail because constant blinding increases the length
of subprog instructions during extra passs.

So cache the value of bpf_jit_blinding_enabled() during program
allocation, and use the cached value during constant blinding, subprog
JITing and args tracking of tail call.

Looks like this patch alone is enough.
With race fixed. Patches 1 and 2 are no longer necessary, right?

Yes and no. With patch 3 applied, the problems described in patch 1 and patch 2
are gone, but it may recur due to other issue in JIT. So I post these two patch
together and hope these fixes can also be merged.

What kind of 'issues in JIT'?
I'd rather fix them than do defensive programming.

Understand. For "issues in JIT" I just mean all kinds of error path handling in
jit, not a real problem.

patch 2 is a hack that should not happen in a correct JIT.
.

And "the hack" is partially due to the introduction of an extra pass in JIT. So
I am fine to drop it.

Regards,
Tao

On 3/9/22 1:33 PM, Hou Tao wrote:

Don't quite follow this one. For example, if we'd fail in the second pass, the
goto out_addrs from jit would free and clear the prog->aux->jit_data. If we'd succeed
but different prog is returned, prog->aux->jit_data is released and later the goto
out_free in here would clear the jited prog via bpf_jit_free(). Which code path leaves
prog->aux->jit_data as non-NULL such that extra bpf_int_jit_abort() is needed?

  			goto out_free;
  		}
  		cond_resched();

On 3/12/22 12:54 AM, Daniel Borkmann wrote:
[...]

Don't quite follow this one. For example, if we'd fail in the second pass, the
goto out_addrs from jit would free and clear the prog->aux->jit_data. If we'd succeed
but different prog is returned, prog->aux->jit_data is released and later the goto
out_free in here would clear the jited prog via bpf_jit_free(). Which code path leaves
prog->aux->jit_data as non-NULL such that extra bpf_int_jit_abort() is needed?

Nevermind, it's for those that haven't been jited second time yet..

Hello:

This series was applied to bpf/bpf-next.git (master)
by Alexei Starovoitov [off-list ref]:

On Wed, 9 Mar 2022 20:33:17 +0800 you wrote:

Hi,

Now bpf_jit_harden will be tested twice for each subprog if there are
subprogs in bpf program and constant blinding may increase the length of
program, so when running "./test_progs -t subprogs" and toggling
bpf_jit_harden between 0 and 2, extra pass in bpf_int_jit_compile() may
fail because constant blinding increases the length of subprog and
the length is mismatched with the first pass.

[...]

Here is the summary with links:
  - [bpf-next,1/4] bpf, x86: Fall back to interpreter mode when extra pass fails
    https://git.kernel.org/bpf/bpf-next/c/73e14451f39e
  - [bpf-next,2/4] bpf: Introduce bpf_int_jit_abort()
    (no matching commit)
  - [bpf-next,3/4] bpf: Fix net.core.bpf_jit_harden race
    https://git.kernel.org/bpf/bpf-next/c/d2a3b7c5becc
  - [bpf-next,4/4] selftests/bpf: Test subprog jit when toggle bpf_jit_harden repeatedly
    https://git.kernel.org/bpf/bpf-next/c/ad13baf45691

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