native-lib mode: avoid unsoundness due to mrpotect

1 files changed, 13 insertions, 17 deletions

diff --git a/src/tools/miri/src/shims/native_lib/trace/child.rs b/src/tools/miri/src/shims/native_lib/trace/child.rs
index b998ba822dd..95b0617a026 100644
--- a/src/tools/miri/src/shims/native_lib/trace/child.rs
+++ b/src/tools/miri/src/shims/native_lib/trace/child.rs
@@ -90,14 +90,6 @@ impl Supervisor {
         // Unwinding might be messed up due to partly protected memory, so let's abort if something
         // breaks inside here.
         let res = std::panic::abort_unwind(|| {
-            // SAFETY: We do not access machine memory past this point until the
-            // supervisor is ready to allow it.
-            // FIXME: this is sketchy, as technically the memory is still in the Rust Abstract Machine,
-            // and the compiler would be allowed to reorder accesses below this block...
-            unsafe {
-                Self::protect_pages(alloc.pages(), mman::ProtFlags::PROT_NONE).unwrap();
-            }
-
             // Send over the info.
             // NB: if we do not wait to receive a blank confirmation response, it is
             // possible that the supervisor is alerted of the SIGSTOP *before* it has
@@ -110,16 +102,14 @@ impl Supervisor {
             // count.
             signal::raise(signal::SIGSTOP).unwrap();
 
-            let res = f();
+            // SAFETY: We have coordinated with the supervisor to ensure that this memory will keep
+            // working as normal, just with extra tracing. So even if the compiler moves memory
+            // accesses down to after the `mprotect`, they won't actually segfault.
+            unsafe {
+                Self::protect_pages(alloc.pages(), mman::ProtFlags::PROT_NONE).unwrap();
+            }
 
-            // We can't use IPC channels here to signal that FFI mode has ended,
-            // since they might allocate memory which could get us stuck in a SIGTRAP
-            // with no easy way out! While this could be worked around, it is much
-            // simpler and more robust to simply use the signals which are left for
-            // arbitrary usage. Since this will block until we are continued by the
-            // supervisor, we can assume past this point that everything is back to
-            // normal.
-            signal::raise(signal::SIGUSR1).unwrap();
+            let res = f();
 
             // SAFETY: We set memory back to normal, so this is safe.
             unsafe {
@@ -130,6 +120,12 @@ impl Supervisor {
                 .unwrap();
             }
 
+            // Signal the supervisor that we are done. Will block until the supervisor continues us.
+            // This will also shut down the segfault handler, so it's important that all memory is
+            // reset back to normal above. There must not be a window in time where accessing the
+            // pages we protected above actually causes the program to abort.
+            signal::raise(signal::SIGUSR1).unwrap();
+
             res
         });