std::sys::pal::sgx: fix mismatched alloc/free alignment

[phlip9]

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 😅

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

// 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 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

// 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).

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @JohnTitor (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from ChrisDenton, JohnTitor, Mark-Simulacrum, clarfonthey, nia-e

[phlip9]

cc @jethrogb @raoulstrackx

[tvsfx]

Thanks for the detailed report! I can't approve this, but FWIW the fix looks fine to me.

Naively fixing alignment in drop would indeed run into issues with from_raw (and from_raw_parts) . Additionally, there's also into_raw which has the dual issue. For example, it is used here in a batched, similarly non-aligned version of free. We could add an alignment requirement to from_raw and friends but we cannot increase alignment of User with something like #[repr(align(8))], because we support less aligned data.

I think making the runner responsible for this optimization is a good idea, since it's not security-critical.

[raoulstrackx]

Thanks for the PR @phlip9 and the additional review @tvsfx. I also approve, but also lack write access.

[JohnTitor]

Sorry for the delay! @bors r+

View changes since this review

[rust-bors]

📌 Commit 3e5c0fa has been approved by JohnTitor

It is now in the queue for this repository.

[GuillaumeGomez]

Failed in #162300 (comment).

@bors r-

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#162300), which was unapproved.

View changes since this unapproval

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[phlip9]

Ugh, sorry bout that. My bootstrap.toml was messed up it seems... Should be fixed now.

[JohnTitor]

@bors r+

[rust-bors]

📌 Commit 14ac84f has been approved by JohnTitor

It is now in the queue for this repository.