Fix soundness issues in std::sys::pal::sgx::waitqueue::unsafe_list

[jethrogb]

View all comments

• I did not use an LLM to create a change in this PR.
• I used an LLM to create a change in this PR, and I have explained below how it was used.

Replace invalid uses of references in std::sys::pal::sgx::waitqueue::unsafe_list internals with raw pointers. I tried to keep the code structure the same as much as possible. In addition to the use of references flagged in the original issue, it turns out the head/tail (raw) pointer stored in the linked list caused provenance issues in miri. Switched to using UnsafePinned for that.

PR organization:

• Commit 1: Main soundness fix.
• Commit 2: Use pinning in the pub(crate) API for UnsafeList. This code predates pinning in Rust. I believe this change isn't strictly necessary as I believe it's valid to document pinning requirements in the unsafe methods on UnsafeList. However, I felt it's better to be explicit about this now that pinning is available in the language.
• Commit 3: Move UnsafeList to a platform-agnostic location so miri can be run on the test suite. This also adds some tests.

This PR was developed with Claude Fable 5 through extensive interactive use, where I directed a detailed plan for making the changes needed for this fix. My input includes keeping the structure the same and the new internal abstraction for dealing with raw pointers. I'm not familiar with miri, I used Claude to test the changes with miri. It said the test suite was failing before the changes (both stacked borrows and tree borrows) but passing after. The additional tests developed this way have been added in the third commit. The head/tail pointer provenance issue was found with Claude. I thoroughly reviewed all the code, including comments, and made manual changes/deletions where necessary/appropriate. The PR description was written by me.

r? @nia-e

Fixes #114581
Fixes #160603
Fixes #161060
Supersedes #160641

[ds84182]

What are the advantages of this PR over #161060?

Keeping the same code structure is not an advantage as the original was already overcomplicated for what should have been a simple linked list. The "dummy node" trick comes with all of this additional baggage around pinning the list and has zero upsides. The dummy node also comes with an extra vestigial T wrapped in an Option, which then needs to be unwrapped in push and pop.

[nia-e]

notably, this factors out a lot of complexity into an sgx-independent module which can thus be thoroughly triaged with miri - which is what I intend to make sure happens here. I haven't gotten a chance to look into this too thoroughly yet - it's going to be a big thing to review and thus will need me to sit down for a long afternoon or so - but keeping the existing code in use which has been somewhat battle-tested is at least a slight upside.

[jethrogb]

I think we can all agree that this code is both tricky and very important to get right: it underpins all synchronization primitives in the standard library for the target that uses it. Any problems with synchronization can lead to hard to debug and catastrophic failures of code that relies on it.

As such, we need to be conservative with making changes. The advantages of keeping the existing construction compared to changing it at all:

• The target maintainers have thoroughly reviewed and fully understand the current construction
• As @nia-e says, the current construction has been battle-tested. This code is backing large scale software deployments that have been running for years without any apparent synchronization issues.
  • I used an LLM to compare the codegen of mainline vs commit 1 and (besides the clear changes related to initialization) it concluded the following: the theoretical cost (&mut UnsafeList/&mut WaitVariable losing noalias) produced no visible extra reloads or reordering barriers, and the link-manipulation cores are instruction-for-instruction equivalent. This also strengthens my belief that while the existing code was unsound it hasn't lead to miscompilation.

The advantages of the current construction vs. the specific alternative in #161060:

• The existing construction has a better encapsulation of concerns, where the users of UnsafeList don't have to deal with raw pointers.

[nia-e]

I do think in the long run there is value to switching over to a "nicer" implementation, if only for maintainability and to reduce the bus factor of the SGX target. I also think it's definitely lower priority than fixing current soundness bugs, and indeed factoring out the unsafe code so it can run on Miri is a useful first step toward allowing us to thoroughly test the other suggested impl as well ^^

[jethrogb]

@joboet mentioned he was working on a more generic queue replacement for several targets. I think that would be a better time for introducing any major changes - it certainly helps with the bus factor.

[nia-e]

I spent quite some time going over this and attempting to break the code or otherwise trigger UB in Miri, and I'm happy to say I haven't managed to. I also threw Codex at it and the best it was able to come up with was an unsoundness if ~ten successive RNG failures occur and the subsequent panic is resumed from; I'm going to say that's the one blocking thing that should be addressed, but it's a small fix (see my comment).

The code itself looks fine and appears to mostly carry over the old code, which is a bit messy but will be superseded eventually by a refactor à la #161060, so nonblocking imo. The tests are a nice addition ^^ r=me once my comment is resolved & that call is properly caught.

@bors delegate+

View changes since this review

[nia-e]

This can unwind on RNG exhaustion, so should be part of the catch_unwind block above or otherwise caught.

[jethrogb]

@nia-e do you have a stack trace for the unwind?

[jethrogb]

I'm guessing it's from https://github.com/rust-lang/rust/blob/main/library/std/src/sys/pal/sgx/abi/usercalls/mod.rs#L180. This should be addressed by making sure rtabort! is done instead of panic! (or maybe it's actually non-fatal in this case?). I'm not sure if that's appropriate for all uses of random() though so it may need some more thought.

[nia-e]

I've got the Miri backtrace ^^ manually forcing RNG to fail gave:

stack backtrace:
  0: std::panicking::panic_handler
  1: core::panicking::panic_fmt
  2: sys::sync::unsafe_list::tests::link_then_unwind
  3: ...miri_post_link_unwind_backtrace::{closure#0}
  4: FnOnce::call_once
  5: AssertUnwindSafe<...>::call_once
  6: panicking::catch_unwind::do_call
  7: panicking::catch_unwind
  8: panic::catch_unwind
  9: ...miri_post_link_unwind_backtrace

which suggests the callstack:

std::sys::random::sgx::fail
std::sys::random::sgx::rdrand64
std::sys::random::sgx::fill_bytes
<SystemRng as Rng>::fill_bytes
<RangeFull as Distribution<i64>>::sample
std::random::random::<i64>
sgx::abi::usercalls::wait
sgx::abi::usercalls::wait_timeout::wait_checked
sgx::abi::usercalls::wait_timeout
sgx::waitqueue::WaitQueue::wait_timeout
sgx::Condvar::wait_timeout
std::sync::Condvar::wait_timeout

I'm quite impressed this was able to trigger UB, but apparently it is. This is a minified form of the example Codex came up with for running in Miri (again, assuming rng always fails):

#[inline(never)]
fn link_then_unwind(list: Pin<&mut UnsafeList<u32>>) {
    let mut entry = UnsafeListEntry::new(1234);
    unsafe { list.push(&mut entry) };
    panic!();
}

#[test]
fn miri_post_link_unwind_backtrace() {
    let mut list = new_list();
    let unwind = catch_unwind(AssertUnwindSafe(|| link_then_unwind(list.as_mut())));
    assert!(unwind.is_err());

    let _ = unsafe { list.as_mut().pop() };
}

[jethrogb]

I'm quite impressed this was able to trigger UB, but apparently it is.

It's a trivial “use-after-free” of stack memory. See the safety invariant on push which calls this out.

[rust-bors]

✌️ @jethrogb, you can now approve this pull request!

If @nia-e told you to "r=me" after making some further change, then please make that change and post @bors r=nia-e.

View changes since this delegation.

[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.

[jethrogb]

Pushed two commits to deal with the randomness generation failure issue. Will run the SGX test suite before r+ing.

[jethrogb]

@raoulstrackx or @tvsfx could you review the two most recent commits?

[jethrogb]

SGX test suite passed on commit 998113d (job rustc-rust-sgx-ci no. 491)

[tvsfx]

@raoulstrackx or @tvsfx could you review the two most recent commits?

Those 2 commits LGTM

[jethrogb]

@bors r=nia-e

[rust-bors]

📌 Commit 998113d has been approved by nia-e

It is now in the queue for this repository.

[rust-bors]

💔 Test for 9290cd7 failed: CI. Failed job:

• auto - dist-aarch64-linux (web logs, enhanced plaintext logs)

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[JonathanBrouwer]

@bors retry