Merge #222

222: Add the Allowed type to libtock_platform. r=hudson-ayers a=jrvanwhy

Allowed is the type that will be returned by `Platform::allow()` in the successful case. It represents a memory buffer shared with the kernel. It uses raw pointers and volatile writes to perform read/write accesses to the memory to avoid encountering undefined behavior.

This is the first step towards fixing #129 and #143.

Co-authored-by: Johnathan Van Why <jrvanwhy@google.com>

core/platform/src/allows/allow_readable.rs

@@ -0,0 +1,18 @@
+/// Because the kernel may modify shared memory and place any bit pattern into
+/// that memory, we can only read types out of shared memory if every bit
+/// pattern is a valid value of that type. `AllowReadable` types are safe to
+/// read out of a shared buffer.
+pub unsafe trait AllowReadable {}
+
+unsafe impl AllowReadable for i8 {}
+unsafe impl AllowReadable for i16 {}
+unsafe impl AllowReadable for i32 {}
+unsafe impl AllowReadable for i64 {}
+unsafe impl AllowReadable for i128 {}
+unsafe impl AllowReadable for isize {}
+unsafe impl AllowReadable for u8 {}
+unsafe impl AllowReadable for u16 {}
+unsafe impl AllowReadable for u32 {}
+unsafe impl AllowReadable for u64 {}
+unsafe impl AllowReadable for u128 {}
+unsafe impl AllowReadable for usize {}

core/platform/src/allows/allowed.rs

@@ -0,0 +1,85 @@
+/// An individual value that has been shared with the kernel using the `allow`
+/// system call.
+// Allowed's implementation does not directly use the 'b lifetime. Platform uses
+// 'b to prevent the Allowed from accessing the buffer after the buffer becomes
+// invalid.
+// Allowed requires T to be Copy due to concerns about the semantics of
+// non-copyable types in shared memory as well as concerns about unexpected
+// behavior with Drop types. See the following PR discussion for more
+// information: https://github.com/tock/libtock-rs/pull/222
+pub struct Allowed<'b, T: Copy + 'b> {
+    // Safety properties:
+    //   1. `buffer` remains valid and usable for the lifetime of this Allowed
+    //      instance.
+    //   2. Read and write accesses of `buffer`'s pointee must be performed as a
+    //      volatile operation, as the kernel may mutate buffer's pointee at any
+    //      time.
+    //   3. The value `buffer` points to may have an arbitrary bit pattern in
+    //      it, so reading from `buffer` is only safe if the type contained
+    //      within is AllowReadable.
+    buffer: core::ptr::NonNull<T>,
+
+    // We need to use the 'b lifetime in Allowed to prevent an "unused lifetime"
+    // compiler error. We use it here. Note that the phantom data must be an
+    // &mut rather than a shared reference in order to make Allowed invariant
+    // with respect to T. Invariance is required because Allowed allows us to
+    // mutate the value in buffer, and invariance is the required property to do
+    // so without allowing callers to produce dangling references. This is
+    // documented at https://doc.rust-lang.org/nomicon/subtyping.html.
+    _phantom: core::marker::PhantomData<&'b mut T>,
+}
+
+// Allowed's API is based on that of core::cell::Cell, but removes some methods
+// that are not safe for use with shared memory.
+//
+// Internally, Allowed performs accesses to the shared memory using volatile
+// reads and writes through raw pointers. We avoid constructing references to
+// shared memory because that leads to undefined behavior (there is some
+// background on this in the following discussion:
+// https://github.com/rust-lang/unsafe-code-guidelines/issues/33). Tock runs on
+// single-threaded platforms, some of which lack atomic instructions, so we only
+// need to be able to deconflict races between the kernel (which will never
+// interrupt an instruction's execution) and this process. Therefore volatile
+// accesses are sufficient to deconflict races.
+impl<'b, T: Copy + 'b> Allowed<'b, T> {
+    // Allowed can only be constructed by the Platform. It is constructed after
+    // the `allow` system call, and as such must accept a raw pointer rather
+    // than a reference. The caller must make sure the following are true:
+    // 1. buffer points to a valid instance of type T
+    // 2. There are no references to buffer's pointee
+    // 3. buffer remains usable until the Allowed's lifetime has ended.
+    #[allow(dead_code)] // TODO: Remove when Platform is implemented
+    pub(crate) unsafe fn new(buffer: core::ptr::NonNull<T>) -> Allowed<'b, T> {
+        Allowed {
+            buffer,
+            _phantom: core::marker::PhantomData,
+        }
+    }
+
+    // Sets the value in the buffer.
+    pub fn set(&self, value: T) {
+        unsafe {
+            core::ptr::write_volatile(self.buffer.as_ptr(), value);
+        }
+    }
+}
+
+impl<'b, T: crate::AllowReadable + Copy + 'b> Allowed<'b, T> {
+    pub fn replace(&self, value: T) -> T {
+        let current = unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) };
+        unsafe {
+            core::ptr::write_volatile(self.buffer.as_ptr(), value);
+        }
+        current
+    }
+
+    pub fn get(&self) -> T {
+        unsafe { core::ptr::read_volatile(self.buffer.as_ptr()) }
+    }
+}
+
+impl<'b, T: crate::AllowReadable + Copy + Default + 'b> Allowed<'b, T> {
+    pub fn take(&self) -> T {
+        self.replace(T::default())
+    }
+}

core/platform/src/allows/allowed_tests.rs

@@ -0,0 +1,124 @@
+use crate::Allowed;
+use core::marker::PhantomData;
+use core::ptr::NonNull;
+
+// How do we simulate accesses to the shared buffer by the kernel?
+//
+// Well, a naive way would be to mutate the `buffer` variable directly. Because
+// Allowed accesses the memory through a *mut pointer, such a test would compile
+// and run fine with the current Rust compiler. As far as I can tell, it would
+// not hit any behavior documented as undefined at
+// https://doc.rust-lang.org/stable/reference/behavior-considered-undefined.html,
+// nor would it cause rustc to generate LLVM bitcode that encounters undefined
+// behavior.
+//
+// However, the naive approach will throw an "undefined behavior" error when run
+// under Miri (e.g. with `cargo miri test`), which uses the stacked borrows
+// model [1]. In particular, accessing the `buffer` variable directly pops the
+// SharedRW off buffer's borrow stack, which prevents Allowed from using its
+// *mut pointer to access `buffer` afterwards. It is likely that Rust will adopt
+// the stacked borrows model as its formal model for borrow validity, and in
+// that case accessing `buffer` in that manner will become undefined behavior.
+// In addition, running these tests under Miri is highly valuable, as this is
+// tricky code to get correct and an unsound API may be hard to fix.
+//
+// Instead, we explicitly refer to buffer through use of a KernelPtr that
+// simulates the pointer that `allow()` would hand to the Tock kernel. As far as
+// the stacked borrows model is concerned, accesses through the KernelPtr
+// variable behave identically to mutations performed by the kernel. This
+// pattern allows us to use `cargo miri test` to not only execute the unit
+// tests, but to test whether Allowed would encounter undefined behavior when
+// interacting with a real Tock kernel.
+//
+// [1] https://plv.mpi-sws.org/rustbelt/stacked-borrows/paper.pdf
+struct KernelPtr<'b, T: Copy + 'b> {
+    ptr: NonNull<T>,
+
+    // We need to consume the 'b lifetime. This is very similar to Allowed's
+    // implementation.
+    _phantom: PhantomData<&'b mut T>,
+}
+
+impl<'b, T: Copy + 'b> KernelPtr<'b, T> {
+    // The constructor for KernelPtr; simulates allow(). Returns both the
+    // Allowed instance the Platform would return and a KernelPtr the test can
+    // use to simulate a kernel.
+    pub fn allow(buffer: &'b mut T) -> (Allowed<'b, T>, KernelPtr<'b, T>) {
+        let ptr = NonNull::new(buffer).unwrap();
+        // Discard buffer *without* creating a reference to it, as would be done
+        // if we called drop().
+        let _ = buffer;
+        // All 3 preconditions of Allowed::new are satisfied by the fact that
+        // `buffer` is directly derived from a &'b mut T.
+        let allowed = unsafe { Allowed::new(ptr) };
+        let kernel_ptr = KernelPtr {
+            ptr,
+            _phantom: PhantomData,
+        };
+        (allowed, kernel_ptr)
+    }
+
+    // Replaces the value in the buffer with a new one.
+    pub fn set(&self, value: T) {
+        unsafe {
+            core::ptr::write(self.ptr.as_ptr(), value);
+        }
+    }
+
+    // Copies the contained value out of the buffer.
+    pub fn get(&self) -> T {
+        unsafe { core::ptr::read(self.ptr.as_ptr()) }
+    }
+}
+
+#[test]
+fn set() {
+    let mut buffer = 1;
+    let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
+    assert_eq!(kernel_ptr.get(), 1);
+
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
+    allowed.set(3);
+    assert_eq!(kernel_ptr.get(), 3);
+}
+
+#[test]
+fn replace() {
+    let mut buffer = 1;
+    let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
+    assert_eq!(kernel_ptr.get(), 1);
+
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
+    let returned = allowed.replace(3);
+    assert_eq!(returned, 2);
+    assert_eq!(kernel_ptr.get(), 3);
+}
+
+#[test]
+fn get() {
+    let mut buffer = 1;
+    let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
+    assert_eq!(kernel_ptr.get(), 1);
+
+    assert_eq!(allowed.get(), 1);
+    assert_eq!(kernel_ptr.get(), 1);
+
+    kernel_ptr.set(2);
+    assert_eq!(allowed.get(), 2);
+    assert_eq!(kernel_ptr.get(), 2);
+}
+
+#[test]
+fn take() {
+    let mut buffer = 1;
+    let (allowed, kernel_ptr) = KernelPtr::allow(&mut buffer);
+    assert_eq!(kernel_ptr.get(), 1);
+
+    // Simulate the kernel replacing the value in buffer.
+    kernel_ptr.set(2);
+    let returned = allowed.take();
+    assert_eq!(returned, 2);
+    assert_eq!(kernel_ptr.get(), 0);
+}

core/platform/src/allows/mod.rs

@@ -0,0 +1,8 @@
+mod allow_readable;
+mod allowed;
+
+pub use allow_readable::AllowReadable;
+pub use allowed::Allowed;
+
+#[cfg(test)]
+mod allowed_tests;

core/platform/src/lib.rs

@@ -8,8 +8,10 @@
 //   3. A system call trait so that Platform works in both real Tock apps and
 //      unit test environments. [DONE]
 
+mod allows;
 mod error_code;
 mod syscalls;
 
+pub use allows::{AllowReadable, Allowed};
 pub use error_code::ErrorCode;
 pub use syscalls::{MemopNoArg, MemopWithArg, Syscalls};