[v3,bpf] bpf: Account for BPF_FETCH in insn_has_def32()

Commit Message

Ilya Leoshkevich March 1, 2021, 3:40 p.m. UTC

insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes
adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1].
This happens because insn_no_def() does not know about the BPF_FETCH
variants of BPF_STX.

Fix in two steps.

First, replace insn_no_def() with insn_def_regno(), which returns the
register an insn defines. Normally insn_no_def() calls are followed by
insn->dst_reg uses; replace those with the insn_def_regno() return
value.

Second, adjust the BPF_STX special case in is_reg64() to deal with
queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state
information is no longer available. Add a comment, since the purpose
of this special case is not clear at first glance.

[1] https://lore.kernel.org/bpf/[e-mail]/

Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
Signed-off-by: Ilya Leoshkevich <[e-mail]>
Acked-by: Martin KaFai Lau <[e-mail]>
---

v1: https://lore.kernel.org/bpf/[e-mail]/
v1 -> v2: Per Martin's comments: rebase against the bpf branch, fix the
          Fixes: tag, fix the comment style, replace ?: with the more
          readable if-else, handle the internal verifier error using
          WARN_ON_ONCE(), verbose() and -EFAULT.

v2: https://lore.kernel.org/bpf/[e-mail]/
v2 -> v3: Per Brendan's comment, add "verifier bug." to the error
          message. Unfortunately, the load_reg assignment cannot be
          moved, because this would also require moving the insn
          assignment, and this would ruin the reverse xmas tree.

 kernel/bpf/verifier.c | 70 ++++++++++++++++++++++++-------------------
 1 file changed, 39 insertions(+), 31 deletions(-)

Comments

Brendan Jackman March 2, 2021, 10:24 a.m. UTC | #1

Thanks!

On Mon, 1 Mar 2021 at 16:40, Ilya Leoshkevich <[e-mail]> wrote:
>
> insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes
> adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1].
> This happens because insn_no_def() does not know about the BPF_FETCH
> variants of BPF_STX.
>
> Fix in two steps.
>
> First, replace insn_no_def() with insn_def_regno(), which returns the
> register an insn defines. Normally insn_no_def() calls are followed by
> insn->dst_reg uses; replace those with the insn_def_regno() return
> value.
>
> Second, adjust the BPF_STX special case in is_reg64() to deal with
> queries made from opt_subreg_zext_lo32_rnd_hi32(), where the state
> information is no longer available. Add a comment, since the purpose
> of this special case is not clear at first glance.
>
> [1] https://lore.kernel.org/bpf/[e-mail]/
>
> Fixes: 5ffa25502b5a ("bpf: Add instructions for atomic_[cmp]xchg")
> Signed-off-by: Ilya Leoshkevich <[e-mail]>
> Acked-by: Martin KaFai Lau <[e-mail]>
> ---
>
> v1: https://lore.kernel.org/bpf/[e-mail]/
> v1 -> v2: Per Martin's comments: rebase against the bpf branch, fix the
>           Fixes: tag, fix the comment style, replace ?: with the more
>           readable if-else, handle the internal verifier error using
>           WARN_ON_ONCE(), verbose() and -EFAULT.
>
> v2: https://lore.kernel.org/bpf/[e-mail]/
> v2 -> v3: Per Brendan's comment, add "verifier bug." to the error
>           message. Unfortunately, the load_reg assignment cannot be
>           moved, because this would also require moving the insn
>           assignment, and this would ruin the reverse xmas tree.

Acked-by: Brendan Jackman <[e-mail]>

[e-mail] March 4, 2021, 3:10 p.m. UTC | #2

Hello:

This patch was applied to bpf/bpf.git (refs/heads/master):

On Mon,  1 Mar 2021 16:40:19 +0100 you wrote:
> insn_has_def32() returns false for 32-bit BPF_FETCH insns. This makes
> adjust_insn_aux_data() incorrectly set zext_dst, as can be seen in [1].
> This happens because insn_no_def() does not know about the BPF_FETCH
> variants of BPF_STX.
> 
> Fix in two steps.
> 
> [...]

Here is the summary with links:
  - [v3,bpf] bpf: Account for BPF_FETCH in insn_has_def32()
    https://git.kernel.org/bpf/bpf/c/83a2881903f3

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

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3d34ba492d46..bb3eaab934f3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -1703,7 +1703,11 @@  static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	}
 
 	if (class == BPF_STX) {
-		if (reg->type != SCALAR_VALUE)
+		/* BPF_STX (including atomic variants) has multiple source
+		 * operands, one of which is a ptr. Check whether the caller is
+		 * asking about it.
+		 */
+		if (t == SRC_OP && reg->type != SCALAR_VALUE)
 			return true;
 		return BPF_SIZE(code) == BPF_DW;
 	}
@@ -1735,22 +1739,38 @@  static bool is_reg64(struct bpf_verifier_env *env, struct bpf_insn *insn,
 	return true;
 }
 
-/* Return TRUE if INSN doesn't have explicit value define. */
-static bool insn_no_def(struct bpf_insn *insn)
+/* Return the regno defined by the insn, or -1. */
+static int insn_def_regno(const struct bpf_insn *insn)
 {
-	u8 class = BPF_CLASS(insn->code);
-
-	return (class == BPF_JMP || class == BPF_JMP32 ||
-		class == BPF_STX || class == BPF_ST);
+	switch (BPF_CLASS(insn->code)) {
+	case BPF_JMP:
+	case BPF_JMP32:
+	case BPF_ST:
+		return -1;
+	case BPF_STX:
+		if (BPF_MODE(insn->code) == BPF_ATOMIC &&
+		    (insn->imm & BPF_FETCH)) {
+			if (insn->imm == BPF_CMPXCHG)
+				return BPF_REG_0;
+			else
+				return insn->src_reg;
+		} else {
+			return -1;
+		}
+	default:
+		return insn->dst_reg;
+	}
 }
 
 /* Return TRUE if INSN has defined any 32-bit value explicitly. */
 static bool insn_has_def32(struct bpf_verifier_env *env, struct bpf_insn *insn)
 {
-	if (insn_no_def(insn))
+	int dst_reg = insn_def_regno(insn);
+
+	if (dst_reg == -1)
 		return false;
 
-	return !is_reg64(env, insn, insn->dst_reg, NULL, DST_OP);
+	return !is_reg64(env, insn, dst_reg, NULL, DST_OP);
 }
 
 static void mark_insn_zext(struct bpf_verifier_env *env,
@@ -11006,9 +11026,10 @@  static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 	for (i = 0; i < len; i++) {
 		int adj_idx = i + delta;
 		struct bpf_insn insn;
-		u8 load_reg;
+		int load_reg;
 
 		insn = insns[adj_idx];
+		load_reg = insn_def_regno(&insn);
 		if (!aux[adj_idx].zext_dst) {
 			u8 code, class;
 			u32 imm_rnd;
@@ -11018,14 +11039,14 @@  static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 
 			code = insn.code;
 			class = BPF_CLASS(code);
-			if (insn_no_def(&insn))
+			if (load_reg == -1)
 				continue;
 
 			/* NOTE: arg "reg" (the fourth one) is only used for
-			 *       BPF_STX which has been ruled out in above
-			 *       check, it is safe to pass NULL here.
+			 *       BPF_STX + SRC_OP, so it is safe to pass NULL
+			 *       here.
 			 */
-			if (is_reg64(env, &insn, insn.dst_reg, NULL, DST_OP)) {
+			if (is_reg64(env, &insn, load_reg, NULL, DST_OP)) {
 				if (class == BPF_LD &&
 				    BPF_MODE(code) == BPF_IMM)
 					i++;
@@ -11040,7 +11061,7 @@  static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 			imm_rnd = get_random_int();
 			rnd_hi32_patch[0] = insn;
 			rnd_hi32_patch[1].imm = imm_rnd;
-			rnd_hi32_patch[3].dst_reg = insn.dst_reg;
+			rnd_hi32_patch[3].dst_reg = load_reg;
 			patch = rnd_hi32_patch;
 			patch_len = 4;
 			goto apply_patch_buffer;
@@ -11049,22 +11070,9 @@  static int opt_subreg_zext_lo32_rnd_hi32(struct bpf_verifier_env *env,
 		if (!bpf_jit_needs_zext())
 			continue;
 
-		/* zext_dst means that we want to zero-extend whatever register
-		 * the insn defines, which is dst_reg most of the time, with
-		 * the notable exception of BPF_STX + BPF_ATOMIC + BPF_FETCH.
-		 */
-		if (BPF_CLASS(insn.code) == BPF_STX &&
-		    BPF_MODE(insn.code) == BPF_ATOMIC) {
-			/* BPF_STX + BPF_ATOMIC insns without BPF_FETCH do not
-			 * define any registers, therefore zext_dst cannot be
-			 * set.
-			 */
-			if (WARN_ON(!(insn.imm & BPF_FETCH)))
-				return -EINVAL;
-			load_reg = insn.imm == BPF_CMPXCHG ? BPF_REG_0
-							   : insn.src_reg;
-		} else {
-			load_reg = insn.dst_reg;
+		if (WARN_ON(load_reg == -1)) {
+			verbose(env, "verifier bug. zext_dst is set, but no reg is defined\n");
+			return -EFAULT;
 		}
 
 		zext_patch[0] = insn;