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>
diff --git a/core/platform/src/allows/allow_readable.rs b/core/platform/src/allows/allow_readable.rs
new file mode 100644
index 0000000..c35701b
--- /dev/null
+++ b/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 {}
diff --git a/core/platform/src/allows/allowed.rs b/core/platform/src/allows/allowed.rs
new file mode 100644
index 0000000..4d66c2b
--- /dev/null
+++ b/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())
+ }
+}
diff --git a/core/platform/src/allows/allowed_tests.rs b/core/platform/src/allows/allowed_tests.rs
new file mode 100644
index 0000000..f1f0c08
--- /dev/null
+++ b/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);
+}
diff --git a/core/platform/src/allows/mod.rs b/core/platform/src/allows/mod.rs
new file mode 100644
index 0000000..8cb2158
--- /dev/null
+++ b/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;
diff --git a/core/platform/src/lib.rs b/core/platform/src/lib.rs
index 2e3dec6..7237a1d 100644
--- a/core/platform/src/lib.rs
+++ b/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};
