From 6c25d561fba31e29848541e66660ec8c1bacb9a9 Mon Sep 17 00:00:00 2001 From: Johnathan Van Why Date: Sun, 12 Dec 2021 12:05:56 -0800 Subject: [PATCH 1/2] Add a Read-Write Allow API to `libtock_platform`. --- platform/src/allow_rw.rs | 73 +++++++++++++++++++ platform/src/default_config.rs | 1 + platform/src/lib.rs | 2 + platform/src/syscalls.rs | 18 ++++- platform/src/syscalls_impl.rs | 90 ++++++++++++++++++++++- syscalls_tests/src/allow_rw.rs | 129 +++++++++++++++++++++++++++++++++ syscalls_tests/src/lib.rs | 5 +- 7 files changed, 312 insertions(+), 6 deletions(-) create mode 100644 platform/src/allow_rw.rs create mode 100644 syscalls_tests/src/allow_rw.rs diff --git a/platform/src/allow_rw.rs b/platform/src/allow_rw.rs new file mode 100644 index 00000000..52fcf1e2 --- /dev/null +++ b/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` 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` +/// instances. +pub struct AllowRw<'share, S: Syscalls, const DRIVER_NUM: u32, const BUFFER_NUM: u32> { + _syscalls: PhantomData, + + // 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>, +} + +// 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) {} +} diff --git a/platform/src/default_config.rs b/platform/src/default_config.rs index 60254cff..7278e641 100644 --- a/platform/src/default_config.rs +++ b/platform/src/default_config.rs @@ -2,4 +2,5 @@ /// default syscall config. pub struct DefaultConfig; +impl crate::allow_rw::Config for DefaultConfig {} impl crate::subscribe::Config for DefaultConfig {} diff --git a/platform/src/lib.rs b/platform/src/lib.rs index da791f85..5993e882 100644 --- a/platform/src/lib.rs +++ b/platform/src/lib.rs @@ -1,6 +1,7 @@ #![cfg_attr(not(test), no_std)] #![warn(unsafe_op_in_unsafe_fn)] +pub mod allow_rw; mod async_traits; mod command_return; mod constants; @@ -17,6 +18,7 @@ mod syscalls_impl; mod termination; mod yield_types; +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}; diff --git a/platform/src/syscalls.rs b/platform/src/syscalls.rs index f60034e2..ec54f04c 100644 --- a/platform/src/syscalls.rs +++ b/platform/src/syscalls.rs @@ -1,5 +1,6 @@ use crate::{ - share, subscribe, CommandReturn, ErrorCode, RawSyscalls, Subscribe, Upcall, YieldNoWaitReturn, + allow_rw, share, subscribe, AllowRw, CommandReturn, ErrorCode, RawSyscalls, Subscribe, Upcall, + YieldNoWaitReturn, }; /// `Syscalls` provides safe abstractions over Tock's system calls. It is @@ -46,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>, + 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); // TODO: Add a read-only allow interface. diff --git a/platform/src/syscalls_impl.rs b/platform/src/syscalls_impl.rs index c6fa5f19..cc79a7d2 100644 --- a/platform/src/syscalls_impl.rs +++ b/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_rw, exit_id, exit_on_drop, return_variant, share, subscribe, syscall_class, yield_id, + AllowRw, CommandReturn, ErrorCode, RawSyscalls, Register, ReturnVariant, Subscribe, Syscalls, + Upcall, YieldNoWaitReturn, }; impl Syscalls for S { @@ -181,6 +181,90 @@ impl 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>, + 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> + // must exist, and `buffer` must last for at least the 'share lifetime. + unsafe fn inner( + 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> + // guarantees that an AllowRw exists and will clean up this Allow ID + // before the 'share lifetime ends. + unsafe { inner::(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(), + ]); + } + } + // ------------------------------------------------------------------------- // Exit // ------------------------------------------------------------------------- diff --git a/syscalls_tests/src/allow_rw.rs b/syscalls_tests/src/allow_rw.rs new file mode 100644 index 00000000..9086f1f0 --- /dev/null +++ b/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, +} + +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 { + 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 = 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::(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::(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::(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]); +} diff --git a/syscalls_tests/src/lib.rs b/syscalls_tests/src/lib.rs index adec718f..8e6732d5 100644 --- a/syscalls_tests/src/lib.rs +++ b/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. +// TODO: Add Read-Only Allow. + +#[cfg(test)] +mod allow_rw; #[cfg(test)] mod command_tests; From 902afff4010c09dc72d023c4febfa32843819ea8 Mon Sep 17 00:00:00 2001 From: Johnathan Van Why Date: Tue, 14 Dec 2021 17:53:25 -0800 Subject: [PATCH 2/2] Mark the RW allow implementation with #'s so that git can better merge RO allow and RW allow. --- platform/src/syscalls_impl.rs | 166 +++++++++++++++++----------------- 1 file changed, 83 insertions(+), 83 deletions(-) diff --git a/platform/src/syscalls_impl.rs b/platform/src/syscalls_impl.rs index cc79a7d2..d11a6f38 100644 --- a/platform/src/syscalls_impl.rs +++ b/platform/src/syscalls_impl.rs @@ -181,89 +181,89 @@ impl 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>, - 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> - // must exist, and `buffer` must last for at least the 'share lifetime. - unsafe fn inner( - 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> - // guarantees that an AllowRw exists and will clean up this Allow ID - // before the 'share lifetime ends. - unsafe { inner::(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-Write Allow +# // ------------------------------------------------------------------------- +# +# fn allow_rw<'share, CONFIG: allow_rw::Config, const DRIVER_NUM: u32, const BUFFER_NUM: u32>( +# _allow_rw: share::Handle>, +# 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> +# // must exist, and `buffer` must last for at least the 'share lifetime. +# unsafe fn inner( +# 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> +# // guarantees that an AllowRw exists and will clean up this Allow ID +# // before the 'share lifetime ends. +# unsafe { inner::(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(), +# ]); +# } +# } // ------------------------------------------------------------------------- // Exit