[Qemu-devel] [PATCH v1 4/4] s390x/tcg: MOVE (MVC): Fault-safe handling

[David Hildenbrand]

MVC can cross page boundaries. In case we fault on the second page, we
already partially copied data. If we have overlaps, we would
trigger a fault after having partially moved data, eventually having
our original data already overwritten. When continuing after the fault,
we would try to move already modified data, not the original data -
very bad.

glibc started to use MVC for forward memmove() and is able to trigger
exectly this corruption (via rpmbuild and rpm). Fedora 31 (rawhide)
currently fails to install as we trigger rpm database corruptions due to
this bug.

Let's properly probe for read/write access in case we cross page
boundaries. In case we don't cross boundaries, the first accesses will
trigger the fault.

We'll have to do the same for other instructions (like MVCLE), too. But
the more I look at the other MOVE variantes the more issues I find, so
let's handle MVC for now only.

Signed-off-by: David Hildenbrand <address@hidden>
---
 target/s390x/mem_helper.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/target/s390x/mem_helper.c b/target/s390x/mem_helper.c
index bf7dfcdc7a..44001ec21a 100644
--- a/target/s390x/mem_helper.c
+++ b/target/s390x/mem_helper.c
@@ -104,6 +104,11 @@ static inline void cpu_stsize_data_ra(CPUS390XState *env, 
uint64_t addr,
     }
 }
 
+static inline bool is_single_page_access(uint64_t addr, uint32_t size)
+{
+    return (addr & TARGET_PAGE_MASK) == ((addr + size - 1) & TARGET_PAGE_MASK);
+}
+
 static void fast_memset(CPUS390XState *env, uint64_t dest, uint8_t byte,
                         uint32_t l, uintptr_t ra)
 {
@@ -310,6 +315,10 @@ static uint32_t do_helper_mvc(CPUS390XState *env, uint32_t 
l, uint64_t dest,
     /* MVC always copies one more byte than specified - maximum is 256 */
     l++;
 
+    if (unlikely(!is_single_page_access(dest, l))) {
+        probe_write_access(env, dest, l, ra);
+    }
+
     /*
      * "When the operands overlap, the result is obtained as if the operands
      * were processed one byte at a time". Only non-overlapping or forward
@@ -317,7 +326,14 @@ static uint32_t do_helper_mvc(CPUS390XState *env, uint32_t 
l, uint64_t dest,
      */
     if (dest == src + 1) {
         fast_memset(env, dest, cpu_ldub_data_ra(env, src, ra), l, ra);
-    } else if (dest < src || src + l <= dest) {
+        return env->cc_op;
+    }
+
+    if (unlikely(!is_single_page_access(src, l))) {
+        probe_read_access(env, src, l, ra);
+    }
+
+    if (dest < src || src + l <= dest) {
         fast_memmove(env, dest, src, l, ra);
     } else {
         for (i = 0; i < l; i++) {
-- 
2.21.0