From 2394fe3416f78504fae0fddff51c0ea5464660a7 Mon Sep 17 00:00:00 2001 From: Vasili Novikov Date: Mon, 16 Oct 2023 15:03:28 +0200 Subject: [PATCH] Async usercalls code review --- .travis.yml | 2 +- intel-sgx/async-usercalls/.cargo/config.toml | 2 -- intel-sgx/async-usercalls/src/io_bufs.rs | 3 +++ intel-sgx/async-usercalls/src/lib.rs | 3 +++ intel-sgx/async-usercalls/src/provider_api.rs | 15 ++++++++++++++- intel-sgx/async-usercalls/src/queues.rs | 17 ++++++++++++----- intel-sgx/async-usercalls/src/utils.rs | 2 ++ ipc-queue/src/position.rs | 10 ++-------- 8 files changed, 37 insertions(+), 17 deletions(-) delete mode 100644 intel-sgx/async-usercalls/.cargo/config.toml diff --git a/.travis.yml b/.travis.yml index d35c2b33..fa9de5b0 100644 --- a/.travis.yml +++ b/.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 --git a/intel-sgx/async-usercalls/.cargo/config.toml b/intel-sgx/async-usercalls/.cargo/config.toml deleted file mode 100644 index 72899009..00000000 --- a/intel-sgx/async-usercalls/.cargo/config.toml +++ /dev/null @@ -1,2 +0,0 @@ -[alias] -ci-test = "test --verbose --locked -p async-usercalls --target x86_64-fortanix-unknown-sgx --no-run" diff --git a/intel-sgx/async-usercalls/src/io_bufs.rs b/intel-sgx/async-usercalls/src/io_bufs.rs index e039aef9..e10bd810 100644 --- a/intel-sgx/async-usercalls/src/io_bufs.rs +++ b/intel-sgx/async-usercalls/src/io_bufs.rs @@ -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 --git a/intel-sgx/async-usercalls/src/lib.rs b/intel-sgx/async-usercalls/src/lib.rs index 1025eed8..7dfe38ff 100644 --- a/intel-sgx/async-usercalls/src/lib.rs +++ b/intel-sgx/async-usercalls/src/lib.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 --git a/intel-sgx/async-usercalls/src/provider_api.rs b/intel-sgx/async-usercalls/src/provider_api.rs index 7faf733d..304c4101 100644 --- a/intel-sgx/async-usercalls/src/provider_api.rs +++ b/intel-sgx/async-usercalls/src/provider_api.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| { + // Passing a `User` 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) + 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::::uninitialized())); @@ -101,7 +106,12 @@ impl AsyncUsercallProvider { let local_addr_ptr = local_addr_buf.as_raw_mut_ptr(); let cb = move |res: io::Result| { + // `_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, 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) -> Vec { unsafe { let buf = buf.to_enclave(); diff --git a/intel-sgx/async-usercalls/src/queues.rs b/intel-sgx/async-usercalls/src/queues.rs index 9cfadcf3..a0ded0c4 100644 --- a/intel-sgx/async-usercalls/src/queues.rs +++ b/intel-sgx/async-usercalls/src/queues.rs @@ -72,6 +72,7 @@ fn init_async_queues() -> io::Result<(Sender, Sender, 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 --git a/intel-sgx/async-usercalls/src/utils.rs b/intel-sgx/async-usercalls/src/utils.rs index 78f3c051..f31e6acc 100644 --- a/intel-sgx/async-usercalls/src/utils.rs +++ b/intel-sgx/async-usercalls/src/utils.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); impl MakeSend { diff --git a/ipc-queue/src/position.rs b/ipc-queue/src/position.rs index fa37e622..a986ed1b 100644 --- a/ipc-queue/src/position.rs +++ b/ipc-queue/src/position.rs @@ -41,10 +41,7 @@ impl PositionMonitor { 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 PositionMonitor { 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)) } }