QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls
@ 2020-01-16 19:43 Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 1/5] target/i386: Renumber EXCP_SYSCALL Richard Henderson
                   ` (6 more replies)
  0 siblings, 7 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, laurent

Changes since v2:

  * Add /proc/self/maps line

	I'm not sure this is really necessary.  The linux kernel
	self-test checks for it, and modifies the set of tests that
	it runs based on it.  But otherwise I think it's unused.

  * Fix errors in base gettimeofday syscall

	This is also checked by test_vsyscall, as noticed by AJB.


r~


Original blurb:

The x86_64 abi has a legacy vsyscall page.  The kernel folk
have been trying to deprecate this since at least v3.1, but

(1) We don't implement the vdso that replaces vsyscalls,
(2) As of v5.5, the vsyscall page is still enabled by default.

This lack is affecting Peter's linux-user testing.

The dependency is not obvious because Peter is running the tests
on x86_64, so the host is providing a vsyscall page to qemu.

Because of how user-only memory operations are handled, with no
validation of guest vs host pages, so long as qemu chooses to
run with guest_base == 0, the guest may Just So Happen to read
the host's vsyscall page.

Complicating this, new OS releases may use a kernel configured
with CONFIG_LEGACY_VSYSCALL_XONLY=y, which means the the vsyscall
page cannot be read, only executed.  Which means that the guest
then cannot read the host vsyscall page during translation and
will SIGSEGV.

Exactly which of these many variables is affecting Peter's testing
with Ubuntu 18.04 of my TCG merge, I'm not exactly sure.  I suspect
that it is the change to drop the textseg_addr adjustment to user-only
static binaries.  IIRC bionic does not support -static-pie, which is
the preferred replacement.  This could mean that the host and guest
binaries overlap, which leads to guest_base != 0.

I vaguely remember someone (Paolo?) implementing something like
this many years ago, but clearly it never got merged.

In any case, this emulation has been missing for too long.


Richard Henderson (5):
  target/i386: Renumber EXCP_SYSCALL
  linux-user/i386: Split out gen_signal
  linux-user/i386: Emulate x86_64 vsyscalls
  linux-user: Add x86_64 vsyscall page to /proc/self/maps
  linux-user: Flush out implementation of gettimeofday

 target/i386/cpu.h          |   6 +-
 linux-user/i386/cpu_loop.c | 198 ++++++++++++++++++++++++++-----------
 linux-user/syscall.c       |  36 ++++++-
 target/i386/translate.c    |  16 ++-
 4 files changed, 190 insertions(+), 66 deletions(-)

-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/5] target/i386: Renumber EXCP_SYSCALL
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
@ 2020-01-16 19:43 ` Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 2/5] linux-user/i386: Split out gen_signal Richard Henderson
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, alex.bennee, laurent, Philippe Mathieu-Daudé

We are not short of numbers for EXCP_*.  There is no need to confuse things
by having EXCP_VMEXIT and EXCP_SYSCALL overlap, even though the former is
only used for system mode and the latter is only used for user mode.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/cpu.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 594326a794..164d038d1f 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -998,9 +998,8 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
 #define EXCP11_ALGN	17
 #define EXCP12_MCHK	18
 
-#define EXCP_SYSCALL    0x100 /* only happens in user only emulation
-                                 for syscall instruction */
-#define EXCP_VMEXIT     0x100
+#define EXCP_VMEXIT     0x100 /* only for system emulation */
+#define EXCP_SYSCALL    0x101 /* only for user emulation */
 
 /* i386-specific interrupt pending bits.  */
 #define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 2/5] linux-user/i386: Split out gen_signal
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 1/5] target/i386: Renumber EXCP_SYSCALL Richard Henderson
@ 2020-01-16 19:43 ` Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls Richard Henderson
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, alex.bennee, laurent, Philippe Mathieu-Daudé

This is a bit tidier than open-coding the 5 lines necessary
to initialize the target_siginfo_t.  In addition, this zeros
the remaining bytes of the target_siginfo_t, rather than
passing in garbage.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/i386/cpu_loop.c | 93 ++++++++++++++------------------------
 1 file changed, 33 insertions(+), 60 deletions(-)

diff --git a/linux-user/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c
index 024b6f4d58..e217cca5ee 100644
--- a/linux-user/i386/cpu_loop.c
+++ b/linux-user/i386/cpu_loop.c
@@ -81,13 +81,23 @@ static void set_idt(int n, unsigned int dpl)
 }
 #endif
 
+static void gen_signal(CPUX86State *env, int sig, int code, abi_ptr addr)
+{
+    target_siginfo_t info = {
+        .si_signo = sig,
+        .si_code = code,
+        ._sifields._sigfault._addr = addr
+    };
+
+    queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+}
+
 void cpu_loop(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
     int trapnr;
     abi_ulong pc;
     abi_ulong ret;
-    target_siginfo_t info;
 
     for(;;) {
         cpu_exec_start(cs);
@@ -134,70 +144,45 @@ void cpu_loop(CPUX86State *env)
 #endif
         case EXCP0B_NOSEG:
         case EXCP0C_STACK:
-            info.si_signo = TARGET_SIGBUS;
-            info.si_errno = 0;
-            info.si_code = TARGET_SI_KERNEL;
-            info._sifields._sigfault._addr = 0;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            gen_signal(env, TARGET_SIGBUS, TARGET_SI_KERNEL, 0);
             break;
         case EXCP0D_GPF:
             /* XXX: potential problem if ABI32 */
 #ifndef TARGET_X86_64
             if (env->eflags & VM_MASK) {
                 handle_vm86_fault(env);
-            } else
-#endif
-            {
-                info.si_signo = TARGET_SIGSEGV;
-                info.si_errno = 0;
-                info.si_code = TARGET_SI_KERNEL;
-                info._sifields._sigfault._addr = 0;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+                break;
             }
+#endif
+            gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
             break;
         case EXCP0E_PAGE:
-            info.si_signo = TARGET_SIGSEGV;
-            info.si_errno = 0;
-            if (!(env->error_code & 1))
-                info.si_code = TARGET_SEGV_MAPERR;
-            else
-                info.si_code = TARGET_SEGV_ACCERR;
-            info._sifields._sigfault._addr = env->cr[2];
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            gen_signal(env, TARGET_SIGSEGV,
+                       (env->error_code & 1 ?
+                        TARGET_SEGV_ACCERR : TARGET_SEGV_MAPERR),
+                       env->cr[2]);
             break;
         case EXCP00_DIVZ:
 #ifndef TARGET_X86_64
             if (env->eflags & VM_MASK) {
                 handle_vm86_trap(env, trapnr);
-            } else
-#endif
-            {
-                /* division by zero */
-                info.si_signo = TARGET_SIGFPE;
-                info.si_errno = 0;
-                info.si_code = TARGET_FPE_INTDIV;
-                info._sifields._sigfault._addr = env->eip;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+                break;
             }
+#endif
+            gen_signal(env, TARGET_SIGFPE, TARGET_FPE_INTDIV, env->eip);
             break;
         case EXCP01_DB:
         case EXCP03_INT3:
 #ifndef TARGET_X86_64
             if (env->eflags & VM_MASK) {
                 handle_vm86_trap(env, trapnr);
-            } else
+                break;
+            }
 #endif
-            {
-                info.si_signo = TARGET_SIGTRAP;
-                info.si_errno = 0;
-                if (trapnr == EXCP01_DB) {
-                    info.si_code = TARGET_TRAP_BRKPT;
-                    info._sifields._sigfault._addr = env->eip;
-                } else {
-                    info.si_code = TARGET_SI_KERNEL;
-                    info._sifields._sigfault._addr = 0;
-                }
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            if (trapnr == EXCP01_DB) {
+                gen_signal(env, TARGET_SIGTRAP, TARGET_TRAP_BRKPT, env->eip);
+            } else {
+                gen_signal(env, TARGET_SIGTRAP, TARGET_SI_KERNEL, 0);
             }
             break;
         case EXCP04_INTO:
@@ -205,31 +190,19 @@ void cpu_loop(CPUX86State *env)
 #ifndef TARGET_X86_64
             if (env->eflags & VM_MASK) {
                 handle_vm86_trap(env, trapnr);
-            } else
-#endif
-            {
-                info.si_signo = TARGET_SIGSEGV;
-                info.si_errno = 0;
-                info.si_code = TARGET_SI_KERNEL;
-                info._sifields._sigfault._addr = 0;
-                queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+                break;
             }
+#endif
+            gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
             break;
         case EXCP06_ILLOP:
-            info.si_signo = TARGET_SIGILL;
-            info.si_errno = 0;
-            info.si_code = TARGET_ILL_ILLOPN;
-            info._sifields._sigfault._addr = env->eip;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            gen_signal(env, TARGET_SIGILL, TARGET_ILL_ILLOPN, env->eip);
             break;
         case EXCP_INTERRUPT:
             /* just indicate that signals should be handled asap */
             break;
         case EXCP_DEBUG:
-            info.si_signo = TARGET_SIGTRAP;
-            info.si_errno = 0;
-            info.si_code = TARGET_TRAP_BRKPT;
-            queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
+            gen_signal(env, TARGET_SIGTRAP, TARGET_TRAP_BRKPT, 0);
             break;
         case EXCP_ATOMIC:
             cpu_exec_step_atomic(cs);
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 1/5] target/i386: Renumber EXCP_SYSCALL Richard Henderson
  2020-01-16 19:43 ` [PATCH v2 2/5] linux-user/i386: Split out gen_signal Richard Henderson
@ 2020-01-16 19:43 ` Richard Henderson
  2020-01-20 11:48   ` Alex Bennée
  2020-01-16 19:43 ` [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps Richard Henderson
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, alex.bennee, laurent

Notice the magic page during translate, much like we already
do for the arm32 commpage.  At runtime, raise an exception to
return cpu_loop for emulation.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/cpu.h          |   1 +
 linux-user/i386/cpu_loop.c | 105 +++++++++++++++++++++++++++++++++++++
 target/i386/translate.c    |  16 +++++-
 3 files changed, 121 insertions(+), 1 deletion(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 164d038d1f..3fb2d2a986 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1000,6 +1000,7 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
 
 #define EXCP_VMEXIT     0x100 /* only for system emulation */
 #define EXCP_SYSCALL    0x101 /* only for user emulation */
+#define EXCP_VSYSCALL   0x102 /* only for user emulation */
 
 /* i386-specific interrupt pending bits.  */
 #define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
diff --git a/linux-user/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c
index e217cca5ee..f9bf6cec27 100644
--- a/linux-user/i386/cpu_loop.c
+++ b/linux-user/i386/cpu_loop.c
@@ -92,6 +92,106 @@ static void gen_signal(CPUX86State *env, int sig, int code, abi_ptr addr)
     queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
 }
 
+#ifdef TARGET_X86_64
+static bool write_ok_or_segv(CPUX86State *env, abi_ptr addr, size_t len)
+{
+    /*
+     * For all the vsyscalls, NULL means "don't write anything" not
+     * "write it at address 0".
+     */
+    if (addr == 0 || access_ok(VERIFY_WRITE, addr, len)) {
+        return true;
+    }
+
+    env->error_code = PG_ERROR_W_MASK | PG_ERROR_U_MASK;
+    gen_signal(env, TARGET_SIGSEGV, TARGET_SEGV_MAPERR, addr);
+    return false;
+}
+
+/*
+ * Since v3.1, the kernel traps and emulates the vsyscall page.
+ * Entry points other than the official generate SIGSEGV.
+ */
+static void emulate_vsyscall(CPUX86State *env)
+{
+    int syscall;
+    abi_ulong ret;
+    uint64_t caller;
+
+    /*
+     * Validate the entry point.  We have already validated the page
+     * during translation, now verify the offset.
+     */
+    switch (env->eip & ~TARGET_PAGE_MASK) {
+    case 0x000:
+        syscall = TARGET_NR_gettimeofday;
+        break;
+    case 0x400:
+        syscall = TARGET_NR_time;
+        break;
+    case 0x800:
+        syscall = TARGET_NR_getcpu;
+        break;
+    default:
+    sigsegv:
+        /* Like force_sig(SIGSEGV).  */
+        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
+        return;
+    }
+
+    /*
+     * Validate the return address.
+     * Note that the kernel treats this the same as an invalid entry point.
+     */
+    if (get_user_u64(caller, env->regs[R_ESP])) {
+        goto sigsegv;
+    }
+
+    /*
+     * Validate the the pointer arguments.
+     */
+    switch (syscall) {
+    case TARGET_NR_gettimeofday:
+        if (!write_ok_or_segv(env, env->regs[R_EDI],
+                              sizeof(struct target_timeval)) ||
+            !write_ok_or_segv(env, env->regs[R_ESI],
+                              sizeof(struct target_timezone))) {
+            return;
+        }
+        break;
+    case TARGET_NR_time:
+        if (!write_ok_or_segv(env, env->regs[R_EDI], sizeof(abi_long))) {
+            return;
+        }
+        break;
+    case TARGET_NR_getcpu:
+        if (!write_ok_or_segv(env, env->regs[R_EDI], sizeof(uint32_t)) ||
+            !write_ok_or_segv(env, env->regs[R_ESI], sizeof(uint32_t))) {
+            return;
+        }
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    /*
+     * Perform the syscall.  None of the vsyscalls should need restarting,
+     * and all faults should have been caught above.
+     */
+    ret = do_syscall(env, syscall, env->regs[R_EDI], env->regs[R_ESI],
+                     env->regs[R_EDX], env->regs[10], env->regs[8],
+                     env->regs[9], 0, 0);
+    g_assert(ret != -TARGET_ERESTARTSYS);
+    g_assert(ret != -TARGET_QEMU_ESIGRETURN);
+    g_assert(ret != -TARGET_EFAULT);
+    env->regs[R_EAX] = ret;
+
+    /* Emulate a ret instruction to leave the vsyscall page.  */
+    env->eip = caller;
+    env->regs[R_ESP] += 8;
+}
+#endif
+
 void cpu_loop(CPUX86State *env)
 {
     CPUState *cs = env_cpu(env);
@@ -141,6 +241,11 @@ void cpu_loop(CPUX86State *env)
                 env->regs[R_EAX] = ret;
             }
             break;
+#endif
+#ifdef TARGET_X86_64
+        case EXCP_VSYSCALL:
+            emulate_vsyscall(env);
+            break;
 #endif
         case EXCP0B_NOSEG:
         case EXCP0C_STACK:
diff --git a/target/i386/translate.c b/target/i386/translate.c
index 7c99ef1385..391b4ef149 100644
--- a/target/i386/translate.c
+++ b/target/i386/translate.c
@@ -8555,7 +8555,21 @@ static bool i386_tr_breakpoint_check(DisasContextBase *dcbase, CPUState *cpu,
 static void i386_tr_translate_insn(DisasContextBase *dcbase, CPUState *cpu)
 {
     DisasContext *dc = container_of(dcbase, DisasContext, base);
-    target_ulong pc_next = disas_insn(dc, cpu);
+    target_ulong pc_next;
+
+#if defined(TARGET_X86_64) && \
+    defined(CONFIG_USER_ONLY) && \
+    defined(CONFIG_LINUX)
+    /*
+     * Detect entry into the vsyscall page and invoke the syscall.
+     */
+    if ((dc->base.pc_next & TARGET_PAGE_MASK) == 0xffffffffff600000ull) {
+        gen_exception(dc, EXCP_VSYSCALL, dc->base.pc_next);
+        return;
+    }
+#endif
+
+    pc_next = disas_insn(dc, cpu);
 
     if (dc->tf || (dc->base.tb->flags & HF_INHIBIT_IRQ_MASK)) {
         /* if single step mode, we generate only one instruction and
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
                   ` (2 preceding siblings ...)
  2020-01-16 19:43 ` [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls Richard Henderson
@ 2020-01-16 19:43 ` Richard Henderson
  2020-01-17  6:39   ` Philippe Mathieu-Daudé
  2020-01-20 14:41   ` Alex Bennée
  2020-01-16 19:43 ` [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday Richard Henderson
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, laurent

The page isn't (necessarily) present in the host /proc/self/maps,
and even if it might be it isn't present in page_flags, and even
if it was it might not have the same set of page permissions.

The easiest thing to do, particularly when it comes to the
"[vsyscall]" note at the end of line, is to special case it.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 171c0caef3..eb867a5296 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7005,6 +7005,15 @@ static int open_self_maps(void *cpu_env, int fd)
         }
     }
 
+#ifdef TARGET_X86_64
+    /*
+     * We only support execution from the vsyscall page.
+     * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
+     */
+    dprintf(fd, "ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0"
+                "          [vsyscall]\n");
+#endif
+
     free(line);
     fclose(fp);
 
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
                   ` (3 preceding siblings ...)
  2020-01-16 19:43 ` [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps Richard Henderson
@ 2020-01-16 19:43 ` Richard Henderson
  2020-01-17  6:37   ` Philippe Mathieu-Daudé
  2020-01-20 13:27   ` Alex Bennée
  2020-01-16 19:46 ` [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
  2020-01-28 17:26 ` Laurent Vivier
  6 siblings, 2 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:43 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, laurent

The first argument, timeval, is allowed to be NULL.

The second argument, timezone, was missing.  While its use is
deprecated, it is still present in the syscall.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/syscall.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index eb867a5296..628b4de9a1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1219,6 +1219,23 @@ static inline abi_long host_to_target_timespec64(abi_ulong target_addr,
     return 0;
 }
 
+static inline abi_long copy_to_user_timezone(abi_ulong target_tz_addr,
+                                             struct timezone *tz)
+{
+    struct target_timezone *target_tz;
+
+    if (!lock_user_struct(VERIFY_WRITE, target_tz, target_tz_addr, 1)) {
+        return -TARGET_EFAULT;
+    }
+
+    __put_user(tz->tz_minuteswest, &target_tz->tz_minuteswest);
+    __put_user(tz->tz_dsttime, &target_tz->tz_dsttime);
+
+    unlock_user_struct(target_tz, target_tz_addr, 1);
+
+    return 0;
+}
+
 static inline abi_long copy_from_user_timezone(struct timezone *tz,
                                                abi_ulong target_tz_addr)
 {
@@ -8567,10 +8584,16 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
     case TARGET_NR_gettimeofday:
         {
             struct timeval tv;
-            ret = get_errno(gettimeofday(&tv, NULL));
+            struct timezone tz;
+
+            ret = get_errno(gettimeofday(&tv, &tz));
             if (!is_error(ret)) {
-                if (copy_to_user_timeval(arg1, &tv))
+                if (arg1 && copy_to_user_timeval(arg1, &tv)) {
                     return -TARGET_EFAULT;
+                }
+                if (arg2 && copy_to_user_timezone(arg2, &tz)) {
+                    return -TARGET_EFAULT;
+                }
             }
         }
         return ret;
-- 
2.20.1



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
                   ` (4 preceding siblings ...)
  2020-01-16 19:43 ` [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday Richard Henderson
@ 2020-01-16 19:46 ` Richard Henderson
  2020-01-28 17:26 ` Laurent Vivier
  6 siblings, 0 replies; 18+ messages in thread
From: Richard Henderson @ 2020-01-16 19:46 UTC (permalink / raw)
  To: qemu-devel; +Cc: alex.bennee, laurent

On 1/16/20 9:43 AM, Richard Henderson wrote:
> Changes since v2:
> 
>   * Add /proc/self/maps line
> 
> 	I'm not sure this is really necessary.  The linux kernel
> 	self-test checks for it, and modifies the set of tests that
> 	it runs based on it.  But otherwise I think it's unused.
> 
>   * Fix errors in base gettimeofday syscall
> 
> 	This is also checked by test_vsyscall, as noticed by AJB.

Oh, and

    * Set error_code in write_ok_or_segv (patch 2)


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday
  2020-01-16 19:43 ` [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday Richard Henderson
@ 2020-01-17  6:37   ` Philippe Mathieu-Daudé
  2020-01-20 13:27   ` Alex Bennée
  1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-17  6:37 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: alex.bennee, laurent

On 1/16/20 8:43 PM, Richard Henderson wrote:
> The first argument, timeval, is allowed to be NULL.
> 
> The second argument, timezone, was missing.  While its use is
> deprecated, it is still present in the syscall.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/syscall.c | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index eb867a5296..628b4de9a1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1219,6 +1219,23 @@ static inline abi_long host_to_target_timespec64(abi_ulong target_addr,
>       return 0;
>   }
>   
> +static inline abi_long copy_to_user_timezone(abi_ulong target_tz_addr,
> +                                             struct timezone *tz)
> +{
> +    struct target_timezone *target_tz;
> +
> +    if (!lock_user_struct(VERIFY_WRITE, target_tz, target_tz_addr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    __put_user(tz->tz_minuteswest, &target_tz->tz_minuteswest);
> +    __put_user(tz->tz_dsttime, &target_tz->tz_dsttime);
> +
> +    unlock_user_struct(target_tz, target_tz_addr, 1);
> +
> +    return 0;
> +}
> +
>   static inline abi_long copy_from_user_timezone(struct timezone *tz,
>                                                  abi_ulong target_tz_addr)
>   {
> @@ -8567,10 +8584,16 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>       case TARGET_NR_gettimeofday:
>           {
>               struct timeval tv;
> -            ret = get_errno(gettimeofday(&tv, NULL));
> +            struct timezone tz;
> +
> +            ret = get_errno(gettimeofday(&tv, &tz));
>               if (!is_error(ret)) {
> -                if (copy_to_user_timeval(arg1, &tv))
> +                if (arg1 && copy_to_user_timeval(arg1, &tv)) {
>                       return -TARGET_EFAULT;
> +                }
> +                if (arg2 && copy_to_user_timezone(arg2, &tz)) {
> +                    return -TARGET_EFAULT;
> +                }
>               }
>           }
>           return ret;
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps
  2020-01-16 19:43 ` [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps Richard Henderson
@ 2020-01-17  6:39   ` Philippe Mathieu-Daudé
  2020-01-20 14:41   ` Alex Bennée
  1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-01-17  6:39 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: alex.bennee, laurent

On 1/16/20 8:43 PM, Richard Henderson wrote:
> The page isn't (necessarily) present in the host /proc/self/maps,
> and even if it might be it isn't present in page_flags, and even
> if it was it might not have the same set of page permissions.
> 
> The easiest thing to do, particularly when it comes to the
> "[vsyscall]" note at the end of line, is to special case it.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/syscall.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 171c0caef3..eb867a5296 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7005,6 +7005,15 @@ static int open_self_maps(void *cpu_env, int fd)
>           }
>       }
>   
> +#ifdef TARGET_X86_64
> +    /*
> +     * We only support execution from the vsyscall page.
> +     * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
> +     */
> +    dprintf(fd, "ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0"
> +                "          [vsyscall]\n");

Can we add a definition for 0xffffffffff600000ull in the previous patch, 
and use it with TARGET_PAGE_MASK here?

> +#endif
> +
>       free(line);
>       fclose(fp);
>   
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-16 19:43 ` [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls Richard Henderson
@ 2020-01-20 11:48   ` Alex Bennée
  2020-01-21  3:38     ` Richard Henderson
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Bennée @ 2020-01-20 11:48 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Paolo Bonzini, qemu-devel, laurent


Richard Henderson <richard.henderson@linaro.org> writes:

> Notice the magic page during translate, much like we already
> do for the arm32 commpage.  At runtime, raise an exception to
> return cpu_loop for emulation.
>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/i386/cpu.h          |   1 +
>  linux-user/i386/cpu_loop.c | 105 +++++++++++++++++++++++++++++++++++++
>  target/i386/translate.c    |  16 +++++-
>  3 files changed, 121 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.h b/target/i386/cpu.h
> index 164d038d1f..3fb2d2a986 100644
> --- a/target/i386/cpu.h
> +++ b/target/i386/cpu.h
> @@ -1000,6 +1000,7 @@ typedef uint64_t FeatureWordArray[FEATURE_WORDS];
>  
>  #define EXCP_VMEXIT     0x100 /* only for system emulation */
>  #define EXCP_SYSCALL    0x101 /* only for user emulation */
> +#define EXCP_VSYSCALL   0x102 /* only for user emulation */
>  
>  /* i386-specific interrupt pending bits.  */
>  #define CPU_INTERRUPT_POLL      CPU_INTERRUPT_TGT_EXT_1
> diff --git a/linux-user/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c
> index e217cca5ee..f9bf6cec27 100644
> --- a/linux-user/i386/cpu_loop.c
> +++ b/linux-user/i386/cpu_loop.c
> @@ -92,6 +92,106 @@ static void gen_signal(CPUX86State *env, int sig, int code, abi_ptr addr)
>      queue_signal(env, info.si_signo, QEMU_SI_FAULT, &info);
>  }
>  
> +#ifdef TARGET_X86_64
> +static bool write_ok_or_segv(CPUX86State *env, abi_ptr addr, size_t len)
> +{
> +    /*
> +     * For all the vsyscalls, NULL means "don't write anything" not
> +     * "write it at address 0".
> +     */
> +    if (addr == 0 || access_ok(VERIFY_WRITE, addr, len)) {
> +        return true;
> +    }
> +
> +    env->error_code = PG_ERROR_W_MASK | PG_ERROR_U_MASK;
> +    gen_signal(env, TARGET_SIGSEGV, TARGET_SEGV_MAPERR, addr);
> +    return false;
> +}
> +
> +/*
> + * Since v3.1, the kernel traps and emulates the vsyscall page.
> + * Entry points other than the official generate SIGSEGV.
> + */
> +static void emulate_vsyscall(CPUX86State *env)
> +{
> +    int syscall;
> +    abi_ulong ret;
> +    uint64_t caller;
> +
> +    /*
> +     * Validate the entry point.  We have already validated the page
> +     * during translation, now verify the offset.
> +     */
> +    switch (env->eip & ~TARGET_PAGE_MASK) {
> +    case 0x000:
> +        syscall = TARGET_NR_gettimeofday;
> +        break;
> +    case 0x400:
> +        syscall = TARGET_NR_time;
> +        break;
> +    case 0x800:
> +        syscall = TARGET_NR_getcpu;
> +        break;
> +    default:
> +    sigsegv:

this label looks a little extraneous.

Otherwise:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday
  2020-01-16 19:43 ` [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday Richard Henderson
  2020-01-17  6:37   ` Philippe Mathieu-Daudé
@ 2020-01-20 13:27   ` Alex Bennée
  1 sibling, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2020-01-20 13:27 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel, laurent


Richard Henderson <richard.henderson@linaro.org> writes:

> The first argument, timeval, is allowed to be NULL.
>
> The second argument, timezone, was missing.  While its use is
> deprecated, it is still present in the syscall.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  linux-user/syscall.c | 27 +++++++++++++++++++++++++--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index eb867a5296..628b4de9a1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1219,6 +1219,23 @@ static inline abi_long host_to_target_timespec64(abi_ulong target_addr,
>      return 0;
>  }
>  
> +static inline abi_long copy_to_user_timezone(abi_ulong target_tz_addr,
> +                                             struct timezone *tz)
> +{
> +    struct target_timezone *target_tz;
> +
> +    if (!lock_user_struct(VERIFY_WRITE, target_tz, target_tz_addr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
> +
> +    __put_user(tz->tz_minuteswest, &target_tz->tz_minuteswest);
> +    __put_user(tz->tz_dsttime, &target_tz->tz_dsttime);
> +
> +    unlock_user_struct(target_tz, target_tz_addr, 1);
> +
> +    return 0;
> +}
> +
>  static inline abi_long copy_from_user_timezone(struct timezone *tz,
>                                                 abi_ulong target_tz_addr)
>  {
> @@ -8567,10 +8584,16 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>      case TARGET_NR_gettimeofday:
>          {
>              struct timeval tv;
> -            ret = get_errno(gettimeofday(&tv, NULL));
> +            struct timezone tz;
> +
> +            ret = get_errno(gettimeofday(&tv, &tz));
>              if (!is_error(ret)) {
> -                if (copy_to_user_timeval(arg1, &tv))
> +                if (arg1 && copy_to_user_timeval(arg1, &tv)) {
>                      return -TARGET_EFAULT;
> +                }
> +                if (arg2 && copy_to_user_timezone(arg2, &tz)) {
> +                    return -TARGET_EFAULT;
> +                }
>              }
>          }
>          return ret;


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps
  2020-01-16 19:43 ` [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps Richard Henderson
  2020-01-17  6:39   ` Philippe Mathieu-Daudé
@ 2020-01-20 14:41   ` Alex Bennée
  1 sibling, 0 replies; 18+ messages in thread
From: Alex Bennée @ 2020-01-20 14:41 UTC (permalink / raw)
  To: Richard Henderson; +Cc: qemu-devel, laurent


Richard Henderson <richard.henderson@linaro.org> writes:

> The page isn't (necessarily) present in the host /proc/self/maps,
> and even if it might be it isn't present in page_flags, and even
> if it was it might not have the same set of page permissions.
>
> The easiest thing to do, particularly when it comes to the
> "[vsyscall]" note at the end of line, is to special case it.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  linux-user/syscall.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 171c0caef3..eb867a5296 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7005,6 +7005,15 @@ static int open_self_maps(void *cpu_env, int fd)
>          }
>      }
>  
> +#ifdef TARGET_X86_64
> +    /*
> +     * We only support execution from the vsyscall page.
> +     * This is as if CONFIG_LEGACY_VSYSCALL_XONLY=y from v5.3.
> +     */
> +    dprintf(fd, "ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0"
> +                "          [vsyscall]\n");
> +#endif
> +

I'm not sure what whitespace is wrong - tabs maybe? But looking at a
real vsyscall system vs emulated things line up better:

  Welcome to Buildroot
  buildroot login: root
  # uname -a
  Linux buildroot 5.4.13 #1 SMP Mon Jan 20 14:36:20 GMT 2020 x86_64 GNU/Linux
  # cat /proc/self/maps
  00400000-004ab000 r-xp 00000000 00:02 7635                               /bin/busybox
  004ab000-004ac000 r-xp 000aa000 00:02 7635                               /bin/busybox
  004ac000-004ad000 rwxp 000ab000 00:02 7635                               /bin/busybox
  004ad000-004ae000 rwxp 00000000 00:00 0
  7fcc91566000-7fcc91567000 rwxp 00000000 00:00 0
  7fcc91567000-7fcc915de000 r-xp 00000000 00:02 7475                       /lib/libuClibc-1.0.32.so
  7fcc915de000-7fcc915df000 ---p 00000000 00:00 0
  7fcc915df000-7fcc915e0000 r-xp 00077000 00:02 7475                       /lib/libuClibc-1.0.32.so
  7fcc915e0000-7fcc915e1000 rwxp 00078000 00:02 7475                       /lib/libuClibc-1.0.32.so
  7fcc915e1000-7fcc915fc000 rwxp 00000000 00:00 0
  7fcc915fc000-7fcc91603000 r-xp 00000000 00:02 7480                       /lib/ld64-uClibc-1.0.32.so
  7fcc91603000-7fcc91604000 r-xp 00006000 00:02 7480                       /lib/ld64-uClibc-1.0.32.so
  7fcc91604000-7fcc91605000 rwxp 00007000 00:02 7480                       /lib/ld64-uClibc-1.0.32.so
  7ffd92001000-7ffd92022000 rwxp 00000000 00:00 0                          [stack]
  7ffd920c3000-7ffd920c6000 r--p 00000000 00:00 0                          [vvar]
  7ffd920c6000-7ffd920c7000 r-xp 00000000 00:00 0                          [vdso]
  ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
  # QEMU 4.2.50 monitor - type 'help' for more information
  (qemu) quit
  14:38:52 [alex.bennee@hackbox2:~/l/q/b/all] review/fix-user-brk|… + ./x86_64-linux-user/qemu-x86_64 /bin/cat /proc/self/maps
  4000000000-4000008000 r-xp 00000000 00:19 8131          /bin/cat
  4000008000-4000207000 ---p 00000000 00:00 0
  4000207000-4000208000 r--p 00007000 00:19 8131          /bin/cat
  4000208000-4000209000 rw-p 00008000 00:19 8131          /bin/cat
  4000209000-400022a000 rw-p 00000000 00:00 0
  4001209000-400120a000 ---p 00000000 00:00 0
  400120a000-4001a0a000 rw-p 00000000 00:00 0                [stack]
  4001a0a000-4001a31000 r-xp 00000000 00:19 3644          /lib/x86_64-linux-gnu/ld-2.27.so
  4001a31000-4001c31000 ---p 00000000 00:00 0
  4001c31000-4001c32000 r--p 00027000 00:19 3644          /lib/x86_64-linux-gnu/ld-2.27.so
  4001c32000-4001c33000 rw-p 00028000 00:19 3644          /lib/x86_64-linux-gnu/ld-2.27.so
  4001c33000-4001c36000 rw-p 00000000 00:00 0
  4001c6d000-4001e54000 r-xp 00000000 00:19 3648          /lib/x86_64-linux-gnu/libc-2.27.so
  4001e54000-4002054000 ---p 001e7000 00:19 3648          /lib/x86_64-linux-gnu/libc-2.27.so
  4002054000-4002058000 r--p 001e7000 00:19 3648          /lib/x86_64-linux-gnu/libc-2.27.so
  4002058000-400205a000 rw-p 001eb000 00:19 3648          /lib/x86_64-linux-gnu/libc-2.27.so
  400205a000-4002060000 rw-p 00000000 00:00 0
  4002060000-40023d2000 r--p 00000000 00:19 195276          /usr/lib/locale/locale-archive
  ffffffffff600000-ffffffffff601000 --xp 00000000 00:00 0          [vsyscall]

>      free(line);
>      fclose(fp);


-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-20 11:48   ` Alex Bennée
@ 2020-01-21  3:38     ` Richard Henderson
  2020-01-21 10:13       ` Alex Bennée
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2020-01-21  3:38 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Paolo Bonzini, qemu-devel, laurent

On 1/20/20 1:48 AM, Alex Bennée wrote:
>> +    default:
>> +    sigsegv:
> 
> this label looks a little extraneous.
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> 

Look a little further down:

> +    default:
> +    sigsegv:
> +        /* Like force_sig(SIGSEGV).  */
> +        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
> +        return;
> +    }
> +
> +    /*
> +     * Validate the return address.
> +     * Note that the kernel treats this the same as an invalid entry point.
> +     */
> +    if (get_user_u64(caller, env->regs[R_ESP])) {
> +        goto sigsegv;
> +    }


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-21  3:38     ` Richard Henderson
@ 2020-01-21 10:13       ` Alex Bennée
  2020-01-21 15:51         ` Richard Henderson
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Bennée @ 2020-01-21 10:13 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Paolo Bonzini, qemu-devel, laurent


Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/20/20 1:48 AM, Alex Bennée wrote:
>>> +    default:
>>> +    sigsegv:
>> 
>> this label looks a little extraneous.
>> 
>> Otherwise:
>> 
>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>> 
>
> Look a little further down:
>
>> +    default:
>> +    sigsegv:
>> +        /* Like force_sig(SIGSEGV).  */
>> +        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>> +        return;
>> +    }
>> +
>> +    /*
>> +     * Validate the return address.
>> +     * Note that the kernel treats this the same as an invalid entry point.
>> +     */
>> +    if (get_user_u64(caller, env->regs[R_ESP])) {
>> +        goto sigsegv;
>> +    }

Wouldn't this read better:

    /*
     * Validate the entry point.  We have already validated the page
     * during translation, now verify the offset.
     */
    switch (env->eip & ~TARGET_PAGE_MASK) {
    case 0x000:
        syscall = TARGET_NR_gettimeofday;
        break;
    case 0x400:
        syscall = TARGET_NR_time;
        break;
    case 0x800:
        syscall = TARGET_NR_getcpu;
        break;
    default:
        syscall = -1;
        break;
    }

    /*
     * If we have an invalid entry point or an invalid return address we
     * generate a SIGSEG.
     */
    if (syscall < 0 || get_user_u64(caller, env->regs[R_ESP])) {
        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
        return;
    }

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-21 10:13       ` Alex Bennée
@ 2020-01-21 15:51         ` Richard Henderson
  2020-01-21 16:15           ` Alex Bennée
  0 siblings, 1 reply; 18+ messages in thread
From: Richard Henderson @ 2020-01-21 15:51 UTC (permalink / raw)
  To: Alex Bennée; +Cc: Paolo Bonzini, qemu-devel, laurent

On 1/21/20 12:13 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 1/20/20 1:48 AM, Alex Bennée wrote:
>>>> +    default:
>>>> +    sigsegv:
>>>
>>> this label looks a little extraneous.
>>>
>>> Otherwise:
>>>
>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>
>>
>> Look a little further down:
>>
>>> +    default:
>>> +    sigsegv:
>>> +        /* Like force_sig(SIGSEGV).  */
>>> +        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>> +        return;
>>> +    }
>>> +
>>> +    /*
>>> +     * Validate the return address.
>>> +     * Note that the kernel treats this the same as an invalid entry point.
>>> +     */
>>> +    if (get_user_u64(caller, env->regs[R_ESP])) {
>>> +        goto sigsegv;
>>> +    }
> 
> Wouldn't this read better:
> 
>     /*
>      * Validate the entry point.  We have already validated the page
>      * during translation, now verify the offset.
>      */
>     switch (env->eip & ~TARGET_PAGE_MASK) {
>     case 0x000:
>         syscall = TARGET_NR_gettimeofday;
>         break;
>     case 0x400:
>         syscall = TARGET_NR_time;
>         break;
>     case 0x800:
>         syscall = TARGET_NR_getcpu;
>         break;
>     default:
>         syscall = -1;
>         break;
>     }
> 
>     /*
>      * If we have an invalid entry point or an invalid return address we
>      * generate a SIGSEG.
>      */
>     if (syscall < 0 || get_user_u64(caller, env->regs[R_ESP])) {
>         gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>         return;
>     }
> 

Only if you have a violent goto allergy.


r~


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-21 15:51         ` Richard Henderson
@ 2020-01-21 16:15           ` Alex Bennée
  2020-01-21 16:23             ` Paolo Bonzini
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Bennée @ 2020-01-21 16:15 UTC (permalink / raw)
  To: Richard Henderson; +Cc: Paolo Bonzini, qemu-devel, laurent


Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/21/20 12:13 AM, Alex Bennée wrote:
>> 
>> Richard Henderson <richard.henderson@linaro.org> writes:
>> 
>>> On 1/20/20 1:48 AM, Alex Bennée wrote:
>>>>> +    default:
>>>>> +    sigsegv:
>>>>
>>>> this label looks a little extraneous.
>>>>
>>>> Otherwise:
>>>>
>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>
>>>
>>> Look a little further down:
>>>
>>>> +    default:
>>>> +    sigsegv:
>>>> +        /* Like force_sig(SIGSEGV).  */
>>>> +        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Validate the return address.
>>>> +     * Note that the kernel treats this the same as an invalid entry point.
>>>> +     */
>>>> +    if (get_user_u64(caller, env->regs[R_ESP])) {
>>>> +        goto sigsegv;
>>>> +    }
>> 
>> Wouldn't this read better:
>> 
>>     /*
>>      * Validate the entry point.  We have already validated the page
>>      * during translation, now verify the offset.
>>      */
>>     switch (env->eip & ~TARGET_PAGE_MASK) {
>>     case 0x000:
>>         syscall = TARGET_NR_gettimeofday;
>>         break;
>>     case 0x400:
>>         syscall = TARGET_NR_time;
>>         break;
>>     case 0x800:
>>         syscall = TARGET_NR_getcpu;
>>         break;
>>     default:
>>         syscall = -1;
>>         break;
>>     }
>> 
>>     /*
>>      * If we have an invalid entry point or an invalid return address we
>>      * generate a SIGSEG.
>>      */
>>     if (syscall < 0 || get_user_u64(caller, env->regs[R_ESP])) {
>>         gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>         return;
>>     }
>> 
>
> Only if you have a violent goto allergy.

gotos have their place but jumping backwards is confusing to eye. If the
compiler want to mess with layout after then it is free to do so.

-- 
Alex Bennée


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls
  2020-01-21 16:15           ` Alex Bennée
@ 2020-01-21 16:23             ` Paolo Bonzini
  0 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2020-01-21 16:23 UTC (permalink / raw)
  To: Alex Bennée, Richard Henderson; +Cc: qemu-devel, laurent

On 21/01/20 17:15, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 1/21/20 12:13 AM, Alex Bennée wrote:
>>>
>>> Richard Henderson <richard.henderson@linaro.org> writes:
>>>
>>>> On 1/20/20 1:48 AM, Alex Bennée wrote:
>>>>>> +    default:
>>>>>> +    sigsegv:
>>>>>
>>>>> this label looks a little extraneous.
>>>>>
>>>>> Otherwise:
>>>>>
>>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
>>>>>
>>>>
>>>> Look a little further down:
>>>>
>>>>> +    default:
>>>>> +    sigsegv:
>>>>> +        /* Like force_sig(SIGSEGV).  */
>>>>> +        gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /*
>>>>> +     * Validate the return address.
>>>>> +     * Note that the kernel treats this the same as an invalid entry point.
>>>>> +     */
>>>>> +    if (get_user_u64(caller, env->regs[R_ESP])) {
>>>>> +        goto sigsegv;
>>>>> +    }
>>>
>>> Wouldn't this read better:
>>>
>>>     /*
>>>      * Validate the entry point.  We have already validated the page
>>>      * during translation, now verify the offset.
>>>      */
>>>     switch (env->eip & ~TARGET_PAGE_MASK) {
>>>     case 0x000:
>>>         syscall = TARGET_NR_gettimeofday;
>>>         break;
>>>     case 0x400:
>>>         syscall = TARGET_NR_time;
>>>         break;
>>>     case 0x800:
>>>         syscall = TARGET_NR_getcpu;
>>>         break;
>>>     default:
>>>         syscall = -1;
>>>         break;
>>>     }
>>>
>>>     /*
>>>      * If we have an invalid entry point or an invalid return address we
>>>      * generate a SIGSEG.
>>>      */
>>>     if (syscall < 0 || get_user_u64(caller, env->regs[R_ESP])) {
>>>         gen_signal(env, TARGET_SIGSEGV, TARGET_SI_KERNEL, 0);
>>>         return;
>>>     }
>>>
>>
>> Only if you have a violent goto allergy.
> 
> gotos have their place but jumping backwards is confusing to eye. If the
> compiler want to mess with layout after then it is free to do so.

I agree, if anything I'd place the sigsegv label at the end of the
function but Alex's version is just fine too.

Paolo



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls
  2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
                   ` (5 preceding siblings ...)
  2020-01-16 19:46 ` [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
@ 2020-01-28 17:26 ` Laurent Vivier
  6 siblings, 0 replies; 18+ messages in thread
From: Laurent Vivier @ 2020-01-28 17:26 UTC (permalink / raw)
  To: Richard Henderson, qemu-devel; +Cc: alex.bennee

Le 16/01/2020 à 20:43, Richard Henderson a écrit :
> Changes since v2:
> 
>   * Add /proc/self/maps line
> 
> 	I'm not sure this is really necessary.  The linux kernel
> 	self-test checks for it, and modifies the set of tests that
> 	it runs based on it.  But otherwise I think it's unused.
> 
>   * Fix errors in base gettimeofday syscall
> 
> 	This is also checked by test_vsyscall, as noticed by AJB.
> 

Hi Richard,

do you plan to send a v3?

Thanks,
Laurent

> r~
> 
> 
> Original blurb:
> 
> The x86_64 abi has a legacy vsyscall page.  The kernel folk
> have been trying to deprecate this since at least v3.1, but
> 
> (1) We don't implement the vdso that replaces vsyscalls,
> (2) As of v5.5, the vsyscall page is still enabled by default.
> 
> This lack is affecting Peter's linux-user testing.
> 
> The dependency is not obvious because Peter is running the tests
> on x86_64, so the host is providing a vsyscall page to qemu.
> 
> Because of how user-only memory operations are handled, with no
> validation of guest vs host pages, so long as qemu chooses to
> run with guest_base == 0, the guest may Just So Happen to read
> the host's vsyscall page.
> 
> Complicating this, new OS releases may use a kernel configured
> with CONFIG_LEGACY_VSYSCALL_XONLY=y, which means the the vsyscall
> page cannot be read, only executed.  Which means that the guest
> then cannot read the host vsyscall page during translation and
> will SIGSEGV.
> 
> Exactly which of these many variables is affecting Peter's testing
> with Ubuntu 18.04 of my TCG merge, I'm not exactly sure.  I suspect
> that it is the change to drop the textseg_addr adjustment to user-only
> static binaries.  IIRC bionic does not support -static-pie, which is
> the preferred replacement.  This could mean that the host and guest
> binaries overlap, which leads to guest_base != 0.
> 
> I vaguely remember someone (Paolo?) implementing something like
> this many years ago, but clearly it never got merged.
> 
> In any case, this emulation has been missing for too long.
> 
> 
> Richard Henderson (5):
>   target/i386: Renumber EXCP_SYSCALL
>   linux-user/i386: Split out gen_signal
>   linux-user/i386: Emulate x86_64 vsyscalls
>   linux-user: Add x86_64 vsyscall page to /proc/self/maps
>   linux-user: Flush out implementation of gettimeofday
> 
>  target/i386/cpu.h          |   6 +-
>  linux-user/i386/cpu_loop.c | 198 ++++++++++++++++++++++++++-----------
>  linux-user/syscall.c       |  36 ++++++-
>  target/i386/translate.c    |  16 ++-
>  4 files changed, 190 insertions(+), 66 deletions(-)
> 



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 19:43 [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
2020-01-16 19:43 ` [PATCH v2 1/5] target/i386: Renumber EXCP_SYSCALL Richard Henderson
2020-01-16 19:43 ` [PATCH v2 2/5] linux-user/i386: Split out gen_signal Richard Henderson
2020-01-16 19:43 ` [PATCH v2 3/5] linux-user/i386: Emulate x86_64 vsyscalls Richard Henderson
2020-01-20 11:48   ` Alex Bennée
2020-01-21  3:38     ` Richard Henderson
2020-01-21 10:13       ` Alex Bennée
2020-01-21 15:51         ` Richard Henderson
2020-01-21 16:15           ` Alex Bennée
2020-01-21 16:23             ` Paolo Bonzini
2020-01-16 19:43 ` [PATCH v2 4/5] linux-user: Add x86_64 vsyscall page to /proc/self/maps Richard Henderson
2020-01-17  6:39   ` Philippe Mathieu-Daudé
2020-01-20 14:41   ` Alex Bennée
2020-01-16 19:43 ` [PATCH v2 5/5] linux-user: Flush out implementation of gettimeofday Richard Henderson
2020-01-17  6:37   ` Philippe Mathieu-Daudé
2020-01-20 13:27   ` Alex Bennée
2020-01-16 19:46 ` [PATCH v2 0/5] linux-user: Implement x86_64 vsyscalls Richard Henderson
2020-01-28 17:26 ` Laurent Vivier

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git