diff mbox series

[v3,28/28] target/i386: Pass host pointer and size to cpu_x86_{xsave, xrstor}

Message ID 20240515150837.259747-29-richard.henderson@linaro.org
State Superseded
Headers show
Series linux-user/i386: Properly align signal frame | expand

Commit Message

Richard Henderson May 15, 2024, 3:08 p.m. UTC 
We have already validated the memory region in the course of
validating the signal frame.  No need to do it again within
the helper function.

In addition, return failure when the header contains invalid
xstate_bv.  The kernel handles this via exception handling
within XSTATE_OP within xrstor_from_user_sigframe.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/i386/cpu.h            |  4 ++--
 linux-user/i386/signal.c     | 20 ++++++++++++--------
 target/i386/tcg/fpu_helper.c | 36 +++++++++++++++++++-----------------
 3 files changed, 33 insertions(+), 27 deletions(-)
diff mbox series

Patch

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index dbd91c064d..df75f4c2b1 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -2245,8 +2245,8 @@  void cpu_x86_fsave(CPUX86State *s, void *host, size_t len);
 void cpu_x86_frstor(CPUX86State *s, void *host, size_t len);
 void cpu_x86_fxsave(CPUX86State *s, void *host, size_t len);
 void cpu_x86_fxrstor(CPUX86State *s, void *host, size_t len);
-void cpu_x86_xsave(CPUX86State *s, target_ulong ptr, uint64_t rbfm);
-void cpu_x86_xrstor(CPUX86State *s, target_ulong ptr, uint64_t rbfm);
+void cpu_x86_xsave(CPUX86State *s, void *host, size_t len, uint64_t rbfm);
+bool cpu_x86_xrstor(CPUX86State *s, void *host, size_t len, uint64_t rbfm);
 
 /* cpu.c */
 void x86_cpu_vendor_words2str(char *dst, uint32_t vendor1,
diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index 1f5a3e9a0b..16f6d17345 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -326,7 +326,7 @@  static void xsave_sigcontext(CPUX86State *env,
 
     /* Zero the header, XSAVE *adds* features to an existing save state.  */
     memset(fxstate + 1, 0, sizeof(X86XSaveHeader));
-    cpu_x86_xsave(env, xstate_addr, env->xcr0);
+    cpu_x86_xsave(env, fxstate, fpend_addr - xstate_addr, env->xcr0);
 
     __put_user(TARGET_FP_XSTATE_MAGIC1, &sw->magic1);
     __put_user(extended_size, &sw->extended_size);
@@ -611,6 +611,8 @@  static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
     uint32_t magic1, magic2;
     uint32_t extended_size, xstate_size, min_size, max_size;
     uint64_t xfeatures;
+    void *xstate;
+    bool ok;
 
     switch (fpkind) {
     case FPSTATE_XSAVE:
@@ -641,8 +643,10 @@  static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
             return false;
         }
 
-        if (!access_ok(env_cpu(env), VERIFY_READ, fxstate_addr,
-                       xstate_size + TARGET_FP_XSTATE_MAGIC2_SIZE)) {
+        /* Re-lock the entire xstate area, with the extensions and magic. */
+        xstate = lock_user(VERIFY_READ, fxstate_addr,
+                           xstate_size + TARGET_FP_XSTATE_MAGIC2_SIZE, 1);
+        if (!xstate) {
             return false;
         }
 
@@ -652,15 +656,15 @@  static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
          * fpstate layout with out copying the extended state information
          * in the memory layout.
          */
-        if (get_user_u32(magic2, fxstate_addr + xstate_size)) {
-            return false;
-        }
+        magic2 = tswap32(*(uint32_t *)(xstate + xstate_size));
         if (magic2 != FP_XSTATE_MAGIC2) {
+            unlock_user(xstate, fxstate_addr, 0);
             break;
         }
 
-        cpu_x86_xrstor(env, fxstate_addr, xfeatures);
-        return true;
+        ok = cpu_x86_xrstor(env, xstate, xstate_size, xfeatures);
+        unlock_user(xstate, fxstate_addr, 0);
+        return ok;
 
     default:
         break;
diff --git a/target/i386/tcg/fpu_helper.c b/target/i386/tcg/fpu_helper.c
index 0e5368951f..c17eaaa22b 100644
--- a/target/i386/tcg/fpu_helper.c
+++ b/target/i386/tcg/fpu_helper.c
@@ -3065,42 +3065,44 @@  void cpu_x86_fxrstor(CPUX86State *env, void *host, size_t len)
     do_fxrstor(&ac, 0);
 }
 
-void cpu_x86_xsave(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
+void cpu_x86_xsave(CPUX86State *env, void *host, size_t len, uint64_t rfbm)
 {
-    X86Access ac;
-    unsigned size;
+    X86Access ac = {
+        .haddr1 = host,
+        .env = env,
+    };
 
     /*
      * Since this is only called from user-level signal handling,
      * we should have done the job correctly there.
      */
     assert((rfbm & ~env->xcr0) == 0);
-    size = xsave_area_size(rfbm, false);
-
-    access_prepare(&ac, env, ptr, size, MMU_DATA_STORE, 0);
-    do_xsave_access(&ac, ptr, rfbm, get_xinuse(env), rfbm);
+    ac.size = xsave_area_size(rfbm, false);
+    assert(ac.size <= len);
+    do_xsave_access(&ac, 0, rfbm, get_xinuse(env), rfbm);
 }
 
-void cpu_x86_xrstor(CPUX86State *env, target_ulong ptr, uint64_t rfbm)
+bool cpu_x86_xrstor(CPUX86State *env, void *host, size_t len, uint64_t rfbm)
 {
-    X86Access ac;
+    X86Access ac = {
+        .haddr1 = host,
+        .env = env,
+    };
     uint64_t xstate_bv;
-    unsigned size;
 
     /*
      * Since this is only called from user-level signal handling,
      * we should have done the job correctly there.
      */
     assert((rfbm & ~env->xcr0) == 0);
-    size = xsave_area_size(rfbm, false);
-    access_prepare(&ac, env, ptr, size, MMU_DATA_LOAD, 0);
+    ac.size = xsave_area_size(rfbm, false);
+    assert(ac.size <= len);
 
-    if (!valid_xrstor_header(&ac, &xstate_bv, ptr)) {
-        /* TODO: Report failure to caller. */
-        xstate_bv &= env->xcr0;
+    if (!valid_xrstor_header(&ac, &xstate_bv, 0)) {
+        return false;
     }
-
-    do_xrstor(&ac, ptr, rfbm, xstate_bv);
+    do_xrstor(&ac, 0, rfbm, xstate_bv);
+    return true;
 }
 #endif