[PATCH] target/i386/tcg: fix EIP truncation for wrapping 16-bit near branches

Paul Galbraith posted 1 patch 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/6c5fca8e-b9f4-48a0-a070-42f10d1f942b@app.fastmail.com
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>
There is a newer version of this series
target/i386/tcg/translate.c                |  18 +-
tests/tcg/i386/Makefile.softmmu-target     |   6 +-
tests/tcg/i386/system/test-branch-wrap16.c | 221 +++++++++++++++++++++
3 files changed, 237 insertions(+), 8 deletions(-)
create mode 100644 tests/tcg/i386/system/test-branch-wrap16.c
[PATCH] target/i386/tcg: fix EIP truncation for wrapping 16-bit near branches
Posted by Paul Galbraith 1 month ago
gen_jmp_rel() decided whether goto_tb chaining was safe by checking if
the branch target's *linear* address stayed on the same guest page as
the translation block, then treated that as proof the EIP addition
could not wrap. For a 16-bit branch (MO_16) that reasoning does not
hold: the wrap boundary is at 0x10000 in EIP, not in the linear
address, so two linear addresses can share a page while the
corresponding EIP values straddle 0x10000. On the CF_PCREL path this
let a wrapping CALL/JMP rel16 execute with EIP left at its raw,
untruncated value whenever the unwrapped target happened to land on
the same page as the branch itself.

Require additionally that cs_base be page aligned before allowing
goto_tb: then the EIP wrap boundary (mask + 1) maps to a page-aligned
linear address, crossing it would cross a page boundary, and the
same-page test genuinely excludes the wrap. A translation-time check
of the computed EIP would not be sound here, because a CF_PCREL TB is
matched by physical address and cs_base -- not virtual PC -- and may
later run at a different linear address, shifting EIP by a multiple
of the page size; the alignment of cs_base, by contrast, is part of
the TB key and stable across reuse. This also subsumes the previous
CODE32-only special case, which forced chaining off unconditionally
for every data16 branch in a 32-bit segment. 64-bit mode is unchanged
(cs_base is 0 there).

Adds a regression test that installs a 16-bit code segment at a
deliberately non-page-aligned base and runs the same wrapping jmp
rel16 from two sites 48 bytes apart -- one whose translation block
starts on a different page than the unwrapped target (must already
pass) and one on the same page (where the defect manifested).

Signed-off-by: Paul Galbraith <paul@galbraiths.ca>
---
 target/i386/tcg/translate.c                |  18 +-
 tests/tcg/i386/Makefile.softmmu-target     |   6 +-
 tests/tcg/i386/system/test-branch-wrap16.c | 221 +++++++++++++++++++++
 3 files changed, 237 insertions(+), 8 deletions(-)
 create mode 100644 tests/tcg/i386/system/test-branch-wrap16.c

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 2115c5cd24..c9755d28dd 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2027,9 +2027,6 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
     if (!CODE64(s)) {
         if (ot == MO_16) {
             mask = 0xffff;
-            if (tb_cflags(s->base.tb) & CF_PCREL && CODE32(s)) {
-                use_goto_tb = false;
-            }
         } else {
             mask = 0xffffffff;
         }
@@ -2039,11 +2036,18 @@ static void gen_jmp_rel(DisasContext *s, MemOp ot, int diff, int tb_num)
     if (tb_cflags(s->base.tb) & CF_PCREL) {
         tcg_gen_addi_tl(cpu_eip, cpu_eip, new_pc - s->pc_save);
         /*
-         * If we can prove the branch does not leave the page and we have
-         * no extra masking to apply (data16 branch in code32, see above),
-         * then we have also proven that the addition does not wrap.
+         * The same-page test proves that the addition does not wrap EIP
+         * only if the wrap boundary (mask + 1) corresponds to a
+         * page-aligned linear address, i.e. only if cs_base is page
+         * aligned: then crossing the boundary would cross a page
+         * boundary, which the test excludes.  This must not be relaxed
+         * to a translation-time check of EIP itself, because a CF_PCREL
+         * TB may later run at a different linear address, shifting EIP
+         * by a multiple of the page size.
          */
-        if (!use_goto_tb || !translator_is_same_page(&s->base, new_pc)) {
+        if (!use_goto_tb
+            || (s->cs_base & ~TARGET_PAGE_MASK) != 0
+            || !translator_is_same_page(&s->base, new_pc)) {
             tcg_gen_andi_tl(cpu_eip, cpu_eip, mask);
             use_goto_tb = false;
         }
diff --git a/tests/tcg/i386/Makefile.softmmu-target b/tests/tcg/i386/Makefile.softmmu-target
index 4096a1cf31..16d3473fb6 100644
--- a/tests/tcg/i386/Makefile.softmmu-target
+++ b/tests/tcg/i386/Makefile.softmmu-target
@@ -18,7 +18,11 @@ LDFLAGS=-Wl,-T$(LINK_SCRIPT) -Wl,-melf_i386
 CFLAGS+=-nostdlib -ggdb -O0 $(MINILIB_INC)
 LDFLAGS+=-static -nostdlib $(CRT_OBJS) $(MINILIB_OBJS) -lgcc
 
-TESTS+=$(MULTIARCH_TESTS)
+I386_SYSTEM_TEST_SRCS=$(wildcard $(I386_SYSTEM_SRC)/*.c)
+I386_SYSTEM_TESTS=$(patsubst $(I386_SYSTEM_SRC)/%.c,%,$(I386_SYSTEM_TEST_SRCS))
+VPATH+=$(I386_SYSTEM_SRC)
+
+TESTS+=$(MULTIARCH_TESTS) $(I386_SYSTEM_TESTS)
 EXTRA_RUNS+=$(MULTIARCH_RUNS)
 
 # building head blobs
diff --git a/tests/tcg/i386/system/test-branch-wrap16.c b/tests/tcg/i386/system/test-branch-wrap16.c
new file mode 100644
index 0000000000..76839895f8
--- /dev/null
+++ b/tests/tcg/i386/system/test-branch-wrap16.c
@@ -0,0 +1,221 @@
+/*
+ * Regression test: a 16-bit near JMP rel16 whose target wraps EIP through
+ * 0xFFFF must land at the truncated target, not at the unwrapped one.
+ *
+ * gen_jmp_rel() in target/i386/tcg/translate.c decides whether it is safe
+ * to chain translation blocks (goto_tb) by checking whether the *linear*
+ * address of the branch target lands on the same guest page as the start
+ * of the current translation block. For a 16-bit branch (MO_16) the wrap
+ * boundary is at 0x10000 in EIP, not in the linear address, so passing
+ * that same-page test does not prove the EIP addition did not wrap: two
+ * linear addresses can share a page while the corresponding EIP values
+ * straddle 0x10000. When that happens on the CF_PCREL path (system-mode
+ * TCG, i.e. here, not linux-user, which is why this is not alongside
+ * test-i386-code16.S) the masking AND is skipped and EIP is left with
+ * the raw, unwrapped, out-of-range value.
+ *
+ * This only shows up for a branch site whose *own* linear address and
+ * whose *unwrapped* target's linear address fall on the same page, which
+ * additionally requires cs_base not to be 4 KiB-aligned (with an aligned
+ * cs_base the 0x10000 wrap in EIP always coincides with a page boundary
+ * in the linear address, and the same-page test can never be fooled).
+ *
+ * The test below runs the identical wrapping JMP rel16 from two sites 48
+ * bytes apart in a purpose-built 16-bit code segment:
+ *
+ *   A (control): TB starts on a different page than the unwrapped target
+ *                -> masked correctly even before the fix -> must pass.
+ *   B (regression check): TB starts on the SAME page as the unwrapped
+ *                target -> this is exactly where the defect manifested.
+ *
+ * Both land on a small hand-encoded stub that records where it arrived
+ * (via a 32-bit-addressed store, using DS which is still the flat data
+ * segment throughout) and far-jumps back into 32-bit code to report it.
+ */
+
+#include <stdint.h>
+#include <minilib.h>
+
+struct gdt_desc {
+    uint16_t limit_lo;
+    uint16_t base_lo;
+    uint8_t  base_mid;
+    uint8_t  access;
+    uint8_t  limit_hi_flags;
+    uint8_t  base_hi;
+};
+
+struct gdtr {
+    uint16_t limit;
+    uint32_t base;
+} __attribute__((packed));
+
+/*
+ * Selectors 0x08/0x10 must describe exactly the same flat code/data
+ * segments boot.S already loaded into CS/DS/ES/SS/FS/GS: those registers
+ * are never reloaded here, so their cached (shadow) descriptor state has
+ * to remain valid against this replacement table.
+ */
+#define SEL_CODE32 0x08
+#define SEL_DATA32 0x10
+#define SEL_CODE16 0x18
+
+static struct gdt_desc test_gdt[4];
+static struct gdtr test_gdtr;
+
+static void set_desc(struct gdt_desc *d, uint32_t base, uint32_t limit,
+                      uint8_t access, uint8_t gran)
+{
+    d->limit_lo = limit & 0xffff;
+    d->base_lo = base & 0xffff;
+    d->base_mid = (base >> 16) & 0xff;
+    d->access = access;
+    d->limit_hi_flags = ((limit >> 16) & 0x0f) | (gran & 0xf0);
+    d->base_hi = (base >> 24) & 0xff;
+}
+
+/*
+ * A page (4 KiB) of headroom, rounded up to at runtime, so that
+ * cs_base = page_base(arena) + 0x10 is guaranteed NOT 4 KiB-aligned
+ * (cs_base & 0xfff == 0x10) while every other bit of cs_base's low 16
+ * bits is otherwise known exactly relative to a 4 KiB page boundary.
+ * That is what makes the branch-site/target page relationship below
+ * fall out of plain arithmetic instead of luck.
+ *
+ * This is computed at runtime rather than via an
+ * __attribute__((aligned(0x10000))) buffer deliberately: boot.S's
+ * multiboot header uses the AOUT-kludge (flags bit 0x10000), which
+ * assumes a single constant "file offset -> load address" mapping
+ * across the whole image. A 64 KiB-aligned .bss object makes the
+ * linker widen that segment's own file alignment past 4 KiB, which
+ * breaks that assumption and corrupts the load. Ordinary alignment on
+ * the buffer plus a runtime round-up avoids perturbing the segment
+ * layout entirely.
+ *
+ * No paging is set up by boot.S, so linear == physical here and a
+ * pointer into arena is directly usable as a linear address.
+ */
+#define ARENA_SIZE (0x11000 + 0x1000)
+static uint8_t arena[ARENA_SIZE] __attribute__((aligned(16)));
+
+/*
+ * Geometry (base = page-aligned-up(arena), cs_base = base + 0x10):
+ *
+ *   branch A  at IP 0xFFC0 -> linear base+0xFFD0, page base+0xF000
+ *   branch B  at IP 0xFFF0 -> linear base+0x10000, page base+0x10000
+ *   both target IP 0x0020  -> linear base+0x10030 (masked, correct)
+ *   unwrapped EIP for both is 0x10020 -> linear base+0x10030 (untruncated)
+ *
+ * A: unwrapped target's page (base+0x10000) differs from A's own TB
+ *    page (base+0xF000) -> gen_jmp_rel() emits the AND -> correct.
+ * B: unwrapped target's page (base+0x10000) is the SAME as B's own TB
+ *    page (base+0x10000) -> the AND used to be skipped -> defect.
+ */
+#define BRANCH_A_IP 0xFFC0u
+#define BRANCH_B_IP 0xFFF0u
+#define TARGET_IP   0x0020u
+#define ARENA_OFF(ip) (0x10u + (ip))
+
+static uint8_t *base;
+
+/* rel16 for "jmp" (3-byte instruction), measured from the next IP */
+#define A_REL ((uint16_t)((0x10000u + TARGET_IP) - (BRANCH_A_IP + 3u)))
+#define B_REL ((uint16_t)((0x10000u + TARGET_IP) - (BRANCH_B_IP + 3u)))
+
+#define RESULT_NONE  0u
+#define RESULT_RIGHT 1u
+#define RESULT_WRONG 2u
+static volatile uint32_t g_result;
+
+static void write_branch(uint32_t off, uint16_t rel)
+{
+    base[off + 0] = 0xE9;              /* jmp rel16 */
+    base[off + 1] = rel & 0xff;
+    base[off + 2] = (rel >> 8) & 0xff;
+}
+
+/*
+ * A landing stub: "mov byte [addr32], marker" (address-size override,
+ * since the default in a 16-bit code segment is 16-bit addressing) then
+ * a far jump back to 32-bit flat code ("data32 ljmp $sel, $off32").
+ */
+static void write_stub(uint32_t off, uint8_t marker, uint32_t ret_addr)
+{
+    uint32_t addr = (uint32_t)&g_result;
+    uint8_t *p = &base[off];
+    int i = 0;
+
+    p[i++] = 0x67;                      /* address-size override */
+    p[i++] = 0xC6;                      /* MOV r/m8, imm8 */
+    p[i++] = 0x05;                      /* modrm: disp32, no base */
+    p[i++] = addr & 0xff;
+    p[i++] = (addr >> 8) & 0xff;
+    p[i++] = (addr >> 16) & 0xff;
+    p[i++] = (addr >> 24) & 0xff;
+    p[i++] = marker;
+
+    p[i++] = 0x66;                      /* operand-size override */
+    p[i++] = 0xEA;                      /* JMP ptr16:32 (direct far jump) */
+    p[i++] = ret_addr & 0xff;
+    p[i++] = (ret_addr >> 8) & 0xff;
+    p[i++] = (ret_addr >> 16) & 0xff;
+    p[i++] = (ret_addr >> 24) & 0xff;
+    p[i++] = SEL_CODE32 & 0xff;
+    p[i++] = (SEL_CODE32 >> 8) & 0xff;
+}
+
+static void write_stubs(uint32_t ret_addr)
+{
+    write_stub(ARENA_OFF(TARGET_IP), RESULT_RIGHT, ret_addr);
+    write_stub(ARENA_OFF(TARGET_IP + 0x10000u), RESULT_WRONG, ret_addr);
+}
+
+int main(void)
+{
+    uint32_t cs_base;
+
+    base = (uint8_t *)(((uint32_t)arena + 0xFFFu) & ~0xFFFu);
+    cs_base = (uint32_t)base + 0x10;
+
+    /* entries 0/1/2 mirror boot.S's null/code32/data32 descriptors */
+    set_desc(&test_gdt[0], 0, 0, 0, 0);
+    set_desc(&test_gdt[1], 0, 0xFFFFF, 0x9b, 0xC0);
+    set_desc(&test_gdt[2], 0, 0xFFFFF, 0x93, 0xC0);
+    /* a genuine 16-bit code segment: G=0, D/B=0, byte-granular 64K limit */
+    set_desc(&test_gdt[3], cs_base, 0xFFFF, 0x9b, 0x00);
+
+    test_gdtr.limit = sizeof(test_gdt) - 1;
+    test_gdtr.base = (uint32_t)&test_gdt;
+
+    write_branch(ARENA_OFF(BRANCH_A_IP), A_REL);
+    write_branch(ARENA_OFF(BRANCH_B_IP), B_REL);
+
+    asm volatile("lgdt %0" : : "m"(test_gdtr) : "memory");
+
+    /* -- A: control. Must already pass before the fix. -- */
+    write_stubs((uint32_t)&&L_return_a);
+    g_result = RESULT_NONE;
+    asm volatile("ljmpl $0x18, $0xFFC0" : : : "memory");
+L_return_a:
+    if (g_result != RESULT_RIGHT) {
+        ml_printf("FAIL: control branch A landed wrong (result=%d)\n",
+                   (int)g_result);
+        return 1;
+    }
+    ml_printf("A (control, different page): landed correctly\n");
+
+    /* -- B: the regression check. -- */
+    write_stubs((uint32_t)&&L_return_b);
+    g_result = RESULT_NONE;
+    asm volatile("ljmpl $0x18, $0xFFF0" : : : "memory");
+L_return_b:
+    if (g_result != RESULT_RIGHT) {
+        ml_printf("FAIL: branch wraparound left EIP untruncated "
+                   "(result=%d)\n", (int)g_result);
+        return 1;
+    }
+    ml_printf("B (same page as unwrapped target): landed correctly\n");
+
+    ml_printf("PASS\n");
+    return 0;
+}
-- 
2.55.0.windows.3
Re: [PATCH] target/i386/tcg: fix EIP truncation for wrapping 16-bit near branches
Posted by Paul Galbraith 2 weeks, 2 days ago
Ping on this one.

Short version: in a 16-bit code segment, a near CALL or JMP rel16 that only
reaches its target by wrapping EIP through 0xFFFF runs with EIP left
untruncated. The SDM has the 16-bit form truncate EIP to 16 bits, so in my
test the branch should land at CS:0x0020, and instead execution carries on at
EIP 0x10020.

It's fiddly to hit. You need all four of these at once:

  - a 16-bit code segment, D/B clear, so it really is a rel16 and EIP really
    is masked to 16 bits

  - a cs_base that isn't page aligned. If it is aligned, the wrap always
    crosses a page boundary in the linear address too, and the existing
    same-page test happens to give the right answer

  - the untruncated linear target landing in the same guest page as the start
    of the TB, which is what makes gen_jmp_rel() skip the masking

  - a CF_PCREL translation. It goes away under -d nochain and under
    -accel tcg,one-insn-per-tb=on, both of which set CF_NO_GOTO_TB and force
    the masking back on

One thing I got wrong in v1: it dropped the data16-in-code32 guard on the
assumption that the new condition subsumed it. It doesn't. That guard is
there for a different case, EIP starting out above the mask, so v1 would have
swapped one problem for another. v2 does something narrower and leaves the
guard where it is.

The test is the same test with the same geometry, just split out into its own
patch and wired into tests/tcg/i386/system/meson.build now that the tcg tests
meson rework has landed.

I'll send v2 as a new thread shortly and link it back here.

Paul
Re: [PATCH] target/i386/tcg: fix EIP truncation for wrapping 16-bit near branches
Posted by Paul Galbraith 2 weeks, 1 day ago
v2 is out, now a two-patch series with the regression test split out:

https://lore.kernel.org/qemu-devel/20260910180517.97-1-paul@galbraiths.ca/

Paul

On Thu, 10 Sep 2026, at 13:13, Paul Galbraith wrote:
> Ping on this one.
> 
> Short version: in a 16-bit code segment, a near CALL or JMP rel16 that only
> reaches its target by wrapping EIP through 0xFFFF runs with EIP left
> untruncated. The SDM has the 16-bit form truncate EIP to 16 bits, so in my
> test the branch should land at CS:0x0020, and instead execution carries on at
> EIP 0x10020.
> 
> It's fiddly to hit. You need all four of these at once:
> 
>   - a 16-bit code segment, D/B clear, so it really is a rel16 and EIP really
>     is masked to 16 bits
> 
>   - a cs_base that isn't page aligned. If it is aligned, the wrap always
>     crosses a page boundary in the linear address too, and the existing
>     same-page test happens to give the right answer
> 
>   - the untruncated linear target landing in the same guest page as the start
>     of the TB, which is what makes gen_jmp_rel() skip the masking
> 
>   - a CF_PCREL translation. It goes away under -d nochain and under
>     -accel tcg,one-insn-per-tb=on, both of which set CF_NO_GOTO_TB and force
>     the masking back on
> 
> One thing I got wrong in v1: it dropped the data16-in-code32 guard on the
> assumption that the new condition subsumed it. It doesn't. That guard is
> there for a different case, EIP starting out above the mask, so v1 would have
> swapped one problem for another. v2 does something narrower and leaves the
> guard where it is.
> 
> The test is the same test with the same geometry, just split out into its own
> patch and wired into tests/tcg/i386/system/meson.build now that the tcg tests
> meson rework has landed.
> 
> I'll send v2 as a new thread shortly and link it back here.
> 
> Paul
>