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

[PATCH v2 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 a minor 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.

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).

Ilya Leoshkevich (4):
  accel/tcg: Invalidate translations when clearing PAGE_READ
  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

 accel/tcg/translate-all.c        |  17 ++--
 include/exec/translator.h        |  10 +++
 target/i386/tcg/translate.c      |  42 ++++++++-
 target/s390x/tcg/translate.c     |  35 ++++----
 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 +++++++++++++++++++++++++
 9 files changed, 461 insertions(+), 22 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.35.3

[PATCH v2 1/4] accel/tcg: Invalidate translations when clearing PAGE_READ

[Ilya Leoshkevich]

After mprotect(addr, PROT_NONE), addr can still be executed if there
are cached translations. Drop them.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 accel/tcg/translate-all.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index ef62a199c7..9318ada6b9 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
          len != 0;
          len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
         PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
+        bool write_set, read_cleared;
 
-        /* If the write protection bit is set, then we invalidate
-           the code inside.  */
-        if (!(p->flags & PAGE_WRITE) &&
-            (flags & PAGE_WRITE) &&
-            p->first_tb) {
+        /*
+         * If the write protection bit is set, then we invalidate the code
+         * inside.
+         */
+        write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
+        /*
+         * If PAGE_READ is cleared, we also need to invalidate the code in
+         * order to force a fault when trying to run it.
+         */
+        read_cleared = (p->flags & PAGE_READ) && !(flags & PAGE_READ);
+        if ((write_set || read_cleared) && p->first_tb) {
             tb_invalidate_phys_page(addr, 0);
         }
         if (reset_target_data) {
-- 
2.35.3

[PATCH v2 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>
---
 include/exec/translator.h    | 10 ++++++++++
 target/s390x/tcg/translate.c | 35 ++++++++++++++++++++---------------
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/include/exec/translator.h b/include/exec/translator.h
index 7db6845535..d27f8c33b6 100644
--- a/include/exec/translator.h
+++ b/include/exec/translator.h
@@ -187,4 +187,14 @@ FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)
 
 #undef GEN_TRANSLATOR_LD
 
+/*
+ * Return whether addr is on the same page as where disassembly started.
+ * Translators can use this to enforce the rule that only single-insn
+ * translation blocks are allowed to cross page boundaries.
+ */
+static inline bool is_same_page(DisasContextBase *db, target_ulong addr)
+{
+    return ((addr ^ db->pc_first) & TARGET_PAGE_MASK) == 0;
+}
+
 #endif /* EXEC__TRANSLATOR_H */
diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
index e2ee005671..0cd0c932fb 100644
--- a/target/s390x/tcg/translate.c
+++ b/target/s390x/tcg/translate.c
@@ -6305,14 +6305,13 @@ static void extract_field(DisasFields *o, const DisasField *f, uint64_t insn)
     o->c[f->indexC] = r;
 }
 
-/* Lookup the insn at the current PC, extracting the operands into O and
-   returning the info struct for the insn.  Returns NULL for invalid insn.  */
+/* Lookup the insn at the current PC, filling the info struct.  */
 
-static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
+static DisasJumpType extract_insn(CPUS390XState *env, DisasContext *s,
+                                  const DisasInsn **info)
 {
     uint64_t insn, pc = s->base.pc_next;
     int op, op2, ilen;
-    const DisasInsn *info;
 
     if (unlikely(s->ex_value)) {
         /* Drop the EX data now, so that it's clear on exception paths.  */
@@ -6325,9 +6324,13 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
         ilen = s->ex_value & 0xf;
         op = insn >> 56;
     } else {
+        assert(s->base.num_insns == 1 || is_same_page(&s->base, pc));
         insn = ld_code2(env, s, pc);
         op = (insn >> 8) & 0xff;
         ilen = get_ilen(op);
+        if (s->base.num_insns > 1 && !is_same_page(&s->base, pc + ilen - 1)) {
+            return DISAS_TOO_MANY;
+        }
         switch (ilen) {
         case 2:
             insn = insn << 48;
@@ -6394,19 +6397,19 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
     s->fields.op2 = op2;
 
     /* Lookup the instruction.  */
-    info = lookup_opc(op << 8 | op2);
-    s->insn = info;
+    *info = lookup_opc(op << 8 | op2);
+    s->insn = *info;
 
     /* If we found it, extract the operands.  */
-    if (info != NULL) {
-        DisasFormat fmt = info->fmt;
+    if (*info != NULL) {
+        DisasFormat fmt = (*info)->fmt;
         int i;
 
         for (i = 0; i < NUM_C_FIELD; ++i) {
             extract_field(&s->fields, &format_info[fmt].op[i], insn);
         }
     }
-    return info;
+    return DISAS_NEXT;
 }
 
 static bool is_afp_reg(int reg)
@@ -6423,12 +6426,17 @@ static bool is_fp_pair(int reg)
 static DisasJumpType translate_one(CPUS390XState *env, DisasContext *s)
 {
     const DisasInsn *insn;
-    DisasJumpType ret = DISAS_NEXT;
+    DisasJumpType ret;
     DisasOps o = {};
     bool icount = false;
 
     /* Search for the insn in the table.  */
-    insn = extract_insn(env, s);
+    ret = extract_insn(env, s, &insn);
+
+    /* This is a subsequent insn that crosses a page boundary.  */
+    if (ret == DISAS_TOO_MANY) {
+        goto out;
+    }
 
     /* Update insn_start now that we know the ILEN.  */
     tcg_set_insn_start_param(s->insn_start, 2, s->ilen);
@@ -6616,10 +6624,7 @@ 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) || dc->ex_value) {
             dc->base.is_jmp = DISAS_TOO_MANY;
         }
     }
-- 
2.35.3

[PATCH v2 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.

We may find out that we crossed page boundary after some ops were
emitted and cc_op was updated. In theory it might be possible to
rearrange the code to disassemble first, but this is too error-prone.
Simply snapshot and restore the disassembly state instead.

Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
 target/i386/tcg/translate.c | 42 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index b7972f0ff5..ea749b0a04 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -2008,6 +2008,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
@@ -4545,6 +4551,29 @@ static void gen_sse(CPUX86State *env, DisasContext *s, int b,
     }
 }
 
+/* Disassembly state that may affect the next instruction. */
+typedef struct {
+    TCGOp *last_op;
+    bool cc_op_dirty;
+    CCOp cc_op;
+} DisasSnapshot;
+
+/* Save disassembly state. */
+static void disas_save(DisasSnapshot *snapshot, const DisasContext *s)
+{
+    snapshot->last_op = tcg_last_op();
+    snapshot->cc_op_dirty = s->cc_op_dirty;
+    snapshot->cc_op = s->cc_op;
+}
+
+/* Restore disassembly state. */
+static void disas_restore(const DisasSnapshot *snapshot, DisasContext *s)
+{
+    tcg_remove_ops_after(snapshot->last_op);
+    s->cc_op_dirty = snapshot->cc_op_dirty;
+    s->cc_op = snapshot->cc_op;
+}
+
 /* convert one instruction. s->base.is_jmp is set if the translation must
    be stopped. Return the next pc value */
 static target_ulong disas_insn(DisasContext *s, CPUState *cpu)
@@ -4556,6 +4585,7 @@ 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;
+    DisasSnapshot snapshot;
 
     s->pc_start = s->pc = pc_start;
     s->override = -1;
@@ -4568,9 +4598,19 @@ 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) {
+    disas_save(&snapshot, s);
+    switch (sigsetjmp(s->jmpbuf, 0)) {
+    case 0:
+        break;
+    case 1:
         gen_exception_gpf(s);
         return s->pc;
+    case 2:
+        disas_restore(&snapshot, s);
+        s->base.is_jmp = DISAS_TOO_MANY;
+        return pc_start;
+    default:
+        g_assert_not_reached();
     }
 
     prefixes = 0;
-- 
2.35.3

[PATCH v2 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.35.3

Re: [PATCH v2 1/4] accel/tcg: Invalidate translations when clearing PAGE_READ

[Peter Maydell]

On Fri, 5 Aug 2022 at 18:33, Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> After mprotect(addr, PROT_NONE), addr can still be executed if there
> are cached translations. Drop them.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  accel/tcg/translate-all.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ef62a199c7..9318ada6b9 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
>           len != 0;
>           len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
>          PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
> +        bool write_set, read_cleared;
>
> -        /* If the write protection bit is set, then we invalidate
> -           the code inside.  */
> -        if (!(p->flags & PAGE_WRITE) &&
> -            (flags & PAGE_WRITE) &&
> -            p->first_tb) {
> +        /*
> +         * If the write protection bit is set, then we invalidate the code
> +         * inside.
> +         */
> +        write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
> +        /*
> +         * If PAGE_READ is cleared, we also need to invalidate the code in
> +         * order to force a fault when trying to run it.
> +         */
> +        read_cleared = (p->flags & PAGE_READ) && !(flags & PAGE_READ);

Isn't it architecture-dependent whether you need PAGE_READ
to execute code ? How about PAGE_EXEC ?

thanks
-- PMM

Re: [PATCH v2 1/4] accel/tcg: Invalidate translations when clearing PAGE_READ

[Richard Henderson]

On 8/5/22 09:09, Ilya Leoshkevich wrote:
> After mprotect(addr, PROT_NONE), addr can still be executed if there
> are cached translations. Drop them.
> 
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>   accel/tcg/translate-all.c | 17 ++++++++++++-----
>   1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
> index ef62a199c7..9318ada6b9 100644
> --- a/accel/tcg/translate-all.c
> +++ b/accel/tcg/translate-all.c
> @@ -2295,12 +2295,19 @@ void page_set_flags(target_ulong start, target_ulong end, int flags)
>            len != 0;
>            len -= TARGET_PAGE_SIZE, addr += TARGET_PAGE_SIZE) {
>           PageDesc *p = page_find_alloc(addr >> TARGET_PAGE_BITS, 1);
> +        bool write_set, read_cleared;
>   
> -        /* If the write protection bit is set, then we invalidate
> -           the code inside.  */
> -        if (!(p->flags & PAGE_WRITE) &&
> -            (flags & PAGE_WRITE) &&
> -            p->first_tb) {
> +        /*
> +         * If the write protection bit is set, then we invalidate the code
> +         * inside.
> +         */
> +        write_set = !(p->flags & PAGE_WRITE) && (flags & PAGE_WRITE);
> +        /*
> +         * If PAGE_READ is cleared, we also need to invalidate the code in
> +         * order to force a fault when trying to run it.
> +         */
> +        read_cleared = (p->flags & PAGE_READ) && !(flags & PAGE_READ);

PAGE_READ has nothing to do with it -- PAGE_EXEC does though.


r~

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

[Richard Henderson]

On 8/5/22 09:09, Ilya Leoshkevich wrote:
> 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>
> ---
>   include/exec/translator.h    | 10 ++++++++++
>   target/s390x/tcg/translate.c | 35 ++++++++++++++++++++---------------
>   2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/include/exec/translator.h b/include/exec/translator.h
> index 7db6845535..d27f8c33b6 100644
> --- a/include/exec/translator.h
> +++ b/include/exec/translator.h
> @@ -187,4 +187,14 @@ FOR_EACH_TRANSLATOR_LD(GEN_TRANSLATOR_LD)
>   
>   #undef GEN_TRANSLATOR_LD
>   
> +/*
> + * Return whether addr is on the same page as where disassembly started.
> + * Translators can use this to enforce the rule that only single-insn
> + * translation blocks are allowed to cross page boundaries.
> + */
> +static inline bool is_same_page(DisasContextBase *db, target_ulong addr)
> +{
> +    return ((addr ^ db->pc_first) & TARGET_PAGE_MASK) == 0;
> +}
> +
>   #endif /* EXEC__TRANSLATOR_H */
> diff --git a/target/s390x/tcg/translate.c b/target/s390x/tcg/translate.c
> index e2ee005671..0cd0c932fb 100644
> --- a/target/s390x/tcg/translate.c
> +++ b/target/s390x/tcg/translate.c
> @@ -6305,14 +6305,13 @@ static void extract_field(DisasFields *o, const DisasField *f, uint64_t insn)
>       o->c[f->indexC] = r;
>   }
>   
> -/* Lookup the insn at the current PC, extracting the operands into O and
> -   returning the info struct for the insn.  Returns NULL for invalid insn.  */
> +/* Lookup the insn at the current PC, filling the info struct.  */
>   
> -static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
> +static DisasJumpType extract_insn(CPUS390XState *env, DisasContext *s,
> +                                  const DisasInsn **info)
>   {
>       uint64_t insn, pc = s->base.pc_next;
>       int op, op2, ilen;
> -    const DisasInsn *info;
>   
>       if (unlikely(s->ex_value)) {
>           /* Drop the EX data now, so that it's clear on exception paths.  */
> @@ -6325,9 +6324,13 @@ static const DisasInsn *extract_insn(CPUS390XState *env, DisasContext *s)
>           ilen = s->ex_value & 0xf;
>           op = insn >> 56;
>       } else {
> +        assert(s->base.num_insns == 1 || is_same_page(&s->base, pc));
>           insn = ld_code2(env, s, pc);
>           op = (insn >> 8) & 0xff;
>           ilen = get_ilen(op);
> +        if (s->base.num_insns > 1 && !is_same_page(&s->base, pc + ilen - 1)) {
> +            return DISAS_TOO_MANY;
> +        }

This doesn't work.

You need to end the TB at the end of the previous insn, not at the beginning of the 
current insn.  Here, we have already committed to executing one instruction.

You need to model this similar to arm's insn_crosses_page, where we check at the end of 
one instruction whether we'll be able to run another.


r~

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

[Richard Henderson]

On 8/5/22 09:09, Ilya Leoshkevich wrote:
> @@ -4568,9 +4598,19 @@ 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) {
> +    disas_save(&snapshot, s);
> +    switch (sigsetjmp(s->jmpbuf, 0)) {
> +    case 0:
> +        break;
> +    case 1:
>           gen_exception_gpf(s);
>           return s->pc;
> +    case 2:
> +        disas_restore(&snapshot, s);
> +        s->base.is_jmp = DISAS_TOO_MANY;
> +        return pc_start;
> +    default:

Similarly, this is too late for DISAS_TOO_MANY.

It will be more difficult to fix this for x86, since unlike s390x (or arm for that 
matter), we cannot probe for the total insn length within the first few bits of the insn.

The simplest possibility would to force single-stepping when we're within 16 bytes of the 
end of the page, as that's a hard maximum on the number of bytes within an x86 insn.

We could probably still use this sort of longjmp thing, but we'd need to unwind more than 
you're doing here.  We'd need to discard the insn_start opcode (which is before the 
last_op that you're currently saving), and decrement s->base.num_insns.


r~