[PATCH bpf v2 2/2] bpf, x86: Fix sign-extension moves in the i386 JIT

[Nicholas Dudar]

RFC9669 defines MOVSX, encoded as a register MOV with an offset of 8,
16 or 32, as dst = (s8,s16,s32)src, which the interpreter implements.
The i386 JIT ignores the offset and emits a plain move, so r0 = (s8)r0
with 0x80 returns 128 instead of -128. A verifier-accepted program can
therefore use values outside the range proved by the verifier.

Sign-extend the low 8, 16 or 32 bits of the source. For ALU64, fill the
high word with the sign; for ALU32, zero it unless the verifier inserts
the zero extension.

On an unpatched i386 kernel, a MOVSX test program passed a helper a
length beyond the verifier's bound. It stayed within the bound after
this change.

Found in a follow-up sweep of the JIT sources for instructions that are
selected by the offset field, and by reading the i386 MOV handler.

On i386 under qemu, with test_progs built as a 32-bit binary from the
same tree:

  # ./test_progs -t verifier_movsx,verifier_sdiv,jit_insns \
    -d jit_insns_blinded,'verifier_movsx/*@unpriv' -w 600
  [...]
  #191/28  jit_insns/movsx:OK
  [...]
  #649/1   verifier_movsx/MOV32SX, S8:OK
  #649/3   verifier_movsx/MOV32SX, S16:OK
  #649/5   verifier_movsx/MOV64SX, S8:OK
  #649/7   verifier_movsx/MOV64SX, S16:OK
  [...]
  #649     verifier_movsx:OK
  Summary: 2/172 PASSED, 16 SKIPPED, 1/6 FAILED

Without this change, the five subtests above fail. The six failures
left are jit_insns subtests that fail without the series as well.

Fixes: 8100928c8814 ("bpf: Support new sign-extension mov insns")
Signed-off-by: Nicholas Dudar <main.kalliope@xxxxxxxxx>
---
 arch/x86/net/bpf_jit_comp32.c | 51 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index 012161da3e82..2ed3f8aad8d5 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -266,6 +266,51 @@ static inline void emit_ia32_mov_r64(const bool is64, const u8 dst[],
 		emit_ia32_mov_i(dst_hi, 0, dstk, pprog);
 }
 
+/* dst = sign_extend(src, insn->off) */
+static inline void emit_ia32_movsx_r64(const struct bpf_insn *insn, u8 **pprog,
+				       const struct bpf_prog_aux *aux)
+{
+	const u8 *dst = bpf2ia32[insn->dst_reg];
+	const u8 *src = bpf2ia32[insn->src_reg];
+	bool dstk = insn->dst_reg != BPF_REG_AX;
+	bool sstk = insn->src_reg != BPF_REG_AX;
+	bool is64 = BPF_CLASS(insn->code) == BPF_ALU64;
+	u8 *prog = *pprog;
+	int cnt = 0;
+
+	if (sstk)
+		/* mov eax,dword ptr [ebp+off] */
+		EMIT3(0x8B, add_2reg(0x40, IA32_EBP, IA32_EAX),
+		      STACK_VAR(src_lo));
+	else
+		/* mov eax,src_lo */
+		EMIT2(0x89, add_2reg(0xC0, IA32_EAX, src_lo));
+
+	switch (insn->off) {
+	case 8:
+		/* movsx eax,al */
+		EMIT3(0x0F, 0xBE, 0xC0);
+		break;
+	case 16:
+		/* movsx eax,ax */
+		EMIT3(0x0F, 0xBF, 0xC0);
+		break;
+	case 32:
+		/* EAX already holds the low word; CDQ supplies the high word. */
+		break;
+	}
+
+	emit_ia32_mov_r(dst_lo, IA32_EAX, dstk, false, &prog);
+	if (is64) {
+		EMIT1(0x99); /* cdq */
+		emit_ia32_mov_r(dst_hi, IA32_EDX, dstk, false, &prog);
+	} else if (!aux->verifier_zext) {
+		emit_ia32_mov_i(dst_hi, 0, dstk, &prog);
+	}
+
+	*pprog = prog;
+}
+
 /* Sign extended move */
 static inline void emit_ia32_mov_i64(const bool is64, const u8 dst[],
 				     const u32 val, bool dstk, u8 **pprog)
@@ -1706,6 +1751,12 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
 					emit_ia32_mov_i(dst_hi, 0, dstk, &prog);
 					break;
 				}
+				if (insn->off == 8 || insn->off == 16 ||
+				    (is64 && insn->off == 32)) {
+					emit_ia32_movsx_r64(insn, &prog,
+							    bpf_prog->aux);
+					break;
+				}
 				emit_ia32_mov_r64(is64, dst, src, dstk, sstk,
 						  &prog, bpf_prog->aux);
 				break;

-- 
2.53.0