From e80989c32773fd5a7f9134ca1a36270f9ffc2838 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Kr=C3=BCger?= Date: Fri, 28 Feb 2025 16:50:39 +0000 Subject: [PATCH 1/4] RISC-V: Support page protections in memory --- src/riscv/lib/src/machine_state.rs | 5 +- src/riscv/lib/src/machine_state/memory.rs | 60 +++++++- .../lib/src/machine_state/memory/config.rs | 35 ++++- .../src/machine_state/memory/protection.rs | 128 +++++++++++++++++ .../lib/src/machine_state/memory/state.rs | 132 +++++++++++++++++- src/riscv/lib/src/pvm/linux.rs | 18 ++- src/riscv/lib/src/pvm/linux/fds.rs | 4 +- src/riscv/lib/src/pvm/linux/rng.rs | 5 +- 8 files changed, 363 insertions(+), 24 deletions(-) create mode 100644 src/riscv/lib/src/machine_state/memory/protection.rs diff --git a/src/riscv/lib/src/machine_state.rs b/src/riscv/lib/src/machine_state.rs index b84e85e20579..361a0d3934b5 100644 --- a/src/riscv/lib/src/machine_state.rs +++ b/src/riscv/lib/src/machine_state.rs @@ -336,9 +336,12 @@ impl, M: backend::Ma where M: backend::ManagerRead, { + // TODO: RV-527: Enforce that `Instruction` came from executable part of memory. + // The link to an `Instruction` is too weak. We want statically-typed guarantees that these + // bytes are only used for instructions. self.core .main_memory - .read(phys_addr) + .read_exec(phys_addr) .map_err(|_: OutOfBounds| Exception::InstructionAccessFault(phys_addr)) } diff --git a/src/riscv/lib/src/machine_state/memory.rs b/src/riscv/lib/src/machine_state/memory.rs index b034334efc6a..59d505ea4752 100644 --- a/src/riscv/lib/src/machine_state/memory.rs +++ b/src/riscv/lib/src/machine_state/memory.rs @@ -3,6 +3,8 @@ // SPDX-License-Identifier: MIT mod config; +#[cfg(feature = "supervisor")] +mod protection; mod state; use std::num::NonZeroU64; @@ -19,6 +21,7 @@ use crate::state_backend::ManagerBase; use crate::state_backend::ManagerClone; use crate::state_backend::ManagerDeserialise; use crate::state_backend::ManagerRead; +use crate::state_backend::ManagerReadWrite; use crate::state_backend::ManagerSerialise; use crate::state_backend::ManagerWrite; use crate::state_backend::ProofLayout; @@ -53,6 +56,42 @@ pub type Address = XValue; /// Lowest address pub const FIRST_ADDRESS: Address = 0; +/// Memory access permissions +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Permissions { + None, + Read, + Write, + ReadWrite, + ReadExec, +} + +impl Permissions { + /// Do the permissions allow reading? + pub const fn can_read(&self) -> bool { + match self { + Self::None | Self::Write => false, + Self::Read | Self::ReadWrite | Self::ReadExec => true, + } + } + + /// Do the permissions allow writing? + pub const fn can_write(&self) -> bool { + match self { + Self::None | Self::Read | Self::ReadExec => false, + Self::ReadWrite | Self::Write => true, + } + } + + /// Do the permissions allow execution? + pub const fn can_exec(&self) -> bool { + match self { + Self::None | Self::Read | Self::ReadWrite | Self::Write => false, + Self::ReadExec => true, + } + } +} + /// Address out of bounds error #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Debug, Error, derive_more::Display)] pub struct OutOfBounds; @@ -71,6 +110,12 @@ pub trait Memory: Sized { E: Elem, M: ManagerRead; + /// Read an element in the region that will be used in execution. `address` is in bytes. + fn read_exec(&self, address: Address) -> Result + where + E: Elem, + M: ManagerRead; + /// Read elements from the region. `address` is in bytes. fn read_all(&self, address: Address, values: &mut [E]) -> Result<(), OutOfBounds> where @@ -81,13 +126,13 @@ pub trait Memory: Sized { fn write(&mut self, address: Address, value: E) -> Result<(), OutOfBounds> where E: Elem, - M: ManagerWrite; + M: ManagerReadWrite; /// Update multiple elements in the region. `address` is in bytes. fn write_all(&mut self, address: Address, values: &[E]) -> Result<(), OutOfBounds> where E: Elem, - M: ManagerWrite; + M: ManagerReadWrite; /// Serialise memory. fn serialise(&self, serializer: S) -> Result @@ -110,6 +155,17 @@ pub trait Memory: Sized { fn reset(&mut self) where M: ManagerWrite; + + /// Protect the pages that belong to the given address range. + #[cfg(feature = "supervisor")] + fn protect_pages( + &mut self, + address: Address, + length: usize, + perms: Permissions, + ) -> Result<(), OutOfBounds> + where + M: ManagerWrite; } /// Memory configuration diff --git a/src/riscv/lib/src/machine_state/memory/config.rs b/src/riscv/lib/src/machine_state/memory/config.rs index 041505b62ac9..d5253cea95d8 100644 --- a/src/riscv/lib/src/machine_state/memory/config.rs +++ b/src/riscv/lib/src/machine_state/memory/config.rs @@ -2,6 +2,10 @@ // // SPDX-License-Identifier: MIT +#[cfg(feature = "supervisor")] +use super::protection::PagePermissions; +#[cfg(feature = "supervisor")] +use super::protection::PagePermissionsLayout; use super::state::MemoryImpl; use crate::state_backend::AllocatedOf; use crate::state_backend::DynArray; @@ -17,8 +21,17 @@ impl super::MemoryConfig { const TOTAL_BYTES: usize = TOTAL_BYTES; + #[cfg(not(feature = "supervisor"))] type Layout = DynArray; + #[cfg(feature = "supervisor")] + type Layout = ( + DynArray, + PagePermissionsLayout, + PagePermissionsLayout, + PagePermissionsLayout, + ); + type State = MemoryImpl; fn bind(space: AllocatedOf) -> Self::State { @@ -34,7 +47,16 @@ impl super::MemoryConfig ); } - MemoryImpl { data: space } + #[cfg(not(feature = "supervisor"))] + return MemoryImpl { data: space }; + + #[cfg(feature = "supervisor")] + MemoryImpl { + data: space.0, + readable_pages: PagePermissions::bind(space.1), + writable_pages: PagePermissions::bind(space.2), + executable_pages: PagePermissions::bind(space.3), + } } fn struct_ref<'a, M, F>(instance: &'a Self::State) -> AllocatedOf @@ -42,7 +64,16 @@ impl super::MemoryConfig M: ManagerBase, F: FnManager>, { - instance.data.struct_ref::() + #[cfg(not(feature = "supervisor"))] + return instance.data.struct_ref::(); + + #[cfg(feature = "supervisor")] + ( + instance.data.struct_ref::(), + instance.readable_pages.struct_ref::(), + instance.writable_pages.struct_ref::(), + instance.executable_pages.struct_ref::(), + ) } } diff --git a/src/riscv/lib/src/machine_state/memory/protection.rs b/src/riscv/lib/src/machine_state/memory/protection.rs new file mode 100644 index 000000000000..e7bf903db45b --- /dev/null +++ b/src/riscv/lib/src/machine_state/memory/protection.rs @@ -0,0 +1,128 @@ +// SPDX-FileCopyrightText: 2025 TriliTech +// +// SPDX-License-Identifier: MIT + +use super::Address; +use crate::state_backend::AllocatedOf; +use crate::state_backend::Atom; +use crate::state_backend::Cell; +use crate::state_backend::FnManager; +use crate::state_backend::ManagerBase; +use crate::state_backend::ManagerClone; +use crate::state_backend::ManagerDeserialise; +use crate::state_backend::ManagerRead; +use crate::state_backend::ManagerSerialise; +use crate::state_backend::ManagerWrite; +use crate::state_backend::Many; +use crate::state_backend::Ref; + +/// State layout for page permissions +pub type PagePermissionsLayout = Many, PAGES>; + +/// Tracks access permissions for each page +pub struct PagePermissions { + pages: Box<[Cell; PAGES]>, +} + +impl PagePermissions { + /// Bind the given allocated space as a page protections state value. + pub fn bind(space: AllocatedOf, M>) -> Self { + Self { + pages: space.try_into().unwrap_or_else(|_| { + unreachable!("Converting a vector into an array of the same length always succeeds") + }), + } + } + + /// Given a manager morphism `f : &M -> N`, return the layout's allocated structure containing + /// the constituents of `N` that were produced from the constituents of `&M`. + pub fn struct_ref<'a, F: FnManager>>( + &'a self, + ) -> AllocatedOf, F::Output> { + self.pages + .iter() + .map(|item| item.struct_ref::()) + .collect() + } + + /// Check if the memory at `address..address+length` can be accessed. + /// + /// # Safety + /// + /// The address and length must be valid for an address space consisting of a number of `PAGES`. + /// This function is not defined for address and length combinations which are out of bounds. + #[inline] + pub unsafe fn can_access(&self, address: Address, length: usize) -> bool + where + M: ManagerRead, + { + // Zero-sized accesses are always valid + if length < 1 { + return true; + } + + let address = address as usize; + + // Extract the page index range from using the start and end addresses + let start_page = address >> super::OFFSET_BITS; + let end_page = address.wrapping_add(length).wrapping_sub(1) >> super::OFFSET_BITS; + + for page in start_page..=end_page { + if !self.pages.get_unchecked(page).read() { + return false; + } + } + + true + } + + /// Change the access permissions for the given range. + pub fn modify_access(&mut self, address: Address, length: usize, accessible: bool) + where + M: ManagerWrite, + { + if length < 1 { + return; + } + + let address = address as usize; + let start_page = address >> super::OFFSET_BITS; + let end_page = address.wrapping_add(length).wrapping_sub(1) >> super::OFFSET_BITS; + + (start_page..=end_page) + .filter(|&page| page < PAGES) + .for_each(|page| { + self.pages[page].write(accessible); + }) + } +} + +impl<'de, const PAGES: usize, M: ManagerDeserialise> serde::Deserialize<'de> + for PagePermissions +{ + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + Ok(Self::bind( + , M>>::deserialize(deserializer)?, + )) + } +} + +impl serde::Serialize for PagePermissions { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + self.pages.serialize(serializer) + } +} + +impl Clone for PagePermissions { + fn clone(&self) -> Self { + Self { + pages: self.pages.clone(), + } + } +} diff --git a/src/riscv/lib/src/machine_state/memory/state.rs b/src/riscv/lib/src/machine_state/memory/state.rs index 86050c447cc6..fd79ee6d000e 100644 --- a/src/riscv/lib/src/machine_state/memory/state.rs +++ b/src/riscv/lib/src/machine_state/memory/state.rs @@ -10,18 +10,36 @@ use serde::Serialize; use super::Address; use super::Memory; use super::OutOfBounds; +#[cfg(feature = "supervisor")] +use super::Permissions; +#[cfg(feature = "supervisor")] +use super::protection::PagePermissions; use crate::state_backend::DynCells; use crate::state_backend::Elem; use crate::state_backend::ManagerBase; use crate::state_backend::ManagerClone; use crate::state_backend::ManagerDeserialise; use crate::state_backend::ManagerRead; +use crate::state_backend::ManagerReadWrite; use crate::state_backend::ManagerSerialise; use crate::state_backend::ManagerWrite; /// Machine's memory pub struct MemoryImpl { + /// Memory contents pub(super) data: DynCells, + + /// Read permissions per page + #[cfg(feature = "supervisor")] + pub(super) readable_pages: PagePermissions, + + /// Write permissions per page + #[cfg(feature = "supervisor")] + pub(super) writable_pages: PagePermissions, + + /// Execute permissions per page + #[cfg(feature = "supervisor")] + pub(super) executable_pages: PagePermissions, } impl @@ -50,6 +68,38 @@ where M: ManagerRead, { Self::check_bounds(address, mem::size_of::())?; + + // SAFETY: The bounds check above ensures the access check below is safe + #[cfg(feature = "supervisor")] + unsafe { + if !self.readable_pages.can_access(address, mem::size_of::()) { + return Err(OutOfBounds); + } + } + + Ok(self.data.read(address as usize)) + } + + #[inline] + fn read_exec(&self, address: Address) -> Result + where + E: Elem, + M: ManagerRead, + { + Self::check_bounds(address, mem::size_of::())?; + + // SAFETY: The bounds check above ensures the access check below is safe + #[cfg(feature = "supervisor")] + unsafe { + // Checking for executable access is sufficient as that implies read access + if !self + .executable_pages + .can_access(address, mem::size_of::()) + { + return Err(OutOfBounds); + } + } + Ok(self.data.read(address as usize)) } @@ -59,6 +109,18 @@ where M: ManagerRead, { Self::check_bounds(address, mem::size_of_val(values))?; + + // SAFETY: The bounds check above ensures the access check below is safe + #[cfg(feature = "supervisor")] + unsafe { + if !self + .readable_pages + .can_access(address, mem::size_of_val(values)) + { + return Err(OutOfBounds); + } + } + self.data.read_all(address as usize, values); Ok(()) } @@ -67,9 +129,18 @@ where fn write(&mut self, address: Address, value: E) -> Result<(), OutOfBounds> where E: Elem, - M: ManagerWrite, + M: ManagerReadWrite, { Self::check_bounds(address, mem::size_of::())?; + + // SAFETY: The bounds check above ensures the access check below is safe + #[cfg(feature = "supervisor")] + unsafe { + if !self.writable_pages.can_access(address, mem::size_of::()) { + return Err(OutOfBounds); + } + } + self.data.write(address as usize, value); Ok(()) } @@ -77,9 +148,21 @@ where fn write_all(&mut self, address: Address, values: &[E]) -> Result<(), OutOfBounds> where E: Elem, - M: ManagerWrite, + M: ManagerReadWrite, { Self::check_bounds(address, mem::size_of_val(values))?; + + // SAFETY: The bounds check above ensures the access check below is safe + #[cfg(feature = "supervisor")] + unsafe { + if !self + .writable_pages + .can_access(address, mem::size_of_val(values)) + { + return Err(OutOfBounds); + } + } + self.data.write_all(address as usize, values); Ok(()) } @@ -97,8 +180,22 @@ where M: ManagerDeserialise, D: serde::Deserializer<'de>, { + #[cfg(not(feature = "supervisor"))] let data = DynCells::deserialize(deserializer)?; - Ok(Self { data }) + + #[cfg(feature = "supervisor")] + let (data, readable_pages, writable_pages, executable_pages) = + Deserialize::deserialize(deserializer)?; + + Ok(Self { + data, + #[cfg(feature = "supervisor")] + readable_pages, + #[cfg(feature = "supervisor")] + writable_pages, + #[cfg(feature = "supervisor")] + executable_pages, + }) } fn clone(&self) -> Self @@ -107,6 +204,12 @@ where { Self { data: self.data.clone(), + #[cfg(feature = "supervisor")] + readable_pages: self.readable_pages.clone(), + #[cfg(feature = "supervisor")] + writable_pages: self.writable_pages.clone(), + #[cfg(feature = "supervisor")] + executable_pages: self.executable_pages.clone(), } } @@ -131,6 +234,28 @@ where self.data.write(address.saturating_add(i), 0u8); } } + + #[cfg(feature = "supervisor")] + fn protect_pages( + &mut self, + address: Address, + length: usize, + perms: Permissions, + ) -> Result<(), OutOfBounds> + where + M: ManagerWrite, + { + Self::check_bounds(address, length)?; + + self.readable_pages + .modify_access(address, length, perms.can_read()); + self.writable_pages + .modify_access(address, length, perms.can_write()); + self.executable_pages + .modify_access(address, length, perms.can_exec()); + + Ok(()) + } } #[cfg(test)] @@ -138,7 +263,6 @@ pub mod tests { use super::*; use crate::backend_test; use crate::machine_state::memory::M4K; - use crate::machine_state::memory::Memory; use crate::machine_state::memory::MemoryConfig; use crate::state_backend::owned_backend::Owned; diff --git a/src/riscv/lib/src/pvm/linux.rs b/src/riscv/lib/src/pvm/linux.rs index 171cc9a6b4b3..fa0e29245ff6 100644 --- a/src/riscv/lib/src/pvm/linux.rs +++ b/src/riscv/lib/src/pvm/linux.rs @@ -23,6 +23,7 @@ use crate::machine_state::block_cache::bcall::Block; use crate::machine_state::memory::Address; use crate::machine_state::memory::Memory; use crate::machine_state::memory::MemoryConfig; +use crate::machine_state::memory::PAGE_SIZE; use crate::machine_state::mode::Mode; use crate::machine_state::registers; use crate::program::Program; @@ -37,9 +38,6 @@ use crate::state_backend::ManagerReadWrite; use crate::state_backend::ManagerWrite; use crate::state_backend::Ref; -/// Size of a memory page in bytes -pub const PAGE_SIZE: u64 = 4096; - /// Thread identifier for the main thread const MAIN_THREAD_ID: u64 = 1; @@ -113,7 +111,7 @@ impl, M: ManagerBase> /// Add data to the stack, returning the updated stack pointer. fn push_stack(&mut self, align: u64, data: impl AsRef<[u8]>) -> Result where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { let data = data.as_ref(); @@ -152,7 +150,7 @@ impl, M: ManagerBase> auxv: &[(AuxVectorKey, u64)], ) -> Result<(), MachineError> where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { // First we push all constants so that they are at the top of the stack let arg_ptrs = args @@ -199,7 +197,7 @@ impl, M: ManagerBase> /// Load the program into memory and set the PC to its entrypoint. fn load_program(&mut self, program: &Program) -> Result<(), MachineError> where - M: ManagerWrite, + M: ManagerReadWrite, { // Reset hart state & set pc to entrypoint self.core.hart.reset(program.entrypoint); @@ -223,7 +221,7 @@ impl, M: ManagerBase> self.prepare_stack(); // Auxiliary values vector - let mut auxv = vec![(AuxVectorKey::PageSize, PAGE_SIZE)]; + let mut auxv = vec![(AuxVectorKey::PageSize, PAGE_SIZE.get())]; // If program headers are available, then we should inform the supervised process of them if let Some(prog_headers) = &program.program_headers { @@ -385,7 +383,7 @@ impl SupervisorState { /// old signal stack configuration is requested, it will be zeroed out. fn handle_sigaltstack(&mut self, core: &mut MachineCoreState) -> bool where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { let old = core.hart.xregisters.read(registers::a1); @@ -411,7 +409,7 @@ impl SupervisorState { /// See: fn handle_rt_sigaction(&mut self, core: &mut MachineCoreState) -> bool where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { let old_action = core.hart.xregisters.read(registers::a2); let sigset_t_size = core.hart.xregisters.read(registers::a3); @@ -449,7 +447,7 @@ impl SupervisorState { /// requested, it will simply be zeroed out. fn handle_rt_sigprocmask(&mut self, core: &mut MachineCoreState) -> bool where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { let old = core.hart.xregisters.read(registers::a2); let sigset_t_size = core.hart.xregisters.read(registers::a3); diff --git a/src/riscv/lib/src/pvm/linux/fds.rs b/src/riscv/lib/src/pvm/linux/fds.rs index e839727a2987..0113b4eb97e6 100644 --- a/src/riscv/lib/src/pvm/linux/fds.rs +++ b/src/riscv/lib/src/pvm/linux/fds.rs @@ -38,7 +38,7 @@ impl SupervisorState { M: ManagerRead, { // Limit how much data we can write to prevent proof-size explosion - let length = length.min(PAGE_SIZE); + let length = length.min(PAGE_SIZE.get()); // Check if the file desciprtor is valid and can be written to. // In our case, it's just standard output (1) and standard error (2). @@ -129,7 +129,7 @@ impl SupervisorState { let (addr, length) = 'search: { // Limit the number of entries through which we iterate to prevent proof-size explosion - let max_entries = len.min(PAGE_SIZE / SIZE_IOVEC); + let max_entries = len.min(PAGE_SIZE.get() / SIZE_IOVEC); for item in 0..max_entries { let struct_addr = SIZE_IOVEC.wrapping_mul(item).wrapping_add(iovec); diff --git a/src/riscv/lib/src/pvm/linux/rng.rs b/src/riscv/lib/src/pvm/linux/rng.rs index ca87b5987b17..537bf3df7b7a 100644 --- a/src/riscv/lib/src/pvm/linux/rng.rs +++ b/src/riscv/lib/src/pvm/linux/rng.rs @@ -9,8 +9,7 @@ use crate::machine_state::memory::MemoryConfig; use crate::machine_state::registers; use crate::pvm::linux::error::Error; use crate::state_backend::ManagerBase; -use crate::state_backend::ManagerRead; -use crate::state_backend::ManagerWrite; +use crate::state_backend::ManagerReadWrite; impl SupervisorState { /// Handle `getrandom` system call. We don't support non-determinism, so we return a fixed @@ -24,7 +23,7 @@ impl SupervisorState { core: &mut MachineCoreState, ) -> bool where - M: ManagerRead + ManagerWrite, + M: ManagerReadWrite, { let buffer = core.hart.xregisters.read(registers::a0); let length = core.hart.xregisters.read(registers::a1); -- GitLab From 613b086f8b0b9771beb8d50a12c7e01f91ad0d11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Kr=C3=BCger?= Date: Wed, 5 Mar 2025 21:35:56 +0000 Subject: [PATCH 2/4] RISC-V: Dedicated type for supervisor virtual addresses --- src/riscv/lib/src/pvm/linux.rs | 1 + src/riscv/lib/src/pvm/linux/addr.rs | 143 ++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 src/riscv/lib/src/pvm/linux/addr.rs diff --git a/src/riscv/lib/src/pvm/linux.rs b/src/riscv/lib/src/pvm/linux.rs index fa0e29245ff6..d1aa6d9ff077 100644 --- a/src/riscv/lib/src/pvm/linux.rs +++ b/src/riscv/lib/src/pvm/linux.rs @@ -2,6 +2,7 @@ // // SPDX-License-Identifier: MIT +mod addr; mod error; mod fds; mod fs; diff --git a/src/riscv/lib/src/pvm/linux/addr.rs b/src/riscv/lib/src/pvm/linux/addr.rs new file mode 100644 index 000000000000..cefea9346577 --- /dev/null +++ b/src/riscv/lib/src/pvm/linux/addr.rs @@ -0,0 +1,143 @@ +// SPDX-FileCopyrightText: 2025 TriliTech +// +// SPDX-License-Identifier: MIT + +use std::fmt; +use std::num::NonZeroU64; +use std::ops::Add; +use std::ops::Sub; + +use crate::default::ConstDefault; +use crate::machine_state; + +/// Virtual address +#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, serde::Serialize, serde::Deserialize)] +#[repr(transparent)] +pub struct VirtAddr(u64); + +impl VirtAddr { + /// Create a new virtual address. + pub const fn new(addr: u64) -> Self { + Self(addr) + } + + /// Return an aligned address that is equal or lower than the current address. + pub const fn align_down(self, align: NonZeroU64) -> Self { + let overhang = self.0.rem_euclid(align.get()); + VirtAddr(self.0.saturating_sub(overhang)) + } + + /// Return an aligned address that is equal or higher than the current address. + pub const fn align_up(self, align: NonZeroU64) -> Option { + let overhang = self.0.rem_euclid(align.get()); + + if overhang == 0 { + return Some(self); + } + + let underhang = align.get().saturating_sub(overhang); + let Some(new) = self.0.checked_add(underhang) else { + // Can't use ? operator in constant expression + return None; + }; + + Some(VirtAddr(new)) + } + + /// Is the address a multiple of the given alignment? + pub const fn is_aligned(self, align: NonZeroU64) -> bool { + self.0.rem_euclid(align.get()) == 0 + } + + /// Convert the virtual address to the machine state's memory address representation. + pub fn to_machine_address(self) -> machine_state::memory::Address { + self.0 + } +} + +impl ConstDefault for VirtAddr { + const DEFAULT: Self = VirtAddr(u64::MAX); +} + +impl fmt::Debug for VirtAddr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#x}", self.0) + } +} + +impl fmt::Display for VirtAddr { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{:#x}", self.0) + } +} + +impl Add for VirtAddr { + type Output = Self; + + fn add(self, rhs: u64) -> Self::Output { + VirtAddr(self.0.wrapping_add(rhs)) + } +} + +impl Sub for VirtAddr { + type Output = i64; + + fn sub(self, rhs: Self) -> Self::Output { + (self.0 as i64).wrapping_sub(rhs.0 as i64) + } +} + +impl Sub for VirtAddr { + type Output = Self; + + fn sub(self, rhs: u64) -> Self::Output { + VirtAddr(self.0.wrapping_sub(rhs)) + } +} + +impl PartialEq for VirtAddr { + fn eq(&self, other: &u64) -> bool { + self.0.eq(other) + } +} + +impl PartialOrd for VirtAddr { + fn partial_cmp(&self, other: &u64) -> Option { + self.0.partial_cmp(other) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_align_up() { + proptest::proptest!(|(org_addr: u64, align: NonZeroU64)| { + let org_addr = VirtAddr::new(org_addr); + let addr = org_addr.align_up(align); + + match addr { + Some(addr) => { + assert!(addr.is_aligned(align)); + assert!(addr >= org_addr.0); + } + + None => { + assert!(org_addr.to_machine_address() >= u64::MAX - align.get()); + } + } + }); + } + + #[test] + fn test_align_down() { + proptest::proptest!(|(org_addr: u64, align: NonZeroU64)| { + let org_addr = VirtAddr::new(org_addr); + let addr = org_addr.align_down(align); + + assert!(addr.is_aligned(align)); + assert!(addr <= org_addr.0); + }); + } +} -- GitLab From 3507f0630cdcdddfe062b250f2de3ef70d41463e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Kr=C3=BCger?= Date: Tue, 4 Mar 2025 22:19:11 +0000 Subject: [PATCH 3/4] RISC-V: Protect the loaded program in memory --- src/riscv/lib/src/kernel_loader.rs | 85 +++++++++++++++++++++++++++--- src/riscv/lib/src/machine_state.rs | 3 ++ src/riscv/lib/src/pvm/linux.rs | 83 +++++++++++++++++++++++++---- src/riscv/lib/src/stepper/pvm.rs | 2 +- 4 files changed, 153 insertions(+), 20 deletions(-) diff --git a/src/riscv/lib/src/kernel_loader.rs b/src/riscv/lib/src/kernel_loader.rs index 51879595a457..43165048e28e 100644 --- a/src/riscv/lib/src/kernel_loader.rs +++ b/src/riscv/lib/src/kernel_loader.rs @@ -12,6 +12,8 @@ use goblin::elf::header::ET_EXEC; use goblin::elf::program_header::PT_LOAD; use goblin::elf::program_header::ProgramHeader; +use crate::machine_state::memory::Permissions; + #[derive(Debug, From, Error, derive_more::Display)] pub enum Error { #[display(fmt = "At address {:#x}: {:?}", addr, "msg.clone()")] @@ -22,6 +24,46 @@ pub enum Error { Goblin(goblin::error::Error), } +/// Permissions for program regions in memory +pub struct MemoryPermissions { + /// Starting address of the memory region + pub start_address: u64, + + /// Length of the memory region + pub length: u64, + + /// Permissions for the memory region + pub permissions: Permissions, +} + +impl MemoryPermissions { + /// Extract the memory permissions from a load segment. `reloc` can be used to offset the + /// memory region. + pub fn from_load_segment(reloc: Option, segment: &ProgramHeader) -> Self { + let start_address = match reloc { + Some(offset) => offset.wrapping_add(segment.p_vaddr), + None => segment.p_paddr, + }; + + MemoryPermissions { + start_address, + length: segment.p_memsz, + + // This is a simplified version of the permissions. It is aligned with what to + // expect in an ELF file. + permissions: if segment.is_executable() { + Permissions::ReadExec + } else if segment.is_write() { + Permissions::ReadWrite + } else if segment.is_read() { + Permissions::Read + } else { + Permissions::None + }, + } + } +} + /// Program headers extracted from the ELF file pub struct ProgramHeaders<'a> { /// Size of each entry in the program headers @@ -32,6 +74,9 @@ pub struct ProgramHeaders<'a> { /// Raw contents pub contents: &'a [u8], + + /// Memory permissions for each segment + pub permissions: Vec, } /// [LoadResult] is the outcome of loading an ELF file @@ -71,22 +116,31 @@ pub fn load_elf_nonreloc<'a>( let mut last_written = 0; + // Capacity of 4 should cover most cases: + // .text => rx + // .rodata => r + // .data => rw + // .bss => rw + let mut permissions = Vec::with_capacity(4); + for segment in loadable_segments { + permissions.push(MemoryPermissions::from_load_segment(None, segment)); + // Copy the region from the file to memory. mem.write_bytes(segment.p_paddr, &contents[segment.file_range()])?; // If the target memory region is larger than the source region, // we must fill the gap with 0s. if segment.p_memsz > segment.p_filesz { - let first_zero = segment.p_paddr + segment.p_filesz; - let num_zeroes = segment.p_memsz - segment.p_filesz; + let first_zero = segment.p_paddr.wrapping_add(segment.p_filesz); + let num_zeroes = segment.p_memsz.saturating_sub(segment.p_filesz); mem.set_zero(first_zero, num_zeroes)?; } last_written = segment.p_paddr + segment.p_memsz; } - let program_headers = extract_program_headers(&elf.header, contents); + let program_headers = extract_program_headers(&elf.header, contents, permissions); Ok(LoadResult { entry: elf.entry, @@ -120,15 +174,25 @@ pub fn load_elf_reloc<'a>( let mut last_written = start; + // Capacity of 4 should cover most cases: + // .text => rx + // .rodata => r + // .data => rw + // .bss => rw + let mut permissions = Vec::with_capacity(4); + for segment in loadable_segments { + permissions.push(MemoryPermissions::from_load_segment(Some(start), segment)); + // Copy the region from the file to memory. - mem.write_bytes(start + segment.p_vaddr, &contents[segment.file_range()])?; + let start_addr = start.wrapping_add(segment.p_vaddr); + mem.write_bytes(start_addr, &contents[segment.file_range()])?; // If the target memory region is larger than the source region, // we must fill the gap with 0s. if segment.p_memsz > segment.p_filesz { - let first_zero = start + segment.p_vaddr + segment.p_filesz; - let num_zeroes = segment.p_memsz - segment.p_filesz; + let first_zero = start_addr.wrapping_add(segment.p_filesz); + let num_zeroes = segment.p_memsz.saturating_sub(segment.p_filesz); mem.set_zero(first_zero, num_zeroes)?; } @@ -152,7 +216,7 @@ pub fn load_elf_reloc<'a>( } } - let program_headers = extract_program_headers(&elf.header, contents); + let program_headers = extract_program_headers(&elf.header, contents, permissions); Ok(LoadResult { entry: elf.header.e_entry + start, @@ -195,7 +259,11 @@ where } /// Extract the raw program headers from the ELF file. -pub fn extract_program_headers<'a>(header: &Header, contents: &'a [u8]) -> ProgramHeaders<'a> { +fn extract_program_headers<'a>( + header: &Header, + contents: &'a [u8], + permissions: Vec, +) -> ProgramHeaders<'a> { let start = header.e_phoff as usize; let length = header.e_phentsize as usize * header.e_phnum as usize; let contents = &contents[start..][..length]; @@ -204,5 +272,6 @@ pub fn extract_program_headers<'a>(header: &Header, contents: &'a [u8]) -> Progr entry_size: header.e_phentsize as u64, num_entries: header.e_phnum as u64, contents, + permissions, } } diff --git a/src/riscv/lib/src/machine_state.rs b/src/riscv/lib/src/machine_state.rs index 361a0d3934b5..61ab0f454ecf 100644 --- a/src/riscv/lib/src/machine_state.rs +++ b/src/riscv/lib/src/machine_state.rs @@ -677,6 +677,9 @@ pub enum MachineError { #[error("Device tree error: {0}")] DeviceTreeError(vm_fdt::Error), + + #[error("Memory too small to properly configure the machine")] + MemoryTooSmall, } #[cfg(test)] diff --git a/src/riscv/lib/src/pvm/linux.rs b/src/riscv/lib/src/pvm/linux.rs index d1aa6d9ff077..16d33f625360 100644 --- a/src/riscv/lib/src/pvm/linux.rs +++ b/src/riscv/lib/src/pvm/linux.rs @@ -11,9 +11,10 @@ mod rng; use std::ffi::CStr; use std::num::NonZeroU64; -use error::Error; use tezos_smart_rollup_constants::riscv::SBI_FIRMWARE_TEZOS; +use self::addr::VirtAddr; +use self::error::Error; use super::Pvm; use super::PvmHooks; use crate::machine_state::CacheLayouts; @@ -25,6 +26,7 @@ use crate::machine_state::memory::Address; use crate::machine_state::memory::Memory; use crate::machine_state::memory::MemoryConfig; use crate::machine_state::memory::PAGE_SIZE; +use crate::machine_state::memory::Permissions; use crate::machine_state::mode::Mode; use crate::machine_state::registers; use crate::program::Program; @@ -194,20 +196,66 @@ impl, M: ManagerBase> Ok(()) } +} +impl, M: ManagerBase> Pvm { /// Load the program into memory and set the PC to its entrypoint. fn load_program(&mut self, program: &Program) -> Result<(), MachineError> where M: ManagerReadWrite, { // Reset hart state & set pc to entrypoint - self.core.hart.reset(program.entrypoint); + self.machine_state.core.hart.reset(program.entrypoint); + + let program_start = program.segments.keys().min().copied().unwrap_or(0); + let program_end = program + .segments + .iter() + .map(|(addr, data)| addr.saturating_add(data.len() as u64)) + .max() + .unwrap_or(0); + let program_length = program_end.saturating_sub(program_start) as usize; + + // Allow the program to be written to main memory + self.machine_state.core.main_memory.protect_pages( + program_start, + program_length, + Permissions::Write, + )?; // Write program to main memory - for (addr, data) in program.segments.iter() { - self.core.main_memory.write_all(*addr, data)?; + for (&addr, data) in program.segments.iter() { + self.machine_state.core.main_memory.write_all(addr, data)?; + } + + // Remove access to the program that has just been placed into memory + self.machine_state.core.main_memory.protect_pages( + program_start, + program_length, + Permissions::None, + )?; + + // Configure memory permissions using the ELF program headers, if present + if let Some(program_headers) = &program.program_headers { + for mem_perms in program_headers.permissions.iter() { + self.machine_state.core.main_memory.protect_pages( + mem_perms.start_address, + mem_perms.length as usize, + mem_perms.permissions, + )?; + } } + // Other parts of the supervisor make use of program start and end to properly divide the + // memory. These addresses need to be properly aligned. + let program_start = VirtAddr::new(program_start).align_down(PAGE_SIZE); + self.system_state.program_start.write(program_start); + + let program_end = VirtAddr::new(program_end) + .align_up(PAGE_SIZE) + .ok_or(MachineError::MemoryTooSmall)?; + self.system_state.program_end.write(program_end); + Ok(()) } @@ -219,7 +267,7 @@ impl, M: ManagerBase> self.load_program(program)?; // The stack needs to be prepared before we can push anything to it - self.prepare_stack(); + self.machine_state.prepare_stack(); // Auxiliary values vector let mut auxv = vec![(AuxVectorKey::PageSize, PAGE_SIZE.get())]; @@ -228,23 +276,22 @@ impl, M: ManagerBase> if let Some(prog_headers) = &program.program_headers { // Program headers are an array of a C struct. The struct for 64-bit ELF requires 8 // byte alignment. - let prog_headers_ptr = self.push_stack(8, prog_headers.contents)?; + let prog_headers_ptr = self.machine_state.push_stack(8, prog_headers.contents)?; auxv.push((AuxVectorKey::NumProgramHeaders, prog_headers.num_entries)); auxv.push((AuxVectorKey::ProgramHeaderSize, prog_headers.entry_size)); auxv.push((AuxVectorKey::ProgramHeadersPtr, prog_headers_ptr)); } - self.init_linux_stack(&[c"tezos-smart-rollup"], &[], &auxv)?; + self.machine_state + .init_linux_stack(&[c"tezos-smart-rollup"], &[], &auxv)?; // The user program may not access the M or S privilege level - self.core.hart.mode.write(Mode::User); + self.machine_state.core.hart.mode.write(Mode::User); Ok(()) } -} -impl, M: ManagerBase> Pvm { /// Check if the supervised process has requested an exit. pub fn has_exited(&self) -> Option where @@ -260,13 +307,21 @@ impl, M: ManagerBase> Pvm, Atom, Atom); +pub type SupervisorStateLayout = ( + Atom, + Atom, + Atom, + Atom, + Atom, +); /// Linux supervisor state pub struct SupervisorState { tid_address: Cell, exited: Cell, exit_code: Cell, + program_start: Cell, + program_end: Cell, } impl SupervisorState { @@ -276,6 +331,8 @@ impl SupervisorState { tid_address: space.0, exited: space.1, exit_code: space.2, + program_start: space.3, + program_end: space.4, } } @@ -288,6 +345,8 @@ impl SupervisorState { self.tid_address.struct_ref::(), self.exited.struct_ref::(), self.exit_code.struct_ref::(), + self.program_start.struct_ref::(), + self.program_end.struct_ref::(), ) } @@ -519,6 +578,8 @@ impl Clone for SupervisorState { tid_address: self.tid_address.clone(), exited: self.exited.clone(), exit_code: self.exit_code.clone(), + program_start: self.program_start.clone(), + program_end: self.program_end.clone(), } } } diff --git a/src/riscv/lib/src/stepper/pvm.rs b/src/riscv/lib/src/stepper/pvm.rs index efb9e80b7109..5b7833c8f636 100644 --- a/src/riscv/lib/src/stepper/pvm.rs +++ b/src/riscv/lib/src/stepper/pvm.rs @@ -91,7 +91,7 @@ impl<'hooks, MC: MemoryConfig, B: Block, CL: CacheLayouts> #[cfg(feature = "supervisor")] { - pvm.machine_state.setup_linux_process(&program)?; + pvm.setup_linux_process(&program)?; assert!(initrd.is_none(), "initrd is not supported"); } -- GitLab From ae8d4996c4493afff4aaa4dceb8c66cbe40e40be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Kr=C3=BCger?= Date: Tue, 4 Mar 2025 22:45:08 +0000 Subject: [PATCH 4/4] RISC-V: Protect the stack memory region --- src/riscv/lib/src/pvm/linux.rs | 70 +++++++++++++++++++++------ src/riscv/lib/src/pvm/linux/memory.rs | 11 +++++ 2 files changed, 67 insertions(+), 14 deletions(-) create mode 100644 src/riscv/lib/src/pvm/linux/memory.rs diff --git a/src/riscv/lib/src/pvm/linux.rs b/src/riscv/lib/src/pvm/linux.rs index 16d33f625360..893bbf98119c 100644 --- a/src/riscv/lib/src/pvm/linux.rs +++ b/src/riscv/lib/src/pvm/linux.rs @@ -6,6 +6,7 @@ mod addr; mod error; mod fds; mod fs; +mod memory; mod rng; use std::ffi::CStr; @@ -15,6 +16,7 @@ use tezos_smart_rollup_constants::riscv::SBI_FIRMWARE_TEZOS; use self::addr::VirtAddr; use self::error::Error; +use self::memory::STACK_SIZE; use super::Pvm; use super::PvmHooks; use crate::machine_state::CacheLayouts; @@ -129,19 +131,6 @@ impl, M: ManagerBase> Ok(stack_ptr) } - /// Configure the stack for a new process. - fn prepare_stack(&mut self) - where - M: ManagerWrite, - { - let mem_size = MC::TOTAL_BYTES as u64; - let init_stack_ptr = mem_size.saturating_sub(mem_size % PAGE_SIZE); - self.core - .hart - .xregisters - .write(registers::sp, init_stack_ptr); - } - /// Initialise the stack for a Linux program. Preparing the stack is a major part of Linux's /// process initialisation. Musl programs extract valuable information from the stack such as /// the program name, command-line arguments, environment variables and other auxiliary @@ -259,6 +248,59 @@ impl, M: ManagerBase> Pvm Result<(), MachineError> + where + M: ManagerReadWrite, + { + let stack_top = VirtAddr::new(MC::TOTAL_BYTES as u64); + let program_break = self.system_state.program_end.read(); + + // We must fit at least one guard page between the program break and the stack + let guarded_stack_space = stack_top - program_break; + if guarded_stack_space < PAGE_SIZE.get() as i64 { + return Err(MachineError::MemoryTooSmall); + } + + let unaligned_stack_space = STACK_SIZE.min(guarded_stack_space as u64 - PAGE_SIZE.get()); + let stack_bottom = (stack_top - unaligned_stack_space) + .align_up(PAGE_SIZE) + .ok_or(MachineError::MemoryTooSmall)?; + + // If the stack top wasn't aligned, the stack bottom may be higher than the stack top after + // aligning it upwards + if stack_top < stack_bottom { + return Err(MachineError::MemoryTooSmall); + } + + // At this point we know that `stack_top` >= `stack_bottom` + let stack_space = (stack_top - stack_bottom) as usize; + + // Guard the stack with a guard page. This prevents stack overflows spilling into the heap + // or even worse, the program's .bss or .data area. + let stack_guard = stack_bottom - PAGE_SIZE.get(); + self.machine_state.core.main_memory.protect_pages( + stack_guard.to_machine_address(), + PAGE_SIZE.get() as usize, + Permissions::None, + )?; + + // Make sure the stack region is readable and writable + self.machine_state.core.main_memory.protect_pages( + stack_bottom.to_machine_address(), + stack_space, + Permissions::ReadWrite, + )?; + + self.machine_state + .core + .hart + .xregisters + .write(registers::sp, stack_top.to_machine_address()); + + Ok(()) + } + /// Install a Linux program and configure the Hart to start it. pub fn setup_linux_process(&mut self, program: &Program) -> Result<(), MachineError> where @@ -267,7 +309,7 @@ impl, M: ManagerBase> Pvm +// +// SPDX-License-Identifier: MIT + +use crate::machine_state::memory::PAGE_SIZE; + +/// Number of pages that make up the stack +const STACK_PAGES: u64 = 0x2000; + +/// Maximum stack size in bytes +pub const STACK_SIZE: u64 = PAGE_SIZE.get() * STACK_PAGES; -- GitLab