Host SIGBUS is now caught around direct guest-memory touches

src/core/guest.c

@@ -47,6 +47,7 @@
 #include "runtime/futex.h"  /* futex_interrupt_request */
 #include "runtime/thread.h" /* thread_destroy_all_vcpus */
 #include "syscall/proc.h"   /* proc_request_exit_group */
+#include "syscall/signal.h"
 #include "syscall/wakeup-pipe.h"
 
 /* Per-vCPU pending TLBI request. Zero-initialized in every host pthread by
@@ -64,6 +65,77 @@ bool g_tlbi_range_supported = false;
 
 static void guest_region_clear(guest_t *g);
 
+/* Copy across the guest slab boundary with host SIGBUS recovery armed.
+ *
+ * Returns -1 when the backing page vanished under a live MAP_SHARED overlay.
+ */
+static int guest_host_memcpy(void *dst, const void *src, size_t len)
+{
+    bool faulted;
+    HOST_SIGBUS_GUARD(faulted, memcpy(dst, src, len));
+    return faulted ? -1 : 0;
+}
+
+/* Copy between two already-resolved host pointers with the pad armed, returning
+ * how many bytes landed.
+ *
+ * guest_host_memcpy can only say whether the whole copy survived, so a caller
+ * reporting a partial transfer had to fall back to the count from before the
+ * copy and lose whatever the faulting copy had already moved. Stepping bounds
+ * that, but only if a step cannot itself be torn: each one is cut at the next
+ * page boundary on whichever side reaches one first, so it lies within a single
+ * page at both ends. A page is the granularity at which backing vanishes, so
+ * such a step either lands whole or faults whole, and done is exact rather than
+ * approximate. A flat stride would not do: unaligned against the real
+ * boundaries by up to a page, it could tear and lose what it had written.
+ *
+ * The unit is the guest page rather than the host's 16 KiB, so the granularity
+ * holds whichever side vanished. done is volatile because it is the one value
+ * read after the jump.
+ *
+ * Exact has one exception, and it is a race rather than a miscount: a truncate
+ * landing while a step is mid-copy can take the page after part of that step
+ * has been written, and those bytes go unreported. The error is bounded by one
+ * page and is always an under-report, which callers absorb because a short
+ * count means resume-from-here and re-copying is idempotent. si_addr cannot
+ * close it: nothing specifies the order memmove touches its range in, so the
+ * faulting address does not say how much of the step had landed.
+ */
+size_t guest_host_copy_partial(void *dst, const void *src, size_t len)
+{
+    volatile size_t done = 0;
+    bool faulted;
+    uintptr_t d = (uintptr_t) dst, s = (uintptr_t) src;
+
+    HOST_SIGBUS_GUARD(
+        faulted, while (done < len) {
+            size_t to_d =
+                GUEST_PAGE_SIZE - ((d + done) & (GUEST_PAGE_SIZE - 1));
+            size_t to_s =
+                GUEST_PAGE_SIZE - ((s + done) & (GUEST_PAGE_SIZE - 1));
+            size_t unit = to_d < to_s ? to_d : to_s;
+            if (unit > len - done)
+                unit = len - done;
+            memmove((uint8_t *) dst + done, (const uint8_t *) src + done, unit);
+            done += unit;
+        });
+    (void) faulted;
+    return done;
+}
+
+static const void *guest_host_memchr(const void *src,
+                                     int c,
+                                     size_t len,
+                                     bool *faulted)
+{
+    /* found is read only on the non-faulted path, so it never survives a
+     * _longjmp, which is what would leave a non-volatile local indeterminate.
+     */
+    const void *found = NULL;
+    HOST_SIGBUS_GUARD(*faulted, found = memchr(src, c, len));
+    return *faulted ? NULL : found;
+}
+
 /* Page table descriptor bits. */
 #define PT_VALID (1ULL << 0)
 #define PT_TABLE (1ULL << 1)     /* Table descriptor (L0/L1/L2) */
@@ -1632,10 +1704,13 @@ static inline int guest_copy(const guest_t *g,
         size_t chunk = len - copied;
         if (chunk > avail)
             chunk = avail;
-        if (required_perms == MEM_PERM_R)
-            memcpy((uint8_t *) dst + copied, ptr, chunk);
-        else
-            memcpy(ptr, (const uint8_t *) src + copied, chunk);
+        if (required_perms == MEM_PERM_R) {
+            if (guest_host_memcpy((uint8_t *) dst + copied, ptr, chunk) < 0)
+                return -1;
+        } else if (guest_host_memcpy(ptr, (const uint8_t *) src + copied,
+                                     chunk) < 0) {
+            return -1;
+        }
         copied += chunk;
     }
     return 0;
@@ -1651,8 +1726,7 @@ int guest_read_small(const guest_t *g, uint64_t gva, void *dst, size_t len)
     uint64_t avail = 0;
     void *src = guest_ptr_bound(g, gva, &avail, MEM_PERM_R, (uint64_t) len);
     if (src && avail >= len) {
-        memcpy(dst, src, len);
-        return 0;
+        return guest_host_memcpy(dst, src, len);
     }
 
     return guest_read(g, gva, dst, len);
@@ -1668,8 +1742,7 @@ int guest_write_small(guest_t *g, uint64_t gva, const void *src, size_t len)
     uint64_t avail = 0;
     void *dst = guest_ptr_bound(g, gva, &avail, MEM_PERM_W, (uint64_t) len);
     if (dst && avail >= len) {
-        memcpy(dst, src, len);
-        return 0;
+        return guest_host_memcpy(dst, src, len);
     }
 
     return guest_write(g, gva, src, len);
@@ -1681,6 +1754,12 @@ int guest_read_str(const guest_t *g, uint64_t gva, char *dst, size_t max)
         return -1;
     size_t copied = 0, limit = max - 1;
 
+    /* Separated from the other ways out, because a caller that retries with a
+     * bigger buffer must not retry a fault: the same address faults again. Both
+     * still return negative, so a caller testing < 0 is unaffected.
+     */
+    bool faulted = false;
+
     while (copied < limit) {
         if (gva > UINT64_MAX - copied)
             break;
@@ -1693,18 +1772,26 @@ int guest_read_str(const guest_t *g, uint64_t gva, char *dst, size_t max)
         size_t remain = limit - copied, chunk = avail < remain ? avail : remain;
         const char *src = (const char *) ptr;
 
-        const void *nul = memchr(src, '\0', chunk);
+        const void *nul = guest_host_memchr(src, '\0', chunk, &faulted);
+        if (faulted)
+            break;
         if (nul) {
             size_t slen = (const char *) nul - src;
-            memcpy(dst + copied, src, slen + 1);
+            if (guest_host_memcpy(dst + copied, src, slen + 1) < 0) {
+                faulted = true;
+                break;
+            }
             return (int) (copied + slen);
         }
-        memcpy(dst + copied, src, chunk);
+        if (guest_host_memcpy(dst + copied, src, chunk) < 0) {
+            faulted = true;
+            break;
+        }
         copied += chunk;
     }
 
     dst[copied] = '\0';
-    return -1;
+    return faulted ? -2 : -1;
 }
 
 int guest_read_str_small(const guest_t *g, uint64_t gva, char *dst, size_t max)
@@ -1717,12 +1804,23 @@ int guest_read_str_small(const guest_t *g, uint64_t gva, char *dst, size_t max)
     const char *src =
         guest_ptr_bound(g, gva, &avail, MEM_PERM_R, (uint64_t) limit);
     if (src && avail >= limit) {
-        const char *nul = memchr(src, '\0', limit);
+        bool faulted;
+        const char *nul = guest_host_memchr(src, '\0', limit, &faulted);
         if (nul) {
             size_t slen = (size_t) (nul - src);
-            memcpy(dst, src, slen + 1);
+            if (guest_host_memcpy(dst, src, slen + 1) < 0) {
+                /* Terminate as guest_read_str does on its own failure path, so
+                 * no caller can read the partially copied bytes as a string.
+                 */
+                dst[0] = '\0';
+                return -2;
+            }
             return (int) slen;
         }
+        if (faulted) {
+            dst[0] = '\0';
+            return -2;
+        }
     }
 
     return guest_read_str(g, gva, dst, max);

src/core/guest.h

@@ -1063,16 +1063,36 @@ int guest_write_small(guest_t *g, uint64_t gva, const void *src, size_t len);
 
 /* Read a null-terminated string from guest memory. Copies up to max-1 bytes +
  * NUL into dst.
- * Returns string length or -1 if out of bounds / unterminated.
+ *
+ * Returns the string length, -1 if out of bounds or unterminated, or -2 when a
+ * host SIGBUS hit a vanished MAP_SHARED page. The two failures differ only for
+ * a caller deciding whether to retry: a -1 can be worth another attempt with a
+ * bigger buffer, a -2 never is. Both are negative, so a caller testing < 0 need
+ * not care.
  */
 int guest_read_str(const guest_t *g, uint64_t gva, char *dst, size_t max);
 
 /* Optimized guest string read for short, contiguous paths. Uses a direct guest
  * pointer when a full max-1-byte window is readable, otherwise falls back to
  * guest_read_str() for boundary-safe scanning.
+ *
+ * Returns the string length, -1 when the fast window did not apply and the
+ * fallback also failed, or -2 when either took a host SIGBUS on a vanished
+ * MAP_SHARED page. A caller holding a larger buffer can retry a -1 but never a
+ * -2: the same address faults again. The fallback propagates its own -2, so a
+ * path that straddles a page boundary is not retried either.
  */
 int guest_read_str_small(const guest_t *g, uint64_t gva, char *dst, size_t max);
 
+/* Copy between two resolved host pointers with host SIGBUS recovery armed.
+ *
+ * Returns the number of bytes that landed, which is len when nothing faulted
+ * and less when a backing page vanished partway. Callers reporting a partial
+ * transfer use this instead of guest_host_memcpy, which can only say whether
+ * the whole copy survived.
+ */
+size_t guest_host_copy_partial(void *dst, const void *src, size_t len);
+
 /* Build L0->L1->L2 page tables from an array of memory regions. Uses 2MiB block
  * descriptors.
  *

src/debug/gdbstub.c

@@ -355,6 +355,14 @@ static thread_entry_t *find_thread_for_tid(int64_t tid)
     return thread_find(tid);
 }
 
+/* The one host-user-mode touch of guest memory left outside HOST_SIGBUS_GUARD.
+ *
+ * sys_icache_invalidate would fault the same way a memcpy does if the range sat
+ * in a MAP_SHARED overlay whose file had been truncated. It is left unguarded
+ * deliberately: reaching it requires --gdb, so no guest can drive it, and the
+ * ranges it flushes are code the debugger just wrote, not file-backed data. Do
+ * not treat this as precedent; anything a guest can reach belongs in the pad.
+ */
 static void gdb_invalidate_written_code(uint64_t gva, size_t len)
 {
     size_t flushed = 0;