Skip to content

Commit 3799cbd

Browse files
authored
Merge pull request rust-lang#4476 from RalfJung/page-prot
move page protection logic inside native_lib
2 parents aef3d9e + a438401 commit 3799cbd

File tree

6 files changed

+84
-93
lines changed

6 files changed

+84
-93
lines changed

src/tools/miri/Cargo.toml

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,13 +38,14 @@ features = ['unprefixed_malloc_on_supported_platforms']
3838

3939
[target.'cfg(unix)'.dependencies]
4040
libc = "0.2"
41+
# native-lib dependencies
4142
libffi = { version = "4.0.0", optional = true }
4243
libloading = { version = "0.8", optional = true }
43-
nix = { version = "0.30.1", features = ["mman", "ptrace", "signal"], optional = true }
44+
serde = { version = "1.0.219", features = ["derive"], optional = true }
4445

4546
[target.'cfg(target_os = "linux")'.dependencies]
47+
nix = { version = "0.30.1", features = ["mman", "ptrace", "signal"], optional = true }
4648
ipc-channel = { version = "0.19.0", optional = true }
47-
serde = { version = "1.0.219", features = ["derive"], optional = true }
4849
capstone = { version = "0.13", optional = true }
4950

5051
[dev-dependencies]

src/tools/miri/src/alloc/isolated_alloc.rs

Lines changed: 9 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
use std::alloc::Layout;
22
use std::ptr::NonNull;
33

4-
use nix::sys::mman;
54
use rustc_index::bit_set::DenseBitSet;
65

76
/// How many bytes of memory each bit in the bitset represents.
@@ -44,6 +43,10 @@ impl IsolatedAlloc {
4443
}
4544
}
4645

46+
pub fn page_size(&self) -> usize {
47+
self.page_size
48+
}
49+
4750
/// For simplicity, we serve small allocations in multiples of COMPRESSION_FACTOR
4851
/// bytes with at least that alignment.
4952
#[inline]
@@ -302,50 +305,11 @@ impl IsolatedAlloc {
302305
}
303306
}
304307

305-
/// Returns a list of page addresses managed by the allocator.
306-
pub fn pages(&self) -> impl Iterator<Item = usize> {
307-
let pages = self.page_ptrs.iter().map(|p| p.expose_provenance().get());
308-
pages.chain(self.huge_ptrs.iter().flat_map(|(ptr, size)| {
309-
(0..size / self.page_size)
310-
.map(|i| ptr.expose_provenance().get().strict_add(i * self.page_size))
311-
}))
312-
}
313-
314-
/// Protects all owned memory as `PROT_NONE`, preventing accesses.
315-
///
316-
/// SAFETY: Accessing memory after this point will result in a segfault
317-
/// unless it is first unprotected.
318-
pub unsafe fn start_ffi(&mut self) -> Result<(), nix::errno::Errno> {
319-
let prot = mman::ProtFlags::PROT_NONE;
320-
unsafe { self.mprotect(prot) }
321-
}
322-
323-
/// Deprotects all owned memory by setting it to RW. Erroring here is very
324-
/// likely unrecoverable, so it may panic if applying those permissions
325-
/// fails.
326-
pub fn end_ffi(&mut self) {
327-
let prot = mman::ProtFlags::PROT_READ | mman::ProtFlags::PROT_WRITE;
328-
unsafe {
329-
self.mprotect(prot).unwrap();
330-
}
331-
}
332-
333-
/// Applies `prot` to every page managed by the allocator.
334-
///
335-
/// SAFETY: Accessing memory in violation of the protection flags will
336-
/// trigger a segfault.
337-
unsafe fn mprotect(&mut self, prot: mman::ProtFlags) -> Result<(), nix::errno::Errno> {
338-
for &pg in &self.page_ptrs {
339-
unsafe {
340-
mman::mprotect(pg.cast(), self.page_size, prot)?;
341-
}
342-
}
343-
for &(hpg, size) in &self.huge_ptrs {
344-
unsafe {
345-
mman::mprotect(hpg.cast(), size.next_multiple_of(self.page_size), prot)?;
346-
}
347-
}
348-
Ok(())
308+
/// Returns a list of page ranges managed by the allocator, given in terms of pointers
309+
/// and size (in bytes).
310+
pub fn pages(&self) -> impl Iterator<Item = (NonNull<u8>, usize)> {
311+
let pages = self.page_ptrs.iter().map(|&p| (p, self.page_size));
312+
pages.chain(self.huge_ptrs.iter().copied())
349313
}
350314
}
351315

src/tools/miri/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#![feature(abort_unwind)]
12
#![feature(cfg_select)]
23
#![feature(rustc_private)]
34
#![feature(float_gamma)]

src/tools/miri/src/shims/native_lib/mod.rs

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use rustc_abi::{BackendRepr, HasDataLayout, Size};
88
use rustc_middle::mir::interpret::Pointer;
99
use rustc_middle::ty::{self as ty, IntTy, UintTy};
1010
use rustc_span::Symbol;
11+
use serde::{Deserialize, Serialize};
1112

1213
#[cfg_attr(
1314
not(all(
@@ -23,18 +24,14 @@ use crate::*;
2324

2425
/// The final results of an FFI trace, containing every relevant event detected
2526
/// by the tracer.
26-
#[allow(dead_code)]
27-
#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
28-
#[derive(Debug)]
27+
#[derive(Serialize, Deserialize, Debug)]
2928
pub struct MemEvents {
3029
/// An list of memory accesses that occurred, in the order they occurred in.
3130
pub acc_events: Vec<AccessEvent>,
3231
}
3332

3433
/// A single memory access.
35-
#[allow(dead_code)]
36-
#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
37-
#[derive(Clone, Debug)]
34+
#[derive(Serialize, Deserialize, Clone, Debug)]
3835
pub enum AccessEvent {
3936
/// A read occurred on this memory range.
4037
Read(AccessRange),
@@ -56,9 +53,7 @@ impl AccessEvent {
5653
}
5754

5855
/// The memory touched by a given access.
59-
#[allow(dead_code)]
60-
#[cfg_attr(target_os = "linux", derive(serde::Serialize, serde::Deserialize))]
61-
#[derive(Clone, Debug)]
56+
#[derive(Serialize, Deserialize, Clone, Debug)]
6257
pub struct AccessRange {
6358
/// The base address in memory where an access occurred.
6459
pub addr: usize,

src/tools/miri/src/shims/native_lib/trace/child.rs

Lines changed: 66 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,9 @@
11
use std::cell::RefCell;
2+
use std::ptr::NonNull;
23
use std::rc::Rc;
34

45
use ipc_channel::ipc;
5-
use nix::sys::{ptrace, signal};
6+
use nix::sys::{mman, ptrace, signal};
67
use nix::unistd;
78
use rustc_const_eval::interpret::InterpResult;
89

@@ -44,6 +45,16 @@ impl Supervisor {
4445
SUPERVISOR.lock().unwrap().is_some()
4546
}
4647

48+
unsafe fn protect_pages(
49+
pages: impl Iterator<Item = (NonNull<u8>, usize)>,
50+
prot: mman::ProtFlags,
51+
) -> Result<(), nix::errno::Errno> {
52+
for (pg, sz) in pages {
53+
unsafe { mman::mprotect(pg.cast(), sz, prot)? };
54+
}
55+
Ok(())
56+
}
57+
4758
/// Performs an arbitrary FFI call, enabling tracing from the supervisor.
4859
/// As this locks the supervisor via a mutex, no other threads may enter FFI
4960
/// until this function returns.
@@ -60,47 +71,67 @@ impl Supervisor {
6071

6172
// Get pointers to all the pages the supervisor must allow accesses in
6273
// and prepare the callback stack.
63-
let page_ptrs = alloc.borrow().pages().collect();
74+
let alloc = alloc.borrow();
75+
let page_size = alloc.page_size();
76+
let page_ptrs = alloc
77+
.pages()
78+
.flat_map(|(pg, sz)| {
79+
// Convert (page, size) pair into list of pages.
80+
let start = pg.expose_provenance().get();
81+
(0..sz.strict_div(alloc.page_size()))
82+
.map(move |i| start.strict_add(i.strict_mul(page_size)))
83+
})
84+
.collect();
6485
let raw_stack_ptr: *mut [u8; CALLBACK_STACK_SIZE] =
6586
Box::leak(Box::new([0u8; CALLBACK_STACK_SIZE])).as_mut_ptr().cast();
6687
let stack_ptr = raw_stack_ptr.expose_provenance();
6788
let start_info = StartFfiInfo { page_ptrs, stack_ptr };
6889

69-
// SAFETY: We do not access machine memory past this point until the
70-
// supervisor is ready to allow it.
71-
unsafe {
72-
if alloc.borrow_mut().start_ffi().is_err() {
73-
// Don't mess up unwinding by maybe leaving the memory partly protected
74-
alloc.borrow_mut().end_ffi();
75-
panic!("Cannot protect memory for FFI call!");
90+
// Unwinding might be messed up due to partly protected memory, so let's abort if something
91+
// breaks inside here.
92+
let res = std::panic::abort_unwind(|| {
93+
// SAFETY: We do not access machine memory past this point until the
94+
// supervisor is ready to allow it.
95+
// FIXME: this is sketchy, as technically the memory is still in the Rust Abstract Machine,
96+
// and the compiler would be allowed to reorder accesses below this block...
97+
unsafe {
98+
Self::protect_pages(alloc.pages(), mman::ProtFlags::PROT_NONE).unwrap();
7699
}
77-
}
78100

79-
// Send over the info.
80-
// NB: if we do not wait to receive a blank confirmation response, it is
81-
// possible that the supervisor is alerted of the SIGSTOP *before* it has
82-
// actually received the start_info, thus deadlocking! This way, we can
83-
// enforce an ordering for these events.
84-
sv.message_tx.send(TraceRequest::StartFfi(start_info)).unwrap();
85-
sv.confirm_rx.recv().unwrap();
86-
// We need to be stopped for the supervisor to be able to make certain
87-
// modifications to our memory - simply waiting on the recv() doesn't
88-
// count.
89-
signal::raise(signal::SIGSTOP).unwrap();
90-
91-
let res = f();
92-
93-
// We can't use IPC channels here to signal that FFI mode has ended,
94-
// since they might allocate memory which could get us stuck in a SIGTRAP
95-
// with no easy way out! While this could be worked around, it is much
96-
// simpler and more robust to simply use the signals which are left for
97-
// arbitrary usage. Since this will block until we are continued by the
98-
// supervisor, we can assume past this point that everything is back to
99-
// normal.
100-
signal::raise(signal::SIGUSR1).unwrap();
101-
102-
// This is safe! It just sets memory to normal expected permissions.
103-
alloc.borrow_mut().end_ffi();
101+
// Send over the info.
102+
// NB: if we do not wait to receive a blank confirmation response, it is
103+
// possible that the supervisor is alerted of the SIGSTOP *before* it has
104+
// actually received the start_info, thus deadlocking! This way, we can
105+
// enforce an ordering for these events.
106+
sv.message_tx.send(TraceRequest::StartFfi(start_info)).unwrap();
107+
sv.confirm_rx.recv().unwrap();
108+
// We need to be stopped for the supervisor to be able to make certain
109+
// modifications to our memory - simply waiting on the recv() doesn't
110+
// count.
111+
signal::raise(signal::SIGSTOP).unwrap();
112+
113+
let res = f();
114+
115+
// We can't use IPC channels here to signal that FFI mode has ended,
116+
// since they might allocate memory which could get us stuck in a SIGTRAP
117+
// with no easy way out! While this could be worked around, it is much
118+
// simpler and more robust to simply use the signals which are left for
119+
// arbitrary usage. Since this will block until we are continued by the
120+
// supervisor, we can assume past this point that everything is back to
121+
// normal.
122+
signal::raise(signal::SIGUSR1).unwrap();
123+
124+
// SAFETY: We set memory back to normal, so this is safe.
125+
unsafe {
126+
Self::protect_pages(
127+
alloc.pages(),
128+
mman::ProtFlags::PROT_READ | mman::ProtFlags::PROT_WRITE,
129+
)
130+
.unwrap();
131+
}
132+
133+
res
134+
});
104135

105136
// SAFETY: Caller upholds that this pointer was allocated as a box with
106137
// this type.

src/tools/miri/src/shims/native_lib/trace/messages.rs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,7 @@ pub enum TraceRequest {
4545
/// Information needed to begin tracing.
4646
#[derive(serde::Serialize, serde::Deserialize, Debug, Clone)]
4747
pub struct StartFfiInfo {
48-
/// A vector of page addresses. These should have been automatically obtained
49-
/// with `IsolatedAlloc::pages` and prepared with `IsolatedAlloc::start_ffi`.
48+
/// A vector of page addresses that store the miri heap which is accessible from C.
5049
pub page_ptrs: Vec<usize>,
5150
/// The address of an allocation that can serve as a temporary stack.
5251
/// This should be a leaked `Box<[u8; CALLBACK_STACK_SIZE]>` cast to an int.

0 commit comments

Comments
 (0)