Async usercalls code review

Author: Vasili Novikov

Diff for: .travis.yml

@@ -45,7 +45,7 @@ matrix:
         - rustup target add x86_64-fortanix-unknown-sgx --toolchain nightly
       script:
         - cargo test --verbose --locked --all --exclude sgxs-loaders --exclude async-usercalls && [ "$(echo $(nm -D target/debug/sgx-detect|grep __vdso_sgx_enter_enclave))" = "w __vdso_sgx_enter_enclave" ]
-        - CARGO_BUILD_RUSTFLAGS='-D warnings' cargo ci-test
+        - cargo test --verbose --locked -p async-usercalls --target x86_64-fortanix-unknown-sgx --no-run
         - cargo test --verbose --locked -p dcap-ql --features link
         - cargo test --verbose --locked -p dcap-ql --features verify
         - cargo test --verbose --locked -p ias --features mbedtls

Diff for: intel-sgx/async-usercalls/.cargo/config.toml

@@ -5,6 +5,9 @@ use std::ops::{Deref, DerefMut, Range};
 use std::os::fortanix_sgx::usercalls::alloc::{User, UserRef};
 use std::sync::Arc;
 
+/// Userspace buffer. Note that you have to be careful with reading data and writing data
+/// from the SGX enclave to/from userspace, to avoid stale memory data attacks.
+/// Using `UserRef<[u8]>` to access the data is a safe choice, as it handles the nuances internally.
 pub struct UserBuf(UserBufKind);
 
 enum UserBufKind {

Diff for: intel-sgx/async-usercalls/src/io_bufs.rs

@@ -1,3 +1,6 @@
+// deny compilation warnings for the whole `async-usercalls` module
+#![deny(warnings)]
+
 //! This crate provides an interface for performing asynchronous usercalls in
 //! SGX enclaves. The motivation behind asynchronous usercalls and ABI
 //! documentation can be found

Diff for: intel-sgx/async-usercalls/src/lib.rs

@@ -28,6 +28,9 @@ impl AsyncUsercallProvider {
         let ptr = read_buf.as_mut_ptr();
         let len = read_buf.len();
         let cb = move |res: io::Result<usize>| {
+            // Passing a `User<u8>` likely leads to the value being dropped implicitly,
+            // which will cause a synchronous `free` usercall.
+            // See: https://github.com/fortanix/rust-sgx/issues/531
             let read_buf = ManuallyDrop::into_inner(read_buf).into_inner();
             callback(res, read_buf);
         };
@@ -93,6 +96,8 @@ impl AsyncUsercallProvider {
         where
             F: FnOnce(io::Result<TcpListener>) + Send + 'static,
     {
+        // `User::<[u8]>::uninitialized` causes a synchronous usercall when dropped.
+        // See: https://github.com/fortanix/rust-sgx/issues/531
         let mut addr_buf = ManuallyDrop::new(MakeSend::new(User::<[u8]>::uninitialized(addr.len())));
         let mut local_addr_buf = ManuallyDrop::new(MakeSend::new(User::<ByteBuffer>::uninitialized()));
 
@@ -101,7 +106,12 @@ impl AsyncUsercallProvider {
         let local_addr_ptr = local_addr_buf.as_raw_mut_ptr();
 
         let cb = move |res: io::Result<Fd>| {
+            // `_addr_buf` is of type `MakeSend<_>`, which will lead to a synchronous usercall
+            // upon being dropped.
+            // See: https://github.com/fortanix/rust-sgx/issues/531
             let _addr_buf = ManuallyDrop::into_inner(addr_buf);
+            // Same as above, synchronous usercall will happen upon dropping
+            // See: https://github.com/fortanix/rust-sgx/issues/531
             let local_addr_buf = ManuallyDrop::into_inner(local_addr_buf);
 
             let local_addr = Some(string_from_bytebuffer(&local_addr_buf, "bind_stream", "local_addr"));
@@ -253,12 +263,15 @@ impl AsyncUsercallProvider {
     }
 }
 
+// The code is similar to `rust-lang/sys/sgx/abi/usercalls/mod.rs`,
+// see: https://github.com/fortanix/rust-sgx/issues/532
 fn string_from_bytebuffer(buf: &UserRef<ByteBuffer>, usercall: &str, arg: &str) -> String {
     String::from_utf8(copy_user_buffer(buf))
         .unwrap_or_else(|_| panic!("Usercall {}: expected {} to be valid UTF-8", usercall, arg))
 }
 
-// adapted from libstd sys/sgx/abi/usercalls/alloc.rs
+// The code is similar to `rust-lang/sys/sgx/abi/usercalls/mod.rs`,
+// see: https://github.com/fortanix/rust-sgx/issues/532
 fn copy_user_buffer(buf: &UserRef<ByteBuffer>) -> Vec<u8> {
     unsafe {
         let buf = buf.to_enclave();

Diff for: intel-sgx/async-usercalls/src/provider_api.rs

@@ -72,6 +72,7 @@ fn init_async_queues() -> io::Result<(Sender<Usercall>, Sender<Cancel>, Receiver
 
     // FIXME: once `WithId` is exported from `std::os::fortanix_sgx::usercalls::raw`, we can remove
     // `transmute` calls here and use FifoDescriptor/WithId from std everywhere including in ipc-queue.
+    // See: https://github.com/fortanix/rust-sgx/issues/533
     let utx = unsafe { Sender::from_descriptor(std::mem::transmute(usercall_queue), QueueSynchronizer { queue: Queue::Usercall }) };
     let ctx = unsafe { Sender::from_descriptor(std::mem::transmute(cancel_queue), QueueSynchronizer { queue: Queue::Cancel }) };
     let rx = unsafe { Receiver::from_descriptor(std::mem::transmute(return_queue), QueueSynchronizer { queue: Queue::Return }) };
@@ -175,18 +176,24 @@ mod map {
 
         /// Insert a new value and return the index.
         ///
-        /// If trying to insert more than `u32` keys, the insert operation will hang forever.
+        /// # Panics
+        /// Panics if `u32::MAX` entries are kept in the queue and a new one is attempted
+        /// to be inserted. Note that removing entries from the queue also frees up space
+        /// and the number of available entries.
         pub fn insert(&mut self, value: T) -> u32 {
+            let initial_id = self.next_id;
             loop {
                 let id = self.next_id;
-                // We intentionally ignore the overflow, thus allowing `next_id` to jump back to 0
+                // We intentionally ignore the overflow here, thus allowing `next_id` to jump back to 0
                 // after `u32::MAX` number of insertions.
                 self.next_id = self.next_id.overflowing_add(1).0;
-                if self.map.contains_key(&id) {
-                    continue
-                } else {
+                if !self.map.contains_key(&id) {
                     self.map.insert(id, value);
                     return id
+                } else if id == initial_id {
+                    panic!("Cannot keep more than {} entries into the async queue. Aborting.", u32::MAX)
+                } else {
+                    continue
                 }
             }
         }

Diff for: intel-sgx/async-usercalls/src/queues.rs

@@ -2,8 +2,10 @@ use std::ops::{Deref, DerefMut};
 use std::os::fortanix_sgx::usercalls::alloc::User;
 use std::os::fortanix_sgx::usercalls::raw::ByteBuffer;
 
+// This might be removed in the future, see: https://github.com/fortanix/rust-sgx/issues/530
 pub(crate) trait MakeSendMarker {}
 
+// This might be removed in the future, see: https://github.com/fortanix/rust-sgx/issues/530
 pub(crate) struct MakeSend<T: MakeSendMarker>(T);
 
 impl<T: MakeSendMarker> MakeSend<T> {

Diff for: intel-sgx/async-usercalls/src/utils.rs

@@ -41,10 +41,7 @@ impl<T> PositionMonitor<T> {
     pub fn read_position(&self) -> ReadPosition {
         let current = self.fifo.current_offsets(Ordering::Relaxed);
         let read_epoch = self.read_epoch.load(Ordering::Relaxed);
-        let read_epoch_shifted = read_epoch
-            .checked_shl(32)
-            .expect("Read epoch is >= 2^32 (2 to the power of 32). This is unsupported.");
-        ReadPosition(read_epoch_shifted | (current.read_offset() as u64))
+        ReadPosition((read_epoch << 32) | (current.read_offset() as u64))
     }
 
     pub fn write_position(&self) -> WritePosition {
@@ -58,10 +55,7 @@ impl<T> PositionMonitor<T> {
         if current.read_high_bit() != current.write_high_bit() {
             write_epoch += 1;
         }
-        let write_epoch_shifted = write_epoch
-            .checked_shl(32)
-            .expect("Write epoch is >= 2^32 (2 to the power of 32). This is unsupported.");
-        WritePosition(write_epoch_shifted | (current.write_offset() as u64))
+        WritePosition((write_epoch << 32) | (current.write_offset() as u64))
     }
 }