[PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Ilya Leoshkevich]

Hi,

I noticed that when we get a SEGV due to jumping to non-readable
memory, sometimes si_addr and program counter in siginfo_t are slightly
off. I tracked this down to the assumption that translators stop before
the end of a page, while in reality they may stop right after it.

Patch 1 fixes an invalidation issue, which may prevent SEGV from
happening altogether.
Patches 2-3 fix the main issue on x86_64 and s390x. Many other
architectures have fixed-size instructions and are not affected.
Patch 4 adds tests.

Note: this series depends on [1].

Best regards,
Ilya

v1: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00822.html
v1 -> v2: Fix individual translators instead of translator_loop
          (Peter).

v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01079.html
v2 -> v3: Peek at the next instruction on s390x (Richard).
          Undo more on i386 (Richard).
          Check PAGE_EXEC, not PAGE_READ (Peter, Richard).

v3: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01306.html
v3 -> v4: Improve the commit message in patch 1 to better reflect what
          exactly is being fixed there.
          Factor out the is_same_page() patch (Richard).
          Do not touch the common code in the i386 fix (Richard).

v4: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01747.html
v4 -> v5: Drop patch 2.
          Use a different fix for the invalidation issue based on
          discussion with Richard [2].

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02472.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02556.html

Ilya Leoshkevich (4):
  linux-user: Clear tb_jmp_cache on mprotect()
  target/s390x: Make translator stop before the end of a page
  target/i386: Make translator stop before the end of a page
  tests/tcg: Test siginfo_t contents when jumping to non-readable pages

 linux-user/mmap.c                |  14 +++
 target/i386/tcg/translate.c      |  25 +++++-
 target/s390x/tcg/translate.c     |  15 +++-
 tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
 tests/tcg/s390x/Makefile.target  |   1 +
 tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
 tests/tcg/x86_64/Makefile.target |   3 +-
 tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
 8 files changed, 427 insertions(+), 6 deletions(-)
 create mode 100644 tests/tcg/multiarch/noexec.h
 create mode 100644 tests/tcg/s390x/noexec.c
 create mode 100644 tests/tcg/x86_64/noexec.c

-- 
2.37.1

[PATCH v5 1/4] linux-user: Clear tb_jmp_cache on mprotect()

[Ilya Leoshkevich]

Currently it's possible to execute pages that do not have PAGE_EXEC if
there is an existing translation block. Fix by clearing tb_jmp_cache,
which forces HELPER(lookup_tb_ptr)() to recheck permission bits the
next time.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 linux-user/mmap.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/linux-user/mmap.c b/linux-user/mmap.c
index bbc674311b..bd96c876ba 100644
--- a/linux-user/mmap.c
+++ b/linux-user/mmap.c
@@ -115,6 +115,7 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
 {
     abi_ulong end, host_start, host_end, addr;
     int prot1, ret, page_flags, host_prot;
+    CPUState *cpu;
 
     trace_target_mprotect(start, len, target_prot);
 
@@ -178,6 +179,19 @@ int target_mprotect(abi_ulong start, abi_ulong len, int target_prot)
         }
     }
     page_set_flags(start, start + len, page_flags);
+
+    /*
+     * Unlike target_mmap(), target_munmap() and target_mremap(), we don't need
+     * to call tb_invalidate_phys_range() here, since pages still hold the same
+     * data.  However, tb_jmp_cache needs to be cleared, otherwise after
+     * clearing PAGE_EXEC it would still be possible to jump to the existing
+     * translation blocks.  We just clear the whole cache here: mprotect() is
+     * rare enough, so a more fine-grained approach is not necessary.
+     */
+    CPU_FOREACH(cpu) {
+        cpu_tb_jmp_cache_clear(cpu);
+    }
+
     mmap_unlock();
     return 0;
 error:
-- 
2.37.1

[PATCH v5 2/4] target/s390x: Make translator stop before the end of a page

[Ilya Leoshkevich]

Right now translator stops right *after* the end of a page, which
breaks reporting of fault locations when the last instruction of a
multi-insn translation block crosses a page boundary.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/s390x/tcg/translate.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index d4c0b9b3a2..1d2dddab1c 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6609,6 +6609,14 @@ static void s390x_tr_insn_start(DisasContextBase *dcbase, CPUState *cs)
     dc->insn_start = tcg_last_op();
 }
 
+static target_ulong get_next_pc(CPUS390XState *env, DisasContext *s,
+                                uint64_t pc)
+{
+    uint64_t insn = ld_code2(env, s, pc);
+
+    return pc + get_ilen((insn >> 8) & 0xff);
+}
+
 static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 {
     CPUS390XState *env = cs->env_ptr;
@@ -6616,10 +6624,9 @@ static void s390x_tr_translate_insn(DisasContextBase *dcbase, CPUState *cs)
 
     dc->base.is_jmp = translate_one(env, dc);
     if (dc->base.is_jmp == DISAS_NEXT) {
-        uint64_t page_start;
-
-        page_start = dc->base.pc_first & TARGET_PAGE_MASK;
-        if (dc->base.pc_next - page_start >= TARGET_PAGE_SIZE || dc->ex_value) {
+        if (!is_same_page(dcbase, dc->base.pc_next) ||
+            !is_same_page(dcbase, get_next_pc(env, dc, dc->base.pc_next)) ||
+            dc->ex_value) {
             dc->base.is_jmp = DISAS_TOO_MANY;
         }
     }
-- 
2.37.1

[PATCH v5 3/4] target/i386: Make translator stop before the end of a page

[Ilya Leoshkevich]

Right now translator stops right *after* the end of a page, which
breaks reporting of fault locations when the last instruction of a
multi-insn translation block crosses a page boundary.

An implementation, like the one arm and s390x have, would require an
i386 length disassembler, which is burdensome to maintain. Another
alternative would be to single-step at the end of a guest page, but
this may come with a performance impact.

Fix by snapshotting disassembly state and restoring it after we figure
out we crossed a page boundary. This includes rolling back cc_op
updates and emitted ops.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1143
---
 target/i386/tcg/translate.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 4836c889e0..6481ae5c24 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -130,6 +130,7 @@ typedef struct DisasContext {
     TCGv_i64 tmp1_i64;
 
     sigjmp_buf jmpbuf;
+    TCGOp *prev_insn_end;
 } DisasContext;
 
 /* The environment in which user-only runs is constrained. */
@@ -2008,6 +2009,12 @@ static uint64_t advance_pc(CPUX86State *env, DisasContext *s, int num_bytes)
 {
     uint64_t pc = s->pc;
 
+    /* This is a subsequent insn that crosses a page boundary.  */
+    if (s->base.num_insns > 1 &&
+        !is_same_page(&s->base, s->pc + num_bytes - 1)) {
+        siglongjmp(s->jmpbuf, 2);
+    }
+
     s->pc += num_bytes;
     if (unlikely(s->pc - s->pc_start > X86_MAX_INSN_LENGTH)) {
         /* If the instruction's 16th byte is on a different page than the 1st, a
@@ -4556,6 +4563,8 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
     int modrm, reg, rm, mod, op, opreg, val;
     target_ulong next_eip, tval;
     target_ulong pc_start = s->base.pc_next;
+    bool orig_cc_op_dirty = s->cc_op_dirty;
+    CCOp orig_cc_op = s->cc_op;
 
     s->pc_start = s->pc = pc_start;
     s->override = -1;
@@ -4568,9 +4577,22 @@ static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
     s->rip_offset = 0; /* for relative ip address */
     s->vex_l = 0;
     s->vex_v = 0;
-    if (sigsetjmp(s->jmpbuf, 0) != 0) {
+    switch (sigsetjmp(s->jmpbuf, 0)) {
+    case 0:
+        break;
+    case 1:
         gen_exception_gpf(s);
         return s->pc;
+    case 2:
+        /* Restore state that may affect the next instruction. */
+        s->cc_op_dirty = orig_cc_op_dirty;
+        s->cc_op = orig_cc_op;
+        s->base.num_insns--;
+        tcg_remove_ops_after(s->prev_insn_end);
+        s->base.is_jmp = DISAS_TOO_MANY;
+        return pc_start;
+    default:
+        g_assert_not_reached();
     }
 
     prefixes = 0;
@@ -8632,6 +8654,7 @@ static void i386_tr_insn_start(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
 
+    dc->prev_insn_end = tcg_last_op();
     tcg_gen_insn_start(dc->base.pc_next, dc->cc_op);
 }
 
-- 
2.37.1

[PATCH v5 4/4] tests/tcg: Test siginfo_t contents when jumping to non-readable pages

[Ilya Leoshkevich]

Add x86_64 and s390x tests to prevent regressions.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
 tests/tcg/s390x/Makefile.target  |   1 +
 tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
 tests/tcg/x86_64/Makefile.target |   3 +-
 tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
 5 files changed, 378 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/multiarch/noexec.h
 create mode 100644 tests/tcg/s390x/noexec.c
 create mode 100644 tests/tcg/x86_64/noexec.c

diff --git a/tests/tcg/multiarch/noexec.h b/tests/tcg/multiarch/noexec.h
new file mode 100644
index 0000000000..a76e0aa9ea
--- /dev/null
+++ b/tests/tcg/multiarch/noexec.h
@@ -0,0 +1,114 @@
+/*
+ * Common code for arch-specific MMU_INST_FETCH fault testing.
+ *
+ * Declare struct arch_noexec_test before including this file and define
+ * arch_check_mcontext() after that.
+ */
+
+#include <assert.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/ucontext.h>
+#include <unistd.h>
+
+/* Forward declarations. */
+
+static void arch_check_mcontext(const struct arch_noexec_test *test,
+                                const mcontext_t *ctx);
+
+/* Utility functions. */
+
+static void safe_print(const char *s)
+{
+    write(0, s, strlen(s));
+}
+
+static void safe_puts(const char *s)
+{
+    safe_print(s);
+    safe_print("\n");
+}
+
+#define PAGE_ALIGN(p) (void *)((unsigned long)(p) & ~0xfffUL)
+
+/* Testing infrastructure. */
+
+struct noexec_test {
+    const char *name;
+    void (*func)(int);
+    void *page;
+    void *expected_si_addr;
+    struct arch_noexec_test arch;
+};
+
+static const struct noexec_test *current_noexec_test;
+
+static void handle_segv(int sig, siginfo_t *info, void *ucontext)
+{
+    int err;
+
+    if (current_noexec_test == NULL) {
+        safe_puts("[  FAILED  ] unexpected SEGV");
+        _exit(1);
+    }
+
+    if (info->si_addr != current_noexec_test->expected_si_addr) {
+        safe_puts("[  FAILED  ] wrong si_addr");
+        _exit(1);
+    }
+
+    arch_check_mcontext(&current_noexec_test->arch,
+                        &((ucontext_t *)ucontext)->uc_mcontext);
+
+    err = mprotect(current_noexec_test->page, 0x1000, PROT_READ | PROT_EXEC);
+    if (err != 0) {
+        safe_puts("[  FAILED  ] mprotect() failed");
+        _exit(1);
+    }
+
+    current_noexec_test = NULL;
+}
+
+static void test_noexec_1(const struct noexec_test *test)
+{
+    int ret;
+
+    /* Trigger TB creation in order to test invalidation. */
+    test->func(0);
+
+    ret = mprotect(test->page, 0x1000, PROT_NONE);
+    assert(ret == 0);
+
+    /* Trigger SEGV and check that handle_segv() ran. */
+    current_noexec_test = test;
+    test->func(0);
+    assert(current_noexec_test == NULL);
+}
+
+static int test_noexec(struct noexec_test *tests, size_t n_tests)
+{
+    struct sigaction act;
+    size_t i;
+    int err;
+
+    memset(&act, 0, sizeof(act));
+    act.sa_sigaction = handle_segv;
+    act.sa_flags = SA_SIGINFO;
+    err = sigaction(SIGSEGV, &act, NULL);
+    assert(err == 0);
+
+    for (i = 0; i < n_tests; i++) {
+        struct noexec_test *test = &tests[i];
+
+        safe_print("[ RUN      ] ");
+        safe_puts(test->name);
+        test_noexec_1(test);
+        safe_puts("[       OK ]");
+    }
+
+    safe_puts("[  PASSED  ]");
+
+    return EXIT_SUCCESS;
+}
diff --git a/tests/tcg/s390x/Makefile.target b/tests/tcg/s390x/Makefile.target
index 1a7a4a2f59..5e13a41c3f 100644
--- a/tests/tcg/s390x/Makefile.target
+++ b/tests/tcg/s390x/Makefile.target
@@ -16,6 +16,7 @@ TESTS+=shift
 TESTS+=trap
 TESTS+=signals-s390x
 TESTS+=branch-relative-long
+TESTS+=noexec
 
 Z14_TESTS=vfminmax
 vfminmax: LDFLAGS+=-lm
diff --git a/tests/tcg/s390x/noexec.c b/tests/tcg/s390x/noexec.c
new file mode 100644
index 0000000000..2dfc9ee817
--- /dev/null
+++ b/tests/tcg/s390x/noexec.c
@@ -0,0 +1,145 @@
+#define _GNU_SOURCE
+
+struct arch_noexec_test {
+    void *expected_pswa;
+    unsigned long expected_r2;
+};
+
+#include "../multiarch/noexec.h"
+
+static void arch_check_mcontext(const struct arch_noexec_test *test,
+                                const mcontext_t *ctx) {
+    if (ctx->psw.addr != (unsigned long)test->expected_pswa) {
+        safe_puts("[  FAILED  ] wrong psw.addr");
+        _exit(1);
+    }
+
+    if (ctx->gregs[2] != test->expected_r2) {
+        safe_puts("[  FAILED  ] wrong r2");
+        _exit(1);
+    }
+}
+
+#define DEFINE_NX(name, offset) \
+    void name ## _1(int); \
+    void name ## _2(int); \
+    void name ## _exrl(int); \
+    extern const short name ## _end[]; \
+    asm(/* Go to the specified page offset. */ \
+        ".align 0x1000\n" \
+        ".org .+" #offset "\n" \
+        /* %r2 is 0 on entry, overwrite it with 1. */ \
+        ".globl " #name "_1\n" \
+        #name "_1:\n" \
+        ".cfi_startproc\n" \
+        "lgfi %r2,1\n" \
+        /* Overwrite %2 with 2. */ \
+        ".globl " #name "_2\n" \
+        #name "_2:\n" \
+        "lgfi %r2,2\n" \
+        "br %r14\n" \
+        /* End of code. */ \
+        ".globl " #name "_end\n" \
+        #name "_end:\n" \
+        ".cfi_endproc\n" \
+        /* Go to the next page. */ \
+        ".align 0x1000\n" \
+        /* Break alignment. */ \
+        "nopr %r7\n" \
+        ".globl " #name "_exrl\n" \
+        #name "_exrl:\n" \
+        ".cfi_startproc\n" \
+        "exrl %r0," #name "_2\n" \
+        "br %r14\n" \
+        ".cfi_endproc");
+
+/* noexec_1 is executable, noexec_2 is non-executable. */
+DEFINE_NX(noexec, 0xffa);
+
+/*
+ * noexec_cross_1 is executable, noexec_cross_2 crosses non-executable page
+ * boundary.
+ */
+DEFINE_NX(noexec_cross, 0xff8);
+
+/* noexec_full_1 and noexec_full_2 are non-executable. */
+DEFINE_NX(noexec_full, 0x322);
+
+int main(void)
+{
+    struct noexec_test noexec_tests[] = {
+        {
+            .name = "Fallthrough",
+            .func = noexec_1,
+            .page = noexec_2,
+            .expected_si_addr = noexec_2,
+            .arch = {
+                .expected_pswa = noexec_2,
+                .expected_r2 = 1,
+            },
+        },
+        {
+            .name = "Jump",
+            .func = noexec_2,
+            .page = noexec_2,
+            .expected_si_addr = noexec_2,
+            .arch = {
+                .expected_pswa = noexec_2,
+                .expected_r2 = 0,
+            },
+        },
+        {
+            .name = "EXRL",
+            .func = noexec_exrl,
+            .page = noexec_2,
+            .expected_si_addr = PAGE_ALIGN(noexec_end),
+            .arch = {
+                .expected_pswa = noexec_exrl,
+                .expected_r2 = 0,
+            },
+        },
+        {
+            .name = "Fallthrough [cross]",
+            .func = noexec_cross_1,
+            .page = PAGE_ALIGN(noexec_cross_end),
+            .expected_si_addr = PAGE_ALIGN(noexec_cross_end),
+            .arch = {
+                .expected_pswa = noexec_cross_2,
+                .expected_r2 = 1,
+            },
+        },
+        {
+            .name = "Jump [cross]",
+            .func = noexec_cross_2,
+            .page = PAGE_ALIGN(noexec_cross_end),
+            .expected_si_addr = PAGE_ALIGN(noexec_cross_end),
+            .arch = {
+                .expected_pswa = noexec_cross_2,
+                .expected_r2 = 0,
+            },
+        },
+        {
+            .name = "EXRL [cross]",
+            .func = noexec_cross_exrl,
+            .page = PAGE_ALIGN(noexec_cross_end),
+            .expected_si_addr = PAGE_ALIGN(noexec_cross_end),
+            .arch = {
+                .expected_pswa = noexec_cross_exrl,
+                .expected_r2 = 0,
+            },
+        },
+        {
+            .name = "Jump [full]",
+            .func = noexec_full_1,
+            .page = PAGE_ALIGN(noexec_full_1),
+            .expected_si_addr = PAGE_ALIGN(noexec_full_1),
+            .arch = {
+                .expected_pswa = noexec_full_1,
+                .expected_r2 = 0,
+            },
+        },
+    };
+
+    return test_noexec(noexec_tests,
+                       sizeof(noexec_tests) / sizeof(noexec_tests[0]));
+}
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index b71a6bcd5e..c0e7e5b005 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -10,6 +10,7 @@ include $(SRC_PATH)/tests/tcg/i386/Makefile.target
 
 ifeq ($(filter %-linux-user, $(TARGET)),$(TARGET))
 X86_64_TESTS += vsyscall
+X86_64_TESTS += noexec
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)
@@ -20,5 +21,5 @@ test-x86_64: LDFLAGS+=-lm -lc
 test-x86_64: test-i386.c test-i386.h test-i386-shift.h test-i386-muldiv.h
 	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
 
-vsyscall: $(SRC_PATH)/tests/tcg/x86_64/vsyscall.c
+%: $(SRC_PATH)/tests/tcg/x86_64/%.c
 	$(CC) $(CFLAGS) $< -o $@ $(LDFLAGS)
diff --git a/tests/tcg/x86_64/noexec.c b/tests/tcg/x86_64/noexec.c
new file mode 100644
index 0000000000..ec07c9f0ba
--- /dev/null
+++ b/tests/tcg/x86_64/noexec.c
@@ -0,0 +1,116 @@
+#define _GNU_SOURCE
+
+struct arch_noexec_test {
+    void *expected_rip;
+    unsigned long expected_rdi;
+};
+
+#include "../multiarch/noexec.h"
+
+static void arch_check_mcontext(const struct arch_noexec_test *test,
+                                const mcontext_t *ctx) {
+    if (ctx->gregs[REG_RIP] != (unsigned long)test->expected_rip) {
+        safe_puts("[  FAILED  ] wrong rip");
+        _exit(1);
+    }
+
+    if (ctx->gregs[REG_RDI] != test->expected_rdi) {
+        safe_puts("[  FAILED  ] wrong rdi");
+        _exit(1);
+    }
+}
+
+#define DEFINE_NX(name, offset) \
+    void name ## _1(int); \
+    void name ## _2(int); \
+    extern const short name ## _end[]; \
+    asm(/* Go to the specified page offset. */ \
+        ".align 0x1000\n" \
+        ".org .+" #offset "\n" \
+        /* %rdi is 0 on entry, overwrite it with 1. */ \
+        ".globl " #name "_1\n" \
+        #name "_1:\n" \
+        ".cfi_startproc\n" \
+        "movq $1,%rdi\n" \
+        /* Overwrite %rdi with 2. */ \
+        ".globl " #name "_2\n" \
+        #name "_2:\n" \
+        "movq $2,%rdi\n" \
+        "ret\n" \
+        /* End of code. */ \
+        ".globl " #name "_end\n" \
+        #name "_end:\n" \
+        ".cfi_endproc\n" \
+        /* Go to the next page. */ \
+        ".align 0x1000");
+
+/* noexec_1 is executable, noexec_2 is non-executable. */
+DEFINE_NX(noexec, 0xff9);
+
+/*
+ * noexec_cross_1 is executable, noexec_cross_2 crosses non-executable page
+ * boundary.
+ */
+DEFINE_NX(noexec_cross, 0xff8);
+
+/* noexec_full_1 and noexec_full_2 are non-executable. */
+DEFINE_NX(noexec_full, 0x321);
+
+int main(void)
+{
+    struct noexec_test noexec_tests[] = {
+        {
+            .name = "Fallthrough",
+            .func = noexec_1,
+            .page = noexec_2,
+            .expected_si_addr = noexec_2,
+            .arch = {
+                .expected_rip = noexec_2,
+                .expected_rdi = 1,
+            },
+        },
+        {
+            .name = "Jump",
+            .func = noexec_2,
+            .page = noexec_2,
+            .expected_si_addr = noexec_2,
+            .arch = {
+                .expected_rip = noexec_2,
+                .expected_rdi = 0,
+            },
+        },
+        {
+            .name = "Fallthrough [cross]",
+            .func = noexec_cross_1,
+            .page = PAGE_ALIGN(noexec_cross_end),
+            .expected_si_addr = PAGE_ALIGN(noexec_cross_end),
+            .arch = {
+                .expected_rip = noexec_cross_2,
+                .expected_rdi = 1,
+            },
+        },
+        {
+            .name = "Jump [cross]",
+            .func = noexec_cross_2,
+            .page = PAGE_ALIGN(noexec_cross_end),
+            .expected_si_addr = PAGE_ALIGN(noexec_cross_end),
+            .arch = {
+                .expected_rip = noexec_cross_2,
+                .expected_rdi = 0,
+            },
+        },
+        {
+            .name = "Jump [full]",
+            .func = noexec_full_1,
+            .page = PAGE_ALIGN(noexec_full_1),
+            .expected_si_addr = noexec_full_1,
+            .arch = {
+                .expected_rip = noexec_full_1,
+                .expected_rdi = 0,
+            },
+        },
+    };
+
+    return test_noexec(noexec_tests,
+                       sizeof(noexec_tests) / sizeof(noexec_tests[0]));
+}
-- 
2.37.1

Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Richard Henderson]

On 8/17/22 10:05, Ilya Leoshkevich wrote:
> Hi,
> 
> I noticed that when we get a SEGV due to jumping to non-readable
> memory, sometimes si_addr and program counter in siginfo_t are slightly
> off. I tracked this down to the assumption that translators stop before
> the end of a page, while in reality they may stop right after it.
> 
> Patch 1 fixes an invalidation issue, which may prevent SEGV from
> happening altogether.
> Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> architectures have fixed-size instructions and are not affected.
> Patch 4 adds tests.
> 
> Note: this series depends on [1].

Hah.  I was just thinking that I should queue your patch set to tcg-next-7.2, and then 
rebase my stuff off of that.  It would ensure that I have your test cases in tree so that 
I don't keep regressing them on you.  :-)

I'll cherry pick the one patch you're depending on.


r~

Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Ilya Leoshkevich]

On Wed, 2022-08-17 at 11:23 -0500, Richard Henderson wrote:
> On 8/17/22 10:05, Ilya Leoshkevich wrote:
> > Hi,
> > 
> > I noticed that when we get a SEGV due to jumping to non-readable
> > memory, sometimes si_addr and program counter in siginfo_t are
> > slightly
> > off. I tracked this down to the assumption that translators stop
> > before
> > the end of a page, while in reality they may stop right after it.
> > 
> > Patch 1 fixes an invalidation issue, which may prevent SEGV from
> > happening altogether.
> > Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> > architectures have fixed-size instructions and are not affected.
> > Patch 4 adds tests.
> > 
> > Note: this series depends on [1].
> 
> Hah.  I was just thinking that I should queue your patch set to tcg-
> next-7.2, and then 
> rebase my stuff off of that.  It would ensure that I have your test
> cases in tree so that 
> I don't keep regressing them on you.  :-)
> 
> I'll cherry pick the one patch you're depending on.
> 
> 
> r~

I just checked and cherry-picking [1] and [2] before this series
should be enough.

[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02462.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02461.html

Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Vivian Wang]

[-- Attachment #1: Type: text/plain, Size: 3172 bytes --]

On 8/17/22 23:05, Ilya Leoshkevich wrote:
> Hi,
>
> I noticed that when we get a SEGV due to jumping to non-readable
> memory, sometimes si_addr and program counter in siginfo_t are slightly
> off. I tracked this down to the assumption that translators stop before
> the end of a page, while in reality they may stop right after it.

Hi,

Could this be related to issue 1155 [1]? On RISC-V, I'm getting
incorrect [m|s]tval/[m|s]epc combinations for page faults in system
emulation and incorrect si_addr and program counter on SIGSEGV in user
emulation. Since it seems to only affect instructions that cross page
boundaries, and RISC-V also has variable length instructions, it seems
that I've run into the same problem as what is fixed here.

Could this fix be extended be extended to targets/riscv?

dram

[1]: https://gitlab.com/qemu-project/qemu/-/issues/1155

> Patch 1 fixes an invalidation issue, which may prevent SEGV from
> happening altogether.
> Patches 2-3 fix the main issue on x86_64 and s390x. Many other
> architectures have fixed-size instructions and are not affected.
> Patch 4 adds tests.
>
> Note: this series depends on [1].
>
> Best regards,
> Ilya
>
> v1: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg00822.html
> v1 -> v2: Fix individual translators instead of translator_loop
>           (Peter).
>
> v2: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01079.html
> v2 -> v3: Peek at the next instruction on s390x (Richard).
>           Undo more on i386 (Richard).
>           Check PAGE_EXEC, not PAGE_READ (Peter, Richard).
>
> v3: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01306.html
> v3 -> v4: Improve the commit message in patch 1 to better reflect what
>           exactly is being fixed there.
>           Factor out the is_same_page() patch (Richard).
>           Do not touch the common code in the i386 fix (Richard).
>
> v4: https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg01747.html
> v4 -> v5: Drop patch 2.
>           Use a different fix for the invalidation issue based on
>           discussion with Richard [2].
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02472.html
> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02556.html
>
> Ilya Leoshkevich (4):
>   linux-user: Clear tb_jmp_cache on mprotect()
>   target/s390x: Make translator stop before the end of a page
>   target/i386: Make translator stop before the end of a page
>   tests/tcg: Test siginfo_t contents when jumping to non-readable pages
>
>  linux-user/mmap.c                |  14 +++
>  target/i386/tcg/translate.c      |  25 +++++-
>  target/s390x/tcg/translate.c     |  15 +++-
>  tests/tcg/multiarch/noexec.h     | 114 ++++++++++++++++++++++++
>  tests/tcg/s390x/Makefile.target  |   1 +
>  tests/tcg/s390x/noexec.c         | 145 +++++++++++++++++++++++++++++++
>  tests/tcg/x86_64/Makefile.target |   3 +-
>  tests/tcg/x86_64/noexec.c        | 116 +++++++++++++++++++++++++
>  8 files changed, 427 insertions(+), 6 deletions(-)
>  create mode 100644 tests/tcg/multiarch/noexec.h
>  create mode 100644 tests/tcg/s390x/noexec.c
>  create mode 100644 tests/tcg/x86_64/noexec.c
>

[-- Attachment #2: Type: text/html, Size: 4420 bytes --]

Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Ilya Leoshkevich]

On Fri, 2022-08-19 at 00:55 +0800, Vivian Wang wrote:
> Hi,
> Could this be related to issue 1155 [1]? On RISC-V, I'm getting
> incorrect [m|s]tval/[m|s]epc combinations for page faults in system
> emulation and incorrect si_addr and program counter on SIGSEGV in
> user emulation. Since it seems to only affect instructions that cross
> page boundaries, and RISC-V also has variable length instructions, it
> seems that I've run into the same problem as what is fixed here.
> Could this fix be extended be extended to targets/riscv?
> dram
> [1]: https://gitlab.com/qemu-project/qemu/-/issues/1155

Yes, this looks quite similar.
I'm not too familiar with riscv, but I just googled [1].
If the following is correct:

---
However, the instruction set reserves enough opcode space to make it
possible to differentiate between 16-bit, 32-bit, 48-bit, and 64-bit
instructions.  Instructions that start with binary 11 (in the lowest
bit position of the instruction) are 32-bit sized instructions (but one
pattern is reserved: so they cannot start with 11111).  The compact
instructions use 00, 01, and 10 in that same position.  48-bit
instructions use starting sequence 011111, and 64-bit instructions
start with 0111111.
---

then we can fix this the same way s390x is being fixed here.

[1]
https://stackoverflow.com/questions/56874101/how-does-risc-v-variable-length-of-instruction-work-in-detail

Re: [PATCH v5 0/4] linux-user: Fix siginfo_t contents when jumping to non-readable pages

[Richard Henderson]

On 8/18/22 09:55, Vivian Wang wrote:
> On 8/17/22 23:05, Ilya Leoshkevich wrote:
>> Hi,
>>
>> I noticed that when we get a SEGV due to jumping to non-readable
>> memory, sometimes si_addr and program counter in siginfo_t are slightly
>> off. I tracked this down to the assumption that translators stop before
>> the end of a page, while in reality they may stop right after it.
> 
> Hi,
> 
> Could this be related to issue 1155 [1]? On RISC-V, I'm getting incorrect 
> [m|s]tval/[m|s]epc combinations for page faults in system emulation and incorrect si_addr 
> and program counter on SIGSEGV in user emulation. Since it seems to only affect 
> instructions that cross page boundaries, and RISC-V also has variable length instructions, 
> it seems that I've run into the same problem as what is fixed here.

It seems likely, and the code at the end of riscv_tr_translate_insn is wrong.

> Could this fix be extended be extended to targets/riscv?

I'll write up something.


r~