Add a Read-Write Allow API to libtock_platform.

platform/src/allow_rw.rs

@@ -0,0 +1,73 @@
+use crate::share::List;
+use crate::Syscalls;
+use core::marker::PhantomData;
+
+// -----------------------------------------------------------------------------
+// `AllowRw` struct
+// -----------------------------------------------------------------------------
+
+/// A `share::Handle<AllowRw>` instance allows safe code to call Tock's
+/// Read-Write Allow system call, by guaranteeing the buffer will be revoked
+/// before 'share ends. It is intended for use with the `share::scope` function,
+/// which offers a safe interface for constructing `share::Handle<AllowRw>`
+/// instances.
+pub struct AllowRw<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> {
+    _syscalls: PhantomData<S>,
+
+    // Make this struct invariant with respect to the 'share lifetime.
+    //
+    // If AllowRw were covariant with respect to 'share, then an
+    // `AllowRw<'static, ...>` could be used to share a buffer that has a
+    // shorter lifetime. The capsule would still have access to the memory after
+    // the buffer is deallocated and the memory re-used (e.g. if the buffer is
+    // on the stack), allowing it to cause undefined behavior in the process.
+    // Therefore, AllowRw cannot be covariant with respect to 'share.
+    // Contravariance would not have this issue, but would still be confusing
+    // and would be unexpected.
+    //
+    // Additionally, this makes AllowRw !Sync, which is probably desirable, as
+    // Sync would allow for races between threads sharing buffers with the
+    // kernel.
+    _share: PhantomData<core::cell::Cell<&'share mut [u8]>>,
+}
+
+// We can't derive(Default) because S is not Default, and derive(Default)
+// generates a Default implementation that requires S to be Default. Instead, we
+// manually implement Default.
+impl<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> Default
+    for AllowRw<'share, S, DRIVER_NUM, BUFFER_NUM>
+{
+    fn default() -> Self {
+        Self {
+            _syscalls: PhantomData,
+            _share: PhantomData,
+        }
+    }
+}
+
+impl<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> Drop
+    for AllowRw<'share, S, DRIVER_NUM, BUFFER_NUM>
+{
+    fn drop(&mut self) {
+        S::unallow_rw(DRIVER_NUM, BUFFER_NUM);
+    }
+}
+
+impl<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> List
+    for AllowRw<'share, S, DRIVER_NUM, BUFFER_NUM>
+{
+}
+
+// -----------------------------------------------------------------------------
+// `Config` trait
+// -----------------------------------------------------------------------------
+
+/// `Config` configures the behavior of the Read-Write Allow system call. It
+/// should generally be passed through by drivers, to allow application code to
+/// configure error handling.
+pub trait Config {
+    /// Called if a Read-Write Allow call succeeds and returns a non-zero
+    /// buffer. In some applications, this may indicate unexpected reentrance.
+    /// By default, the non-zero buffer is ignored.
+    fn returned_nonzero_buffer(_driver_num: u32, _buffer_num: u32) {}
+}

platform/src/default_config.rs

@@ -3,4 +3,5 @@
 pub struct DefaultConfig;
 
 impl crate::allow_ro::Config for DefaultConfig {}
+impl crate::allow_rw::Config for DefaultConfig {}
 impl crate::subscribe::Config for DefaultConfig {}

platform/src/lib.rs

@@ -2,6 +2,7 @@
 #![warn(unsafe_op_in_unsafe_fn)]
 
 pub mod allow_ro;
+pub mod allow_rw;
 mod async_traits;
 mod command_return;
 mod constants;
@@ -19,6 +20,7 @@ mod termination;
 mod yield_types;
 
 pub use allow_ro::AllowRo;
+pub use allow_rw::AllowRw;
 pub use async_traits::{CallbackContext, FreeCallback, Locator, MethodCallback};
 pub use command_return::CommandReturn;
 pub use constants::{exit_id, syscall_class, yield_id};

platform/src/syscalls.rs

@@ -1,6 +1,6 @@
 use crate::{
-    allow_ro, share, subscribe, AllowRo, CommandReturn, ErrorCode, RawSyscalls, Subscribe, Upcall,
-    YieldNoWaitReturn,
+    allow_ro, allow_rw, share, subscribe, AllowRo, AllowRw, CommandReturn, ErrorCode, RawSyscalls,
+    Subscribe, Upcall, YieldNoWaitReturn,
 };
 
 /// `Syscalls` provides safe abstractions over Tock's system calls. It is
@@ -47,7 +47,20 @@ pub trait Syscalls: RawSyscalls + Sized {
 
     fn command(driver_id: u32, command_id: u32, argument0: u32, argument1: u32) -> CommandReturn;
 
-    // TODO: Add a read-write allow interface.
+    // -------------------------------------------------------------------------
+    // Read-Write Allow
+    // -------------------------------------------------------------------------
+
+    /// Shares a read-write buffer with the kernel.
+    fn allow_rw<'share, CONFIG: allow_rw::Config, const DRIVER_NUM: u32, const BUFFER_NUM: u32>(
+        allow_rw: share::Handle<AllowRw<'share, Self, DRIVER_NUM, BUFFER_NUM>>,
+        buffer: &'share mut [u8],
+    ) -> Result<(), ErrorCode>;
+
+    /// Revokes the kernel's access to the buffer with the given ID, overwriting
+    /// it with a zero buffer. If no buffer is shared with the given ID,
+    /// `unallow_rw` does nothing.
+    fn unallow_rw(driver_num: u32, buffer_num: u32);
 
     // -------------------------------------------------------------------------
     // Read-Only Allow

platform/src/syscalls_impl.rs

@@ -1,9 +1,9 @@
 //! Implements `Syscalls` for all types that implement `RawSyscalls`.
 
 use crate::{
-    allow_ro, exit_id, exit_on_drop, return_variant, share, subscribe, syscall_class, yield_id,
-    AllowRo, CommandReturn, ErrorCode, RawSyscalls, Register, ReturnVariant, Subscribe, Syscalls,
-    Upcall, YieldNoWaitReturn,
+    allow_ro, allow_rw, exit_id, exit_on_drop, return_variant, share, subscribe, syscall_class,
+    yield_id, AllowRo, AllowRw, CommandReturn, ErrorCode, RawSyscalls, Register, ReturnVariant,
+    Subscribe, Syscalls, Upcall, YieldNoWaitReturn,
 };
 
 impl<S: RawSyscalls> Syscalls for S {
@@ -181,6 +181,90 @@ impl<S: RawSyscalls> Syscalls for S {
         }
     }
 
+    // -------------------------------------------------------------------------
+    // Read-Write Allow
+    // -------------------------------------------------------------------------
+
+    fn allow_rw<'share, CONFIG: allow_rw::Config, const DRIVER_NUM: u32, const BUFFER_NUM: u32>(
+        _allow_rw: share::Handle<AllowRw<'share, Self, DRIVER_NUM, BUFFER_NUM>>,
+        buffer: &'share mut [u8],
+    ) -> Result<(), ErrorCode> {
+        // Inner function that does the majority of the work. This is not
+        // monomorphized over DRIVER_NUM and BUFFER_NUM to keep code size small.
+        //
+        // Safety: A share::Handle<AllowRw<'share, S, driver_num, buffer_num>>
+        // must exist, and `buffer` must last for at least the 'share lifetime.
+        unsafe fn inner<S: Syscalls, CONFIG: allow_rw::Config>(
+            driver_num: u32,
+            buffer_num: u32,
+            buffer: &mut [u8],
+        ) -> Result<(), ErrorCode> {
+            // Safety: syscall4's documentation indicates it can be used to call
+            // Read-Write Allow. These arguments follow TRD104.
+            let [r0, r1, r2, _] = unsafe {
+                S::syscall4::<{ syscall_class::ALLOW_RW }>([
+                    driver_num.into(),
+                    buffer_num.into(),
+                    buffer.as_mut_ptr().into(),
+                    buffer.len().into(),
+                ])
+            };
+
+            let return_variant: ReturnVariant = r0.as_u32().into();
+            // TRD 104 guarantees that Read-Write Allow returns either Success
+            // with 2 U32 or Failure with 2 U32. We check the return variant by
+            // comparing against Failure with 2 U32 for 2 reasons:
+            //
+            //   1. On RISC-V with compressed instructions, it generates smaller
+            //      code. FAILURE_2_U32 has value 2, which can be loaded into a
+            //      register with a single compressed instruction, whereas
+            //      loading SUCCESS_2_U32 uses an uncompressed instruction.
+            //   2. In the event the kernel malfuctions and returns a different
+            //      return variant, the success path is actually safer than the
+            //      failure path. The failure path assumes that r1 contains an
+            //      ErrorCode, and produces UB if it has an out of range value.
+            //      Incorrectly assuming the call succeeded will not generate
+            //      unsoundness, and will likely lead to the application
+            //      panicing.
+            if return_variant == return_variant::FAILURE_2_U32 {
+                // Safety: TRD 104 guarantees that if r0 is Failure with 2 U32,
+                // then r1 will contain a valid error code. ErrorCode is
+                // designed to be safely transmuted directly from a kernel error
+                // code.
+                return Err(unsafe { core::mem::transmute(r1.as_u32() as u16) });
+            }
+
+            // r0 indicates Success with 2 u32s. Confirm a zero buffer was
+            // returned, and it if wasn't then call the configured function.
+            // We're relying on the optimizer to remove this branch if
+            // returned_nozero_buffer is a no-op.
+            let returned_buffer: (usize, usize) = (r1.into(), r2.into());
+            if returned_buffer != (0, 0) {
+                CONFIG::returned_nonzero_buffer(driver_num, buffer_num);
+            }
+            Ok(())
+        }
+
+        // Safety: The presence of the share::Handle<AllowRw<'share, ...>>
+        // guarantees that an AllowRw exists and will clean up this Allow ID
+        // before the 'share lifetime ends.
+        unsafe { inner::<Self, CONFIG>(DRIVER_NUM, BUFFER_NUM, buffer) }
+    }
+
+    fn unallow_rw(driver_num: u32, buffer_num: u32) {
+        unsafe {
+            // syscall4's documentation indicates it can be used to call
+            // Read-Write Allow. The buffer passed has 0 length, which cannot
+            // cause undefined behavior on its own.
+            Self::syscall4::<{ syscall_class::ALLOW_RW }>([
+                driver_num.into(),
+                buffer_num.into(),
+                0usize.into(),
+                0usize.into(),
+            ]);
+        }
+    }
+
     // -------------------------------------------------------------------------
     // Read-Only Allow
     // -------------------------------------------------------------------------

syscalls_tests/src/allow_rw.rs

@@ -0,0 +1,129 @@
+use libtock_platform::{allow_rw, share, CommandReturn, ErrorCode, Syscalls};
+use libtock_unittest::{command_return, fake, RwAllowBuffer, SyscallLogEntry};
+use std::cell::Cell;
+use std::rc::Rc;
+use std::thread_local;
+
+#[derive(Default)]
+struct TestDriver {
+    buffer_0: Cell<RwAllowBuffer>,
+}
+
+impl fake::Driver for TestDriver {
+    fn id(&self) -> u32 {
+        42
+    }
+
+    fn num_upcalls(&self) -> u32 {
+        0
+    }
+
+    fn command(&self, _command_num: u32, _argument0: u32, _argument1: u32) -> CommandReturn {
+        command_return::failure(ErrorCode::NoSupport)
+    }
+
+    fn allow_readwrite(
+        &self,
+        buffer_number: u32,
+        buffer: RwAllowBuffer,
+    ) -> Result<RwAllowBuffer, (RwAllowBuffer, ErrorCode)> {
+        if buffer_number != 0 {
+            return Err((buffer, ErrorCode::NoSupport));
+        }
+        Ok(self.buffer_0.replace(buffer))
+    }
+}
+
+struct TestConfig;
+
+// CALLED is set to true when returned_nonzero_buffer is called.
+thread_local! {static CALLED: Cell<bool> = Cell::new(false); }
+
+impl allow_rw::Config for TestConfig {
+    fn returned_nonzero_buffer(driver_num: u32, buffer_num: u32) {
+        assert_eq!(driver_num, 42);
+        assert_eq!(buffer_num, 0);
+        CALLED.with(|cell| cell.set(true));
+    }
+}
+
+#[test]
+fn allow_rw() {
+    let kernel = fake::Kernel::new();
+    let driver = Rc::new(TestDriver::default());
+    kernel.add_driver(&driver);
+    let mut buffer1 = [1, 2, 3, 4];
+    let mut buffer2 = [5, 6];
+    share::scope(|allow_rw| {
+        // Tests a call that should fail because it has an incorrect buffer
+        // number.
+        let result = fake::Syscalls::allow_rw::<TestConfig, 42, 1>(allow_rw, &mut buffer1);
+        assert!(!CALLED.with(|c| c.get()));
+        assert_eq!(result, Err(ErrorCode::NoSupport));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRw {
+                driver_number: 42,
+                buffer_number: 1,
+                len: 4,
+            }]
+        );
+    });
+
+    // Verify that share::scope unallowed the buffer.
+    assert_eq!(
+        kernel.take_syscall_log(),
+        [SyscallLogEntry::AllowRw {
+            driver_number: 42,
+            buffer_number: 1,
+            len: 0,
+        }]
+    );
+
+    share::scope(|allow_rw| {
+        // Tests a call that should succeed and return a zero buffer.
+        let result = fake::Syscalls::allow_rw::<TestConfig, 42, 0>(allow_rw, &mut buffer1);
+        assert!(!CALLED.with(|c| c.get()));
+        assert_eq!(result, Ok(()));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRw {
+                driver_number: 42,
+                buffer_number: 0,
+                len: 4,
+            }]
+        );
+
+        // Tests a call that should succeed and return a nonzero buffer.
+        let result = fake::Syscalls::allow_rw::<TestConfig, 42, 0>(allow_rw, &mut buffer2);
+        assert!(CALLED.with(|c| c.get()));
+        assert_eq!(result, Ok(()));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRw {
+                driver_number: 42,
+                buffer_number: 0,
+                len: 2,
+            }]
+        );
+
+        // Mutate the buffer, which under Miri will verify the buffer has been
+        // shared with the kernel properly.
+        let mut buffer = driver.buffer_0.take();
+        buffer[1] = 31;
+        driver.buffer_0.set(buffer);
+    });
+
+    // Verify that share::scope unallowed the buffer, but only once.
+    assert_eq!(
+        kernel.take_syscall_log(),
+        [SyscallLogEntry::AllowRw {
+            driver_number: 42,
+            buffer_number: 0,
+            len: 0,
+        }]
+    );
+
+    // Verify the buffer write occurred.
+    assert_eq!(buffer2, [5, 31]);
+}

syscalls_tests/src/lib.rs

@@ -13,7 +13,8 @@
 #[cfg(test)]
 mod allow_ro;
 
-// TODO: Add Read-Write Allow.
+#[cfg(test)]
+mod allow_rw;
 
 #[cfg(test)]
 mod command_tests;