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

library/std/src/sys/pal/sgx/abi/usercalls/mod.rs

@@ -1,6 +1,6 @@
+use crate::arch::x86_64::_rdrand64_step;
 use crate::cmp;
 use crate::io::{self, BorrowedCursor, IoSlice, IoSliceMut};
-use crate::random::random;
 use crate::time::{Duration, Instant};
 
 pub(crate) mod alloc;
@@ -167,6 +167,12 @@ pub fn exit(panic: bool) -> ! {
 /// Usercall `wait`. See the ABI documentation for more information.
 #[unstable(feature = "sgx_platform", issue = "56975")]
 pub fn wait(event_mask: u64, mut timeout: u64) -> io::Result<u64> {
+    fn try_rdrand() -> Option<u64> {
+        let mut val: u64 = 0;
+        // SAFETY: the rdrand feature is enabled on SGX targets
+        if unsafe { _rdrand64_step(&mut val) } == 1 { Some(val) } else { None }
+    }
+
     if timeout != WAIT_NO && timeout != WAIT_INDEFINITE {
         // We don't want people to rely on accuracy of timeouts to make
         // security decisions in an SGX enclave. That's why we add a random
@@ -175,9 +181,14 @@ pub fn wait(event_mask: u64, mut timeout: u64) -> io::Result<u64> {
         // to make things work in other cases. Note that in the SGX threat
         // model the enclave runner which is serving the wait usercall is not
         // trusted to ensure accurate timeouts.
+        //
+        // Since the random timeout is only intended as defense-in-depth
+        // protection at development/testing time, it's ok to continue if
+        // randomness generation fails.
         if let Ok(timeout_signed) = i64::try_from(timeout) {
             let tenth = timeout_signed / 10;
-            let deviation = random::<i64>(..).checked_rem(tenth).unwrap_or(0);
+            let deviation =
+                try_rdrand().and_then(|rnd| (rnd as i64).checked_rem(tenth)).unwrap_or(0);
             timeout = timeout_signed.saturating_add(deviation) as _;
         }
     }

library/std/src/sys/pal/sgx/waitqueue/mod.rs

@@ -14,16 +14,16 @@
 mod tests;
 
 mod spin_mutex;
-mod unsafe_list;
 
 use fortanix_sgx_abi::{EV_UNPARK, Tcs, WAIT_INDEFINITE};
 
-pub use self::spin_mutex::{SpinMutex, SpinMutexGuard, try_lock_or_false};
-use self::unsafe_list::{UnsafeList, UnsafeListEntry};
+pub use self::spin_mutex::{SpinMutex, SpinMutexGuard};
 use super::abi::{thread, usercalls};
 use crate::num::NonZero;
 use crate::ops::{Deref, DerefMut};
 use crate::panic::{self, AssertUnwindSafe};
+use crate::pin::Pin;
+use crate::sys::sync::unsafe_list::{UnsafeList, UnsafeListEntry};
 use crate::time::Duration;
 
 /// An queue entry in a `WaitQueue`.
@@ -38,24 +38,41 @@ struct WaitEntry {
 /// queue and the data are synchronized, since the type itself is not `Sync`.
 ///
 /// Consumers of this API should use a synchronization primitive for shared
-/// access, such as `SpinMutex`.
-#[derive(Default)]
+/// access. `WaitVariable::new` is the only constructor and provides that
+/// with `SpinMutex`.
 pub struct WaitVariable<T> {
     queue: WaitQueue,
     lock: T,
 }
 
 impl<T> WaitVariable<T> {
-    pub const fn new(var: T) -> Self {
-        WaitVariable { queue: WaitQueue::new(), lock: var }
-    }
-
     pub fn lock_var(&self) -> &T {
         &self.lock
     }
 
-    pub fn lock_var_mut(&mut self) -> &mut T {
-        &mut self.lock
+    pub fn lock_var_mut(self: Pin<&mut Self>) -> &mut T {
+        // SAFETY: `lock` is not structurally pinned: a pinned `WaitVariable`
+        // makes no promise that `T` is pinned.
+        unsafe { &mut self.get_unchecked_mut().lock }
+    }
+
+    fn queue(self: Pin<&mut Self>) -> Pin<&mut WaitQueue> {
+        // SAFETY: `queue` is structurally pinned: a pinned `WaitVariable`
+        // pins it, and it is never moved out of it.
+        unsafe { self.map_unchecked_mut(|this| &mut this.queue) }
+    }
+
+    /// Creates a mutex-protected `WaitVariable` on the heap, with its queue's
+    /// list initialized. Initialization makes the list self-referential and
+    /// happens before pinning: only the `Box` pointer is moved into the
+    /// `Pin`, the heap allocation itself never moves.
+    pub fn new(value: T) -> Pin<Box<SpinMutex<WaitVariable<T>>>> {
+        // SAFETY: `init` is called below, before the queue is otherwise used
+        // or dropped.
+        let queue = unsafe { WaitQueue::new() };
+        let result = Box::new(SpinMutex::new(WaitVariable { queue, lock: value }));
+        result.lock().queue.inner.init();
+        Box::into_pin(result)
     }
 }
 
@@ -68,7 +85,7 @@ pub enum NotifiedTcs {
 /// An RAII guard that will notify a set of target threads as well as unlock
 /// a mutex on drop.
 pub struct WaitGuard<'a, T: 'a> {
-    mutex_guard: Option<SpinMutexGuard<'a, WaitVariable<T>>>,
+    mutex_guard: Option<Pin<SpinMutexGuard<'a, WaitVariable<T>>>>,
     notified_tcs: NotifiedTcs,
 }
 
@@ -79,21 +96,36 @@ pub struct WaitGuard<'a, T: 'a> {
 /// safe because the waiting thread will not return from that stack frame until
 /// after it is notified. The notifying thread ensures to clean up any
 /// references to the list entries before sending the wakeup event.
+// The safety requirements of `UnsafeList` are upheld as follows:
+//
+// * All list operations are performed while holding the lock of the
+//   `SpinMutex` around the `WaitVariable` containing the list.
+// * A waiting thread pushes a stack-allocated entry and does not invalidate
+//   it while it is in the list: it only accesses the entry through the
+//   reference `push` returned, reading `wake` under the `WaitEntry`'s own
+//   `SpinMutex`.
+// * `push` -> `pop`: a notifying thread pops the entry and sets `wake` under
+//   the `WaitEntry`'s `SpinMutex`; when that mutex is released, the thread
+//   will no longer access the entry (guaranteed by the mutex guard). The
+//   waiting thread only returns from the stack frame containing the entry
+//   once it observes `wake == true` under that same mutex, so the entry is
+//   only deallocated after the notifying thread's last access to it.
+// * `push` -> `remove`: on a timeout, `wait_timeout` re-acquires the queue
+//   lock and checks `wake`: the entry is still in the list if and only if
+//   `wake` is not set, because notifying threads always `pop` an entry
+//   before setting its `wake`. Only if the entry is still in the list is it
+//   removed.
+// * Besides as described, no other exclusive references to the entry are
+//   taken.
 pub struct WaitQueue {
     // We use an inner Mutex here to protect the data in the face of spurious
     // wakeups.
     inner: UnsafeList<SpinMutex<WaitEntry>>,
 }
 unsafe impl Send for WaitQueue {}
 
-impl Default for WaitQueue {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
 impl<'a, T> Deref for WaitGuard<'a, T> {
-    type Target = SpinMutexGuard<'a, WaitVariable<T>>;
+    type Target = Pin<SpinMutexGuard<'a, WaitVariable<T>>>;
 
     fn deref(&self) -> &Self::Target {
         self.mutex_guard.as_ref().unwrap()
@@ -118,23 +150,42 @@ impl<'a, T> Drop for WaitGuard<'a, T> {
 }
 
 impl WaitQueue {
-    pub const fn new() -> Self {
-        WaitQueue { inner: UnsafeList::new() }
+    /// Creates a new queue.
+    ///
+    /// # Safety
+    ///
+    /// The caller must initialize the queue's list (`UnsafeList::init`)
+    /// before any other use of the queue, including dropping it.
+    /// `WaitVariable::new`, the sole constructor of the containing
+    /// structure, does this.
+    pub const unsafe fn new() -> Self {
+        // SAFETY: the caller upholds `UnsafeList::new`'s contract (see this
+        // function's safety requirements).
+        WaitQueue { inner: unsafe { UnsafeList::new() } }
+    }
+
+    fn inner(self: Pin<&mut Self>) -> Pin<&mut UnsafeList<SpinMutex<WaitEntry>>> {
+        // SAFETY: `inner` is structurally pinned: a pinned `WaitQueue` pins
+        // it, and it is never moved out of it.
+        unsafe { self.map_unchecked_mut(|this| &mut this.inner) }
     }
 
     /// Adds the calling thread to the `WaitVariable`'s wait queue, then wait
     /// until a wakeup event.
     ///
     /// This function does not return until this thread has been awoken. When `before_wait` panics,
     /// this function will abort.
-    pub fn wait<T, F: FnOnce()>(mut guard: SpinMutexGuard<'_, WaitVariable<T>>, before_wait: F) {
+    pub fn wait<T, F: FnOnce()>(
+        mut guard: Pin<SpinMutexGuard<'_, WaitVariable<T>>>,
+        before_wait: F,
+    ) {
         // very unsafe: check requirements of UnsafeList::push
         unsafe {
             let mut entry = UnsafeListEntry::new(SpinMutex::new(WaitEntry {
                 tcs: thread::current(),
                 wake: false,
             }));
-            let entry = guard.queue.inner.push(&mut entry);
+            let entry = guard.as_mut().queue().inner().push(&mut entry);
             drop(guard);
             if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) {
                 rtabort!("Panic before wait on wakeup event")
@@ -155,7 +206,7 @@ impl WaitQueue {
     /// If not, it will remove the calling thread from the wait queue.
     /// When `before_wait` panics, this function will abort.
     pub fn wait_timeout<T, F: FnOnce()>(
-        lock: &SpinMutex<WaitVariable<T>>,
+        lock: Pin<&SpinMutex<WaitVariable<T>>>,
         timeout: Duration,
         before_wait: F,
     ) -> bool {
@@ -165,19 +216,19 @@ impl WaitQueue {
                 tcs: thread::current(),
                 wake: false,
             }));
-            let entry_lock = lock.lock().queue.inner.push(&mut entry);
+            let entry_lock = lock.lock_pinned().as_mut().queue().inner().push(&mut entry);
             if let Err(_e) = panic::catch_unwind(AssertUnwindSafe(|| before_wait())) {
                 rtabort!("Panic before wait on wakeup event or timeout")
             }
             usercalls::wait_timeout(EV_UNPARK, timeout, || entry_lock.lock().wake);
             // acquire the wait queue's lock first to avoid deadlock
             // and ensure no other function can simultaneously access the list
             // (e.g., `notify_one` or `notify_all`)
-            let mut guard = lock.lock();
+            let mut guard = lock.lock_pinned();
             let success = entry_lock.lock().wake;
             if !success {
                 // nobody is waking us up, so remove our entry from the wait queue.
-                guard.queue.inner.remove(&mut entry);
+                guard.as_mut().queue().inner().remove(&mut entry);
             }
             success
         }
@@ -189,14 +240,14 @@ impl WaitQueue {
     /// If a waiter is found, a `WaitGuard` is returned which will notify the
     /// waiter when it is dropped.
     pub fn notify_one<T>(
-        mut guard: SpinMutexGuard<'_, WaitVariable<T>>,
-    ) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
+        mut guard: Pin<SpinMutexGuard<'_, WaitVariable<T>>>,
+    ) -> Result<WaitGuard<'_, T>, Pin<SpinMutexGuard<'_, WaitVariable<T>>>> {
         // SAFETY: lifetime of the pop() return value is limited to the map
         // closure (The closure return value is 'static). The underlying
         // stack frame won't be freed until after the lock on the queue is released
         // (i.e., `guard` is dropped).
         unsafe {
-            let tcs = guard.queue.inner.pop().map(|entry| -> Tcs {
+            let tcs = guard.as_mut().queue().inner().pop().map(|entry| -> Tcs {
                 let mut entry_guard = entry.lock();
                 entry_guard.wake = true;
                 entry_guard.tcs
@@ -216,14 +267,14 @@ impl WaitQueue {
     /// If at least one waiter is found, a `WaitGuard` is returned which will
     /// notify all waiters when it is dropped.
     pub fn notify_all<T>(
-        mut guard: SpinMutexGuard<'_, WaitVariable<T>>,
-    ) -> Result<WaitGuard<'_, T>, SpinMutexGuard<'_, WaitVariable<T>>> {
+        mut guard: Pin<SpinMutexGuard<'_, WaitVariable<T>>>,
+    ) -> Result<WaitGuard<'_, T>, Pin<SpinMutexGuard<'_, WaitVariable<T>>>> {
         // SAFETY: lifetime of the pop() return values are limited to the
         // while loop body. The underlying stack frames won't be freed until
         // after the lock on the queue is released (i.e., `guard` is dropped).
         unsafe {
             let mut count = 0;
-            while let Some(entry) = guard.queue.inner.pop() {
+            while let Some(entry) = guard.as_mut().queue().inner().pop() {
                 count += 1;
                 let mut entry_guard = entry.lock();
                 entry_guard.wake = true;

library/std/src/sys/pal/sgx/waitqueue/spin_mutex.rs

@@ -7,6 +7,7 @@ mod tests;
 use crate::cell::UnsafeCell;
 use crate::hint;
 use crate::ops::{Deref, DerefMut};
+use crate::pin::Pin;
 use crate::sync::atomic::{Atomic, AtomicBool, Ordering};
 
 #[derive(Default)]
@@ -52,11 +53,19 @@ impl<T> SpinMutex<T> {
             None
         }
     }
-}
 
-/// Lock the Mutex or return false.
-pub macro try_lock_or_false($e:expr) {
-    if let Some(v) = $e.try_lock() { v } else { return false }
+    #[inline(always)]
+    pub fn lock_pinned(self: Pin<&Self>) -> Pin<SpinMutexGuard<'_, T>> {
+        // SAFETY: `value` is structurally pinned: a pinned mutex pins its
+        // contents, and `SpinMutexGuard` never moves the value.
+        unsafe { Pin::new_unchecked(self.get_ref().lock()) }
+    }
+
+    #[inline(always)]
+    pub fn try_lock_pinned(self: Pin<&Self>) -> Option<Pin<SpinMutexGuard<'_, T>>> {
+        // SAFETY: see `lock_pinned`
+        self.get_ref().try_lock().map(|guard| unsafe { Pin::new_unchecked(guard) })
+    }
 }
 
 impl<'a, T> Deref for SpinMutexGuard<'a, T> {

library/std/src/sys/pal/sgx/waitqueue/tests.rs

@@ -4,14 +4,14 @@ use crate::thread;
 
 #[test]
 fn queue() {
-    let wq = Arc::new(SpinMutex::<WaitVariable<()>>::default());
+    let wq = Arc::new(WaitVariable::new(()));
     let wq2 = wq.clone();
 
-    let locked = wq.lock();
+    let locked = (*wq).as_ref().lock_pinned();
 
     let t1 = thread::spawn(move || {
         // if we obtain the lock, the main thread should be waiting
-        assert!(WaitQueue::notify_one(wq2.lock()).is_ok());
+        assert!(WaitQueue::notify_one((*wq2).as_ref().lock_pinned()).is_ok());
     });
 
     WaitQueue::wait(locked, || {});