Merge #348

348: Add a Read-Only Allow API to `libtock_platform`. r=jrvanwhy a=jrvanwhy



Co-authored-by: Johnathan Van Why <[email protected]>

platform/src/allow_ro.rs

@@ -0,0 +1,73 @@
+use crate::share::List;
+use crate::Syscalls;
+use core::marker::PhantomData;
+
+// -----------------------------------------------------------------------------
+// `AllowRo` struct
+// -----------------------------------------------------------------------------
+
+/// A `share::Handle<AllowRo>` instance allows safe code to call Tock's
+/// Read-Only 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<AllowRo>`
+/// instances.
+pub struct AllowRo<'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 AllowRo were covariant with respect to 'share, then an
+    // `AllowRo<'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), likely leaking data the process binary does not want to
+    // share. Therefore, AllowRo 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 AllowRo !Sync, which is probably desirable, as
+    // Sync would allow for races between threads sharing buffers with the
+    // kernel.
+    _share: PhantomData<core::cell::Cell<&'share [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 AllowRo<'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 AllowRo<'share, S, DRIVER_NUM, BUFFER_NUM>
+{
+    fn drop(&mut self) {
+        S::unallow_ro(DRIVER_NUM, BUFFER_NUM);
+    }
+}
+
+impl<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> List
+    for AllowRo<'share, S, DRIVER_NUM, BUFFER_NUM>
+{
+}
+
+// -----------------------------------------------------------------------------
+// `Config` trait
+// -----------------------------------------------------------------------------
+
+/// `Config` configures the behavior of the Read-Only 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-Only 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

@@ -2,4 +2,5 @@
 /// default syscall config.
 pub struct DefaultConfig;
 
+impl crate::allow_ro::Config for DefaultConfig {}
 impl crate::subscribe::Config for DefaultConfig {}

platform/src/lib.rs

@@ -1,6 +1,7 @@
 #![cfg_attr(not(test), no_std)]
 #![warn(unsafe_op_in_unsafe_fn)]
 
+pub mod allow_ro;
 mod async_traits;
 mod command_return;
 mod constants;
@@ -17,6 +18,7 @@ mod syscalls_impl;
 mod termination;
 mod yield_types;
 
+pub use allow_ro::AllowRo;
 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,5 +1,6 @@
 use crate::{
-    share, subscribe, CommandReturn, ErrorCode, RawSyscalls, Subscribe, Upcall, YieldNoWaitReturn,
+    allow_ro, share, subscribe, AllowRo, CommandReturn, ErrorCode, RawSyscalls, Subscribe, Upcall,
+    YieldNoWaitReturn,
 };
 
 /// `Syscalls` provides safe abstractions over Tock's system calls. It is
@@ -48,7 +49,20 @@ pub trait Syscalls: RawSyscalls + Sized {
 
     // TODO: Add a read-write allow interface.
 
-    // TODO: Add a read-only allow interface.
+    // -------------------------------------------------------------------------
+    // Read-Only Allow
+    // -------------------------------------------------------------------------
+
+    /// Shares a read-only buffer with the kernel.
+    fn allow_ro<'share, CONFIG: allow_ro::Config, const DRIVER_NUM: u32, const BUFFER_NUM: u32>(
+        allow_ro: share::Handle<AllowRo<'share, Self, DRIVER_NUM, BUFFER_NUM>>,
+        buffer: &'share [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_ro` does nothing.
+    fn unallow_ro(driver_num: u32, buffer_num: u32);
 
     // TODO: Add memop() methods.
 

platform/src/syscalls_impl.rs

@@ -1,9 +1,9 @@
 //! Implements `Syscalls` for all types that implement `RawSyscalls`.
 
 use crate::{
-    exit_id, exit_on_drop, return_variant, share, subscribe, syscall_class, yield_id,
-    CommandReturn, ErrorCode, RawSyscalls, Register, ReturnVariant, Subscribe, Syscalls, Upcall,
-    YieldNoWaitReturn,
+    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,
 };
 
 impl<S: RawSyscalls> Syscalls for S {
@@ -181,6 +181,93 @@ impl<S: RawSyscalls> Syscalls for S {
         }
     }
 
+    // -------------------------------------------------------------------------
+    // Read-Only Allow
+    // -------------------------------------------------------------------------
+
+    fn allow_ro<'share, CONFIG: allow_ro::Config, const DRIVER_NUM: u32, const BUFFER_NUM: u32>(
+        _allow_ro: share::Handle<AllowRo<'share, Self, DRIVER_NUM, BUFFER_NUM>>,
+        buffer: &'share [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.
+        //
+        // Security note: The syscall driver will retain read-only access to
+        // `*buffer` until this Allow ID is unallowed or overwritten via another
+        // Allow call. Therefore the caller must ensure the Allow ID is
+        // unallowed or overwritten before `*buffer` is deallocated, to avoid
+        // leaking newly-allocated information at the same address as `*buffer`.
+        fn inner<S: Syscalls, CONFIG: allow_ro::Config>(
+            driver_num: u32,
+            buffer_num: u32,
+            buffer: &[u8],
+        ) -> Result<(), ErrorCode> {
+            // Safety: syscall4's documentation indicates it can be used to call
+            // Read-Only Allow. These arguments follow TRD104.
+            let [r0, r1, r2, _] = unsafe {
+                S::syscall4::<{ syscall_class::ALLOW_RO }>([
+                    driver_num.into(),
+                    buffer_num.into(),
+                    buffer.as_ptr().into(),
+                    buffer.len().into(),
+                ])
+            };
+
+            let return_variant: ReturnVariant = r0.as_u32().into();
+            // TRD 104 guarantees that Read-Only 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(())
+        }
+
+        // Security: The presence of the share::Handle<AllowRo<'share, ...>>
+        // guarantees that an AllowRo exists and will clean up this Allow ID
+        // before the 'share lifetime ends.
+        inner::<Self, CONFIG>(DRIVER_NUM, BUFFER_NUM, buffer)
+    }
+
+    fn unallow_ro(driver_num: u32, buffer_num: u32) {
+        unsafe {
+            // syscall4's documentation indicates it can be used to call
+            // Read-Only Allow. The buffer passed has 0 length, which cannot
+            // cause undefined behavior on its own.
+            Self::syscall4::<{ syscall_class::ALLOW_RO }>([
+                driver_num.into(),
+                buffer_num.into(),
+                0usize.into(),
+                0usize.into(),
+            ]);
+        }
+    }
+
     // -------------------------------------------------------------------------
     // Exit
     // -------------------------------------------------------------------------

syscalls_tests/src/allow_ro.rs

@@ -0,0 +1,120 @@
+use libtock_platform::{allow_ro, share, CommandReturn, ErrorCode, Syscalls};
+use libtock_unittest::{command_return, fake, RoAllowBuffer, SyscallLogEntry};
+use std::cell::Cell;
+use std::rc::Rc;
+use std::thread_local;
+
+#[derive(Default)]
+struct TestDriver {
+    buffer_0: Cell<RoAllowBuffer>,
+}
+
+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_readonly(
+        &self,
+        buffer_number: u32,
+        buffer: RoAllowBuffer,
+    ) -> Result<RoAllowBuffer, (RoAllowBuffer, 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_ro::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_ro() {
+    let kernel = fake::Kernel::new();
+    let driver = Rc::new(TestDriver::default());
+    kernel.add_driver(&driver);
+    let buffer1 = [1, 2, 3, 4];
+    let buffer2 = [5, 6];
+    share::scope(|allow_ro| {
+        // Tests a call that should fail because it has an incorrect buffer
+        // number.
+        let result = fake::Syscalls::allow_ro::<TestConfig, 42, 1>(allow_ro, &buffer1);
+        assert!(!CALLED.with(|c| c.get()));
+        assert_eq!(result, Err(ErrorCode::NoSupport));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRo {
+                driver_number: 42,
+                buffer_number: 1,
+                len: 4,
+            }]
+        );
+    });
+
+    // Verify that share::scope unallowed the buffer.
+    assert_eq!(
+        kernel.take_syscall_log(),
+        [SyscallLogEntry::AllowRo {
+            driver_number: 42,
+            buffer_number: 1,
+            len: 0,
+        }]
+    );
+
+    share::scope(|allow_ro| {
+        // Tests a call that should succeed and return a zero buffer.
+        let result = fake::Syscalls::allow_ro::<TestConfig, 42, 0>(allow_ro, &buffer1);
+        assert!(!CALLED.with(|c| c.get()));
+        assert_eq!(result, Ok(()));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRo {
+                driver_number: 42,
+                buffer_number: 0,
+                len: 4,
+            }]
+        );
+
+        // Tests a call that should succeed and return a nonzero buffer.
+        let result = fake::Syscalls::allow_ro::<TestConfig, 42, 0>(allow_ro, &buffer2);
+        assert!(CALLED.with(|c| c.get()));
+        assert_eq!(result, Ok(()));
+        assert_eq!(
+            kernel.take_syscall_log(),
+            [SyscallLogEntry::AllowRo {
+                driver_number: 42,
+                buffer_number: 0,
+                len: 2,
+            }]
+        );
+    });
+
+    // Verify that share::scope unallowed the buffer, but only once.
+    assert_eq!(
+        kernel.take_syscall_log(),
+        [SyscallLogEntry::AllowRo {
+            driver_number: 42,
+            buffer_number: 0,
+            len: 0,
+        }]
+    );
+}

syscalls_tests/src/lib.rs

@@ -10,7 +10,10 @@
 //! Mixing types from the two `libtock_platform` instantiations in tests results
 //! in confusing error messages, so instead those tests live in this crate.
 
-// TODO: Add Allow.
+#[cfg(test)]
+mod allow_ro;
+
+// TODO: Add Read-Write Allow.
 
 #[cfg(test)]
 mod command_tests;