[PATCH v1 0/2] accel/tcg: fix crash on instruction straddling address-space end

[PATCH v1 0/2] accel/tcg: fix crash on instruction straddling address-space end

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

translator_ld() asserts that a page-crossing instruction reads its two
pages from virtually-contiguous addresses (page0 + TARGET_PAGE_SIZE). On
32-bit targets a guest can defeat this by executing an instruction at
the very end of the address space, which wraps the second page back to
the start; the assert fires and QEMU aborts. This is a guest-triggered
host crash (DoS).

Patch 1 takes the second page from pc when the linear page0 +
TARGET_PAGE_SIZE does not land on pc's page, handling both the normal
contiguous case and the wraparound case without needing the
address-space width.

Patch 2 adds a multiboot regression test (tests/multiboot/wraparound)
that runs on qemu-system-i386 and aborts without the fix.

Tao Cui (2):
  accel/tcg: fix crash on instruction straddling address-space end
  tests/multiboot: add regression test for translator_ld wraparound

 accel/tcg/translator.c         | 16 ++++++------
 tests/multiboot/Makefile       |  7 +++++-
 tests/multiboot/run_test.sh    |  8 +++++-
 tests/multiboot/wraparound.c   | 46 ++++++++++++++++++++++++++++++++++
 tests/multiboot/wraparound.out |  5 ++++
 5 files changed, 72 insertions(+), 10 deletions(-)
 create mode 100644 tests/multiboot/wraparound.c
 create mode 100644 tests/multiboot/wraparound.out

-- 
2.43.0

[PATCH v1 1/2] accel/tcg: fix crash on instruction straddling address-space end

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

translator_ld() reads the bytes of a page-crossing instruction into a
contiguous buffer. When computing the second page address it assumes the
two pages are virtually contiguous (second = page0 + TARGET_PAGE_SIZE)
and asserts that the read pointer pc is on that page.

On targets whose address space is narrower than vaddr (the 32-bit
targets, e.g. i386 or ARM AArch32 Thumb), an instruction at the very end
of the address space wraps the second page back to the start of the
address space. The target advances its fetch PC as a target_ulong, so pc
wraps to 0, while the linear page0 + TARGET_PAGE_SIZE stays at
0x1_0000_0000; the assert fails and QEMU aborts. A guest can trigger this
by mapping the last page executable and running a straddling instruction,
i.e. a guest-triggered host crash (DoS).

Instead of asserting, take the second page from pc: if the linear
page0 + TARGET_PAGE_SIZE does not land on pc's page, use pc's page. This
is correct for both the normal contiguous case and the wraparound case,
without needing to know the address-space width.

Tested with a minimal i386 multiboot payload that executes an instruction
at 0xfffffffe: before this change QEMU aborts (translator.c:296), after it
translates and runs normally.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 accel/tcg/translator.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/accel/tcg/translator.c b/accel/tcg/translator.c
index cd7d079fe0..750c132bbc 100644
--- a/accel/tcg/translator.c
+++ b/accel/tcg/translator.c
@@ -283,16 +283,16 @@ static bool translator_ld(CPUArchState *env, DisasContextBase *db,
     }
 
     /*
-     * The read must conclude on the second page and not extend to a third.
-     *
-     * TODO: We could allow the two pages to be virtually discontiguous,
-     * since we already allow the two pages to be physically discontiguous.
-     * The only reasonable use case would be executing an insn at the end
-     * of the address space wrapping around to the beginning.  For that,
-     * we would need to know the current width of the address space.
-     * In the meantime, assert.
+     * The read must conclude on the second page and must not extend to a
+     * third.  An instruction that straddles the end of the address space
+     * wraps the second page back to the start of the address space, so the
+     * linear page0 + TARGET_PAGE_SIZE does not land on pc's page; in that
+     * case take the second page from pc instead of asserting.
      */
     base = (base & TARGET_PAGE_MASK) + TARGET_PAGE_SIZE;
+    if (((base ^ pc) & TARGET_PAGE_MASK) != 0) {
+        base = pc & TARGET_PAGE_MASK;
+    }
     assert(((base ^ pc) & TARGET_PAGE_MASK) == 0);
     assert(((base ^ last) & TARGET_PAGE_MASK) == 0);
     host = db->host_addr[1];
-- 
2.43.0

[PATCH v1 2/2] tests/multiboot: add regression test for translator_ld wraparound

[Tao Cui]

From: Tao Cui <cuitao@kylinos.cn>

Add a multiboot test case that reaches an instruction straddling the
end of the 32-bit address space (0xfffffffe). Before the translator_ld
fix QEMU aborts at translator.c:296; afterwards the case reaches
isa-debug-exit and passes.

The top page (0xfffff000) is SeaBIOS ROM, so the cross-boundary byte is
the ROM's own 0x00 (add r/m8, r8) at 0xffffffff, whose modrm is fetched
from [0x0]. A short exit stub is placed there. The case runs on
qemu-system-i386 since the bug is 32-bit only.

Signed-off-by: Tao Cui <cuitao@kylinos.cn>
---
 tests/multiboot/Makefile       |  7 +++++-
 tests/multiboot/run_test.sh    |  8 +++++-
 tests/multiboot/wraparound.c   | 46 ++++++++++++++++++++++++++++++++++
 tests/multiboot/wraparound.out |  5 ++++
 4 files changed, 64 insertions(+), 2 deletions(-)
 create mode 100644 tests/multiboot/wraparound.c
 create mode 100644 tests/multiboot/wraparound.out

diff --git a/tests/multiboot/Makefile b/tests/multiboot/Makefile
index ed4225e7d1..b5045855d7 100644
--- a/tests/multiboot/Makefile
+++ b/tests/multiboot/Makefile
@@ -9,7 +9,7 @@ LIBS=$(shell $(CC) $(CCFLAGS) -print-libgcc-file-name)
 
 AOUT_KLUDGE_BIN=$(foreach x,$(shell seq 1 9),aout_kludge_$x.bin)
 
-all: mmap.elf modules.elf $(AOUT_KLUDGE_BIN)
+all: mmap.elf modules.elf wraparound.elf $(AOUT_KLUDGE_BIN)
 
 mmap.elf: start.o mmap.o libc.o link.ld
 	$(LD) $(LDFLAGS_ELF) -o $@ $^ $(LIBS)
@@ -17,6 +17,11 @@ mmap.elf: start.o mmap.o libc.o link.ld
 modules.elf: start.o modules.o libc.o link.ld
 	$(LD) $(LDFLAGS_ELF) -o $@ $^ $(LIBS)
 
+# link.ld is provided via LDFLAGS_ELF (-T); keep it out of the
+# prerequisite list so $^ does not pass it to ld a second time.
+wraparound.elf: start.o wraparound.o libc.o
+	$(LD) $(LDFLAGS_ELF) -o $@ $^ $(LIBS)
+
 aout_kludge_%.bin: aout_kludge_%.o link.ld
 	$(LD) $(LDFLAGS_BIN) -o $@ $^ $(LIBS)
 
diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh
index f968bf797e..517281ba49 100755
--- a/tests/multiboot/run_test.sh
+++ b/tests/multiboot/run_test.sh
@@ -73,9 +73,15 @@ aout_kludge() {
     done
 }
 
+wraparound() {
+    # The translator_ld wraparound bug is i386-only (32-bit address space);
+    # the default x86_64 QEMU does not reproduce it.
+    QEMU="${QEMU%x86_64}i386" run_qemu wraparound.elf -m 4G
+}
+
 make all
 
-for t in mmap modules aout_kludge; do
+for t in mmap modules aout_kludge wraparound; do
 
     echo > test.log
     pass=1
diff --git a/tests/multiboot/wraparound.c b/tests/multiboot/wraparound.c
new file mode 100644
index 0000000000..3b757f3f98
--- /dev/null
+++ b/tests/multiboot/wraparound.c
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Regression test for the translator_ld crash when an instruction
+ * straddles the end of the 32-bit address space (i386).
+ */
+
+#include "libc.h"
+#include "multiboot.h"
+
+int test_main(uint32_t magic, struct mb_info *mbi)
+{
+    (void) magic;
+    (void) mbi;
+
+    printf("cross-boundary insn at end of address space\n");
+
+    /*
+     * The top page (0xfffff000) is SeaBIOS ROM and cannot be written.
+     * Its byte at 0xffffffff (0x00 = "add r/m8, r8") already crosses the
+     * page boundary into page1 at 0x0, which is exactly the case
+     * translator_ld must handle without aborting.  Reaching 0xfffffffe
+     * runs the ROM's cld, then that add; the add's modrm is fetched from
+     * [0x0], which is RAM, so build a short exit stub there:
+     *
+     *   [0x0] c0                 modrm -> "add al, al" (reg; EIP -> 1)
+     *   [1] eb 00                jmp +0          (EIP -> 3)
+     *   [3] b8 00 00 00 00       mov eax, 0
+     *   [8] e7 f4                out 0xf4, eax   -> isa-debug-exit(0)
+     *
+     * Without the translator_ld fix QEMU aborts while reading the modrm
+     * at the wrapped address 0x0 (translator.c assert).
+     *
+     * Note: this relies on the SeaBIOS byte at 0xffffffff being 0x00
+     * (add r/m8, r8); if that ever changes, the stub below must move.
+     */
+    volatile uint8_t *p = (uint8_t *)0x00000000;
+    p[0] = 0xC0;                        /* modrm: add al, al */
+    p[1] = 0xEB; p[2] = 0x00;           /* jmp +0 -> 0x3 */
+    /* mov eax, 0 */
+    p[3] = 0xB8; p[4] = 0x00; p[5] = 0x00; p[6] = 0x00; p[7] = 0x00;
+    p[8] = 0xE7; p[9] = 0xF4;           /* out 0xf4, eax -> exit 0 */
+
+    asm volatile("mov $0xFFFFFFFE, %%eax; jmp *%%eax" : : : "eax");
+
+    return 1; /* unreachable */
+}
diff --git a/tests/multiboot/wraparound.out b/tests/multiboot/wraparound.out
new file mode 100644
index 0000000000..23cb05ed11
--- /dev/null
+++ b/tests/multiboot/wraparound.out
@@ -0,0 +1,5 @@
+
+
+=== Running test case: wraparound.elf -m 4G ===
+
+cross-boundary insn at end of address space
-- 
2.43.0