Repository navigation
Commit 8f41ed2
authored
Rollup merge of rust-lang#161895 - phlip9:phlip9/fix-sgx-alloc-align, r=JohnTitor
std::sys::pal::sgx: fix mismatched alloc/free alignment
### Why the PR?
I've got a local `miri` branch that's able to test `x86_64-fortanix-unknown-sgx`, so I can get better assurance about our enclaves. It's now complaining about a bunch of stuff in std :sweat_smile:
### Context
1. `x86_64-fortanix-unknown-sgx` enclaves can request the untrusted host enclave runner to allocate/free memory in userspace and get a pointer to it in return.
2. There's a userspace/enclave space memory split for `x86_64-fortanix-unknown-sgx` enclaves. It's a bit like the userspace/kernel space split, where the kernel doesn't trust pointers from userspace and is very paranoid about copying data to/from userspace.
### Problem
In the enclave, `User::new_uninit_bytes` and `User::drop` are requesting the host to alloc/dealloc memory with potentially mismatched alignment, as the enclave side is unconditionally over-aligning on allocation but not doing the same on free.
- Ex: `User::<ByteBuffer>` -> `alloc(_, align=8)` -> `drop()` -> `free(_, align=1)`
See: <https://github.com/rust-lang/rust/blob/main/library/std/src/sys/pal/sgx/abi/usercalls/alloc.rs>
```rust
// Enclave-side
impl<T: ?Sized> User<T>
where
T: UserSafe,
{
// This function returns memory that is practically uninitialized, but is
// not considered "unspecified" or "undefined" for purposes of an
// optimizing compiler. This is achieved by returning a pointer from
// from outside as obtained by `super::alloc`.
fn new_uninit_bytes(size: usize) -> Self {
unsafe {
// Mustn't call alloc with size 0.
let ptr = if size > 0 {
// `copy_to_userspace` is more efficient when data is 8-byte aligned
let alignment = cmp::max(T::align_of(), 8); // <------------------------- HERE
rtunwrap!(Ok, super::alloc(size, alignment)) as _
} else {
T::align_of() as _ // dangling pointer ok for size 0
};
if let Ok(v) = crate::panic::catch_unwind(|| T::from_raw_sized(ptr, size)) {
User(NonNull::new_userref(v))
} else {
rtabort!("Got invalid pointer from alloc() usercall")
}
}
}
// ...
}
// ...
impl<T: ?Sized> Drop for User<T>
where
T: UserSafe,
{
fn drop(&mut self) {
unsafe {
let ptr = (*self.0.as_ptr()).0.get();
// vvvvvvvvvvvvv------------------ HERE
super::free(ptr as _, size_of_val(&mut *ptr), T::align_of());
}
}
}
```
This min. alignment optimization was introduced in rust-lang@6f7d193. See below for more details on why.
The two usercalls, `super::alloc` and `super::free`, are eventually handled by the host runner. They just delegate to the `System` allocator:
See: <https://github.com/fortanix/rust-sgx/blob/master/intel-sgx/enclave-runner-sgx/src/usercalls/mod.rs>
```rust
// Host-side / userspace
impl<'tcs> IOHandlerInput<'tcs> {
// ...
#[inline(always)]
fn alloc(&self, size: usize, alignment: usize) -> IoResult<*mut u8> {
unsafe {
// vvvvvvvvv--------------- UNCHANGED
let layout = Layout::from_size_align(size, alignment)
.map_err(|_| IoErrorKind::InvalidInput)?;
if layout.size() == 0 {
return Err(IoErrorKind::InvalidInput.into());
}
let ptr = System.alloc(layout);
if ptr.is_null() {
Err(IoErrorKind::Other.into())
} else {
Ok(ptr)
}
}
}
#[inline(always)]
fn free(&self, ptr: *mut u8, size: usize, alignment: usize) -> IoResult<()> {
unsafe {
// vvvvvvvvv--------------- UNCHANGED
let layout = Layout::from_size_align(size, alignment)
.map_err(|_| IoErrorKind::InvalidInput)?;
if size == 0 {
return Ok(());
}
Ok(System.dealloc(ptr, layout))
}
}
// ...
}
```
It also appears that `enclave-runner-sgx` assumes that there's no `#[global_allocator]` override (<https://github.com/fortanix/rust-sgx/blob/master/intel-sgx/enclave-runner-sgx/src/usercalls/interface.rs#L333>).
For most enclave hosts running stock x86_64-unknown-linux-gnu (glibc malloc), I don't believe this mismatch is currently an issue, since posix `free` ignores the alignment anyway.
If you did swap in jemalloc, which does care about the dealloc alignment, then something would definitely go wrong elsewhere, as the you'd have mismatched allocators (`System` above vs `Box<_>`/`Vec<_>` using `Global`).
### Solutions
It's not clear that we can round-up the alignment on `free`, since `User::from_raw` exists, and there's various places that call it outside std.
We should probably just remove the in-enclave min. alignment until we come up with a more satisfactory solution. My guess is that the right place to do the min. alignment optimization is on enclave-runner-sgx side: <https://github.com/fortanix/rust-sgx/blob/master/intel-sgx/enclave-runner-sgx/src/usercalls/mod.rs#L1596> and other places that hand memory to the SGX enclave.
### Why over-align in the first place?
The min. alignment exists for performance reasons (see: `copy_from_userspace`). It's highly preferable if all memory copied from userspace is at least 8 byte aligned, otherwise we have to fallback to a super slow copy routine for the unaligned prefix (and suffix).1 file changed
Lines changed: 2 additions & 4 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
253 | 253 | | |
254 | 254 | | |
255 | 255 | | |
256 | | - | |
257 | | - | |
258 | | - | |
| 256 | + | |
259 | 257 | | |
260 | | - | |
| 258 | + | |
261 | 259 | | |
262 | 260 | | |
263 | 261 | | |
| |||
0 commit comments