[PATCH bpf-next 0/5] bpf: Fix three bugs around constant blinding and nop removal

[PATCH bpf-next 0/5] bpf: Fix three bugs around constant blinding and nop removal

[Qiliang Yuan]

Alexei listed three bugs that were in the tree before the patch list
series [1]. Each patch fixes one, the last two add tests:

  bug                                                  fix   test
  line info stays blinded when the JIT falls back      1     -
  removing a nospec jump as a nop drops its barrier    2     4
  log_true_size misses an error from blinding          3     5

Patch 1 needs a JIT failure behind blinding with
CONFIG_BPF_JIT_ALWAYS_ON=n, which a selftest can't cause, so its log
shows a run with fail-nth instead. Each bug shows up on bpf-next and is
gone with its fix. test_verifier and test_progs show no new failures.

[1] https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/

Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
Qiliang Yuan (5):
      bpf: Put the line info back when the JIT falls back after blinding
      bpf: Keep the barrier of a nospec insn when removing nops
      bpf: Finalize the verifier log after the runtime is selected
      selftests/bpf: Cover a nospec jump that becomes a nop
      selftests/bpf: Test log_true_size with an error from blinding

 include/linux/bpf.h                                |  6 +++
 kernel/bpf/core.c                                  | 29 ++++++++++-
 kernel/bpf/fixups.c                                |  3 +-
 kernel/bpf/verifier.c                              | 13 +++--
 .../selftests/bpf/prog_tests/blinding_log.c        | 59 ++++++++++++++++++++++
 .../testing/selftests/bpf/progs/verifier_unpriv.c  | 35 +++++++++++++
 6 files changed, 138 insertions(+), 7 deletions(-)
---
base-commit: cd5bc55288d4fd999595c8f2af6a5408cbe92a52
change-id: 20261102-bpf-blind-nops-fixes-e45e91d65c75

Best regards,
-- 
Qiliang Yuan <odys.yuan@gmail.com>

[PATCH bpf-next 1/5] bpf: Put the line info back when the JIT falls back after blinding

[Qiliang Yuan]

bpf_jit_blind_constants() blinds the constants of a clone of the prog,
which shares the aux data with the prog. The line info moves with the
blinded insns.

When the JIT then fails, the kernel goes back to the prog that isn't
blinded and runs it in the interpreter (CONFIG_BPF_JIT_ALWAYS_ON=n). Its
line info keeps the offsets of the blinded insns. A prog of 4 insns with
a line info record at each insn, loaded with bpf_jit_harden=2 and an
allocation of the JIT failed through fail-nth:

  xlated=4 insns linfo: 0 3 6 9

Keep the offsets from before blinding in aux->linfo_unblinded, and put
them back in bpf_jit_prog_release_other() when it keeps the prog that
isn't blinded. Free them either way.

Fixes: c454a46b5efd ("bpf: Add bpf_line_info support")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 include/linux/bpf.h |  6 ++++++
 kernel/bpf/core.c   | 29 ++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index a80e81ca19ec1..83b3688f176e7 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1963,6 +1963,12 @@ struct bpf_prog_aux {
 	 * using the linfo_idx.
 	 */
 	void **jited_linfo;
+	/*
+	 * linfo->insn_off of the prog before constant blinding, while the
+	 * blinded clone is JITed. The kernel puts it back when it falls back
+	 * to the prog that isn't blinded.
+	 */
+	u32 *linfo_unblinded;
 	u32 func_info_cnt;
 	u32 nr_linfo;
 	/* subprog can use linfo_idx to access its first linfo and
diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
index 0586ed0203975..d45a82f1a42a5 100644
--- a/kernel/bpf/core.c
+++ b/kernel/bpf/core.c
@@ -1762,6 +1762,17 @@ static void bpf_prog_clone_free(struct bpf_prog *fp)
 
 void bpf_jit_prog_release_other(struct bpf_prog *fp, struct bpf_prog *fp_other)
 {
+	struct bpf_prog_aux *aux = fp->aux;
+	u32 i;
+
+	/* The line info moved with the blinded insns, move it back. */
+	if (aux->linfo_unblinded && !fp->blinded) {
+		for (i = 0; i < aux->nr_linfo; i++)
+			aux->linfo[i].insn_off = aux->linfo_unblinded[i];
+	}
+	kvfree(aux->linfo_unblinded);
+	aux->linfo_unblinded = NULL;
+
 	/* We have to repoint aux->prog to self, as we don't
 	 * know whether fp here is the clone or the original.
 	 */
@@ -1808,6 +1819,7 @@ static struct bpf_prog *bpf_patch_list_apply(struct bpf_prog *prog, struct bpf_p
 struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bpf_prog *prog)
 {
 	struct bpf_insn insn_buff[16], aux[2], *last;
+	u32 *linfo_unblinded = NULL, nr_linfo;
 	struct bpf_patch_list pl = {};
 	struct bpf_prog *clone, *tmp;
 	struct bpf_insn *insn;
@@ -1816,9 +1828,22 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bp
 	if (WARN_ON_ONCE(env && env->prog != prog))
 		return ERR_PTR(-EINVAL);
 
+	/* keep where the line info is, for a fallback to 'prog' */
+	nr_linfo = prog->aux->nr_linfo;
+	if (nr_linfo) {
+		linfo_unblinded = kvmalloc_array(nr_linfo, sizeof(*linfo_unblinded),
+						 GFP_USER);
+		if (!linfo_unblinded)
+			return ERR_PTR(-ENOMEM);
+		for (i = 0; i < nr_linfo; i++)
+			linfo_unblinded[i] = prog->aux->linfo[i].insn_off;
+	}
+
 	clone = bpf_prog_clone_create(prog, GFP_USER);
-	if (!clone)
+	if (!clone) {
+		kvfree(linfo_unblinded);
 		return ERR_PTR(-ENOMEM);
+	}
 
 	/* make sure the patches go to the clone */
 	if (env)
@@ -1883,11 +1908,13 @@ struct bpf_prog *bpf_jit_blind_constants(struct bpf_verifier_env *env, struct bp
 		 * 'clone' is still valid.
 		 */
 		bpf_jit_prog_release_other(prog, clone);
+		kvfree(linfo_unblinded);
 		if (env && fatal_signal_pending(current))
 			return ERR_PTR(-EINTR);
 		return ERR_PTR(err);
 	}
 
+	tmp->aux->linfo_unblinded = linfo_unblinded;
 	tmp->blinded = 1;
 	return tmp;
 }

-- 
2.43.0

[PATCH bpf-next 2/5] bpf: Keep the barrier of a nospec insn when removing nops

[Qiliang Yuan]

When an insn is unsafe on a mispredicted path, the verifier marks it
nospec, and bpf_convert_ctx_accesses() puts a barrier in front of it.
The verifier doesn't check the mispredicted path beyond the insn.

bpf_opt_remove_nops() runs before that and doesn't look at the mark. A
conditional jump can be nospec, then become a 'goto' over dead code,
and then a nop. The pass removes it, and the barrier goes with it. Here
the jump behind 'r1 = 0' compared r1 = r10 on the mispredicted path, for
a prog loaded with CAP_BPF and without CAP_PERFMON:

  0: call unknown
  1: r0 &= 255
  2: r1 = 0
  3: r0 = 0
  4: exit

Don't take a nospec insn for a nop.

Fixes: d6f1c85f2253 ("bpf: Fall back to nospec for Spectre v1")
Cc: stable@vger.kernel.org # 6.17+
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
For stable: before the patch list, opt_remove_nops() in verifier.c
removes 'ja 0' and 'may_goto +0'. Skip an insn whose aux data has nospec
there.
---
 kernel/bpf/fixups.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/bpf/fixups.c b/kernel/bpf/fixups.c
index ead78713e04f4..0cc758749be9b 100644
--- a/kernel/bpf/fixups.c
+++ b/kernel/bpf/fixups.c
@@ -699,7 +699,8 @@ static bool insn_is_nop(struct bpf_verifier_env *env, int i, int next)
 {
 	struct bpf_insn insn = env->prog->insnsi[i];
 
-	if (env->insn_aux_data[i].indirect_target)
+	/* keep a nospec insn, the barrier in front of it must stay */
+	if (env->insn_aux_data[i].indirect_target || env->insn_aux_data[i].nospec)
 		return false;
 	if (insn.off < 0 || i + 1 + insn.off > next)
 		return false;

-- 
2.43.0

[PATCH bpf-next 3/5] bpf: Finalize the verifier log after the runtime is selected

[Qiliang Yuan]

bpf_check() finalizes the log, which reports its size in log_true_size,
and selects the runtime of the prog after that. For a prog of one
function, selecting the runtime blinds the constants.

Blinding can log "insn %d cannot be patched due to 16-bit range". The
message lands in the buffer, but log_true_size doesn't count it: with a
stats-only log, log_true_size is 219 while the log takes 264 bytes. A
caller that sizes the buffer from log_true_size loses the message.

Finalize the log after __bpf_prog_select_runtime().

Fixes: d9ef13f72711 ("bpf: Pass bpf_verifier_env to JIT")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 kernel/bpf/verifier.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 935b163f81e4b..974dc571eff05 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -24112,11 +24112,6 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
 	print_verification_stats(env);
 	env->prog->aux->verified_insns = env->insn_processed;
 
-	/* preserve original error even if log finalization is successful */
-	err = bpf_log_attr_finalize(attr_log, &env->log);
-	if (err)
-		ret = err;
-
 	if (ret)
 		goto err_release_maps;
 
@@ -24167,6 +24162,14 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr, bpfptr_t uattr,
 	env->prog = __bpf_prog_select_runtime(env, env->prog, &ret);
 
 err_release_maps:
+	/*
+	 * Finalize the log after the runtime is selected, which blinds the
+	 * constants of a prog of one function and can log an error. Preserve
+	 * the original error even if log finalization is successful.
+	 */
+	err = bpf_log_attr_finalize(attr_log, &env->log);
+	if (err)
+		ret = err;
 	if (ret)
 		release_insn_arrays(env);
 	if (!env->prog->aux->used_maps)

-- 
2.43.0

[PATCH bpf-next 4/5] selftests/bpf: Cover a nospec jump that becomes a nop

[Qiliang Yuan]

A conditional jump that is unsafe on a mispredicted path gets a barrier,
even when it later becomes a 'goto' over dead code only.

No test has such a jump, so nothing checks that bpf_opt_remove_nops()
keeps it with its barrier.

Compare a pointer on the mispredicted path at a jump that always goes
over dead code, and check that the barrier and the jump stay for a prog
loaded without CAP_PERFMON.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 .../testing/selftests/bpf/progs/verifier_unpriv.c  | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/tools/testing/selftests/bpf/progs/verifier_unpriv.c b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
index a54b5a58b945b..b9d96366e656d 100644
--- a/tools/testing/selftests/bpf/progs/verifier_unpriv.c
+++ b/tools/testing/selftests/bpf/progs/verifier_unpriv.c
@@ -1072,4 +1072,39 @@ __naked void nospec_backward_call(void)
 "	::: __clobber_all);
 }
 
+/*
+ * Compare a pointer at l0 on the mispredicted path, so that the jump gets a
+ * barrier. On the real path r1 is 0, and the jump only goes over dead code and
+ * becomes a nop. Keep it with its barrier.
+ */
+SEC("socket")
+__description("unpriv: nospec on a jump that becomes a nop")
+__success __success_unpriv
+__caps_unpriv(CAP_BPF)
+__retval(0)
+#ifdef SPEC_V1
+__xlated_unpriv("r1 = 0")
+__xlated_unpriv("nospec")
+__xlated_unpriv("goto pc+0")
+__xlated_unpriv("r0 = 0")
+__xlated_unpriv("exit")
+#endif
+__naked void nospec_jump_becomes_nop(void)
+{
+	asm volatile ("					\
+	call %[bpf_get_prandom_u32];			\
+	r0 &= 0xff;					\
+	r1 = 0;						\
+	if r0 < 0x100 goto l0_%=;			\
+	/* executed only speculatively */		\
+	r1 = r10;					\
+l0_%=:	if r1 == 0 goto l1_%=;				\
+	r0 = 1;						\
+l1_%=:	r0 = 0;						\
+	exit;						\
+"	:
+	: __imm(bpf_get_prandom_u32)
+	: __clobber_all);
+}
+
 char _license[] SEC("license") = "GPL";

-- 
2.43.0

[PATCH bpf-next 5/5] selftests/bpf: Test log_true_size with an error from blinding

[Qiliang Yuan]

A prog of one function gets its constants blinded when its runtime is
selected, which can log an error.

No test checks that log_true_size counts such an error.

Load a prog whose jump goes over 11000 constants with bpf_jit_harden=2.
Blinding pushes the jump out of its 16-bit range, and the message must
be in the log and in log_true_size.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Closes: https://lore.kernel.org/bpf/DMKHZS5B5CNO.186F6YH1T6YU4@gmail.com/
Signed-off-by: Qiliang Yuan <odys.yuan@gmail.com>
---
 .../selftests/bpf/prog_tests/blinding_log.c        | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/blinding_log.c b/tools/testing/selftests/bpf/prog_tests/blinding_log.c
new file mode 100644
index 0000000000000..658a8494dae5b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/blinding_log.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+/* The verifier log of a prog whose constants are blinded */
+
+#include <test_progs.h>
+#include <linux/filter.h>
+#include "sysctl_helpers.h"
+
+#define NR_CONSTS 11000
+#define LOG_SIZE 65536
+
+/*
+ * A prog of one function gets its constants blinded when its runtime is
+ * selected. Each 'r0 += 1' becomes 3 insns, which pushes the jump over them
+ * out of its 16-bit range. Serial, because it sets bpf_jit_harden.
+ */
+void serial_test_blinding_log(void)
+{
+	const char *harden = "/proc/sys/net/core/bpf_jit_harden";
+	LIBBPF_OPTS(bpf_prog_load_opts, opts);
+	struct bpf_insn *insns;
+	char old[16] = {}, *log;
+	int i, cnt = 0, fd;
+
+	if (!is_jit_enabled()) {
+		printf("%s:SKIP:JIT is disabled\n", __func__);
+		test__skip();
+		return;
+	}
+
+	insns = calloc(NR_CONSTS + 3, sizeof(*insns));
+	log = calloc(1, LOG_SIZE);
+	if (!ASSERT_OK_PTR(insns, "insns") || !ASSERT_OK_PTR(log, "log"))
+		goto out;
+
+	insns[cnt++] = BPF_EMIT_CALL(BPF_FUNC_get_prandom_u32);
+	insns[cnt++] = BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, NR_CONSTS);
+	for (i = 0; i < NR_CONSTS; i++)
+		insns[cnt++] = BPF_ALU64_IMM(BPF_ADD, BPF_REG_0, 1);
+	insns[cnt++] = BPF_EXIT_INSN();
+
+	opts.log_buf = log;
+	opts.log_size = LOG_SIZE;
+	/* log stats only: level 1 logs each insn, which wraps the buffer */
+	opts.log_level = 4;
+	if (sysctl_set_or_fail(harden, old, "2"))
+		goto out;
+	fd = bpf_prog_load(BPF_PROG_TYPE_SOCKET_FILTER, NULL, "GPL", insns, cnt, &opts);
+	sysctl_set_or_fail(harden, NULL, old);
+	/* don't mind whether the interpreter takes the prog or the load fails */
+	if (fd >= 0)
+		close(fd);
+
+	/* expect the message about the jump in the log and in log_true_size */
+	ASSERT_HAS_SUBSTR(log, "cannot be patched due to 16-bit range", "log");
+	ASSERT_EQ(opts.log_true_size, strlen(log) + 1, "log_true_size");
+out:
+	free(insns);
+	free(log);
+}

-- 
2.43.0