From f6d2dc00ad3fe907dc8fc312ae8187df0cff816e Mon Sep 17 00:00:00 2001 From: Sangho Lee Date: Fri, 29 May 2026 04:25:47 +0000 Subject: [PATCH] Physical memory range ownership for safe memory API --- dev_tests/src/ratchet.rs | 2 +- litebox_common_linux/src/physical_pointers.rs | 185 ++++++---------- litebox_common_linux/src/vmap.rs | 81 +++++-- litebox_common_optee/src/lib.rs | 2 +- litebox_platform_linux_userland/src/lib.rs | 4 +- litebox_platform_lvbs/src/lib.rs | 198 +++++++++++++++--- litebox_platform_lvbs/src/mshv/ringbuffer.rs | 4 +- litebox_platform_lvbs/src/mshv/vsm.rs | 29 +-- litebox_runner_lvbs/src/lib.rs | 19 +- litebox_shim_optee/src/msg_handler.rs | 20 +- 10 files changed, 336 insertions(+), 208 deletions(-) diff --git a/dev_tests/src/ratchet.rs b/dev_tests/src/ratchet.rs index d5b76f069..71190b32d 100644 --- a/dev_tests/src/ratchet.rs +++ b/dev_tests/src/ratchet.rs @@ -37,7 +37,7 @@ fn ratchet_globals() -> Result<()> { ("litebox/", 9), ("litebox_platform_linux_kernel/", 6), ("litebox_platform_linux_userland/", 5), - ("litebox_platform_lvbs/", 24), + ("litebox_platform_lvbs/", 25), ("litebox_platform_multiplex/", 1), ("litebox_platform_windows_userland/", 8), ("litebox_runner_lvbs/", 6), diff --git a/litebox_common_linux/src/physical_pointers.rs b/litebox_common_linux/src/physical_pointers.rs index 4ddf51a43..03bad3ad1 100644 --- a/litebox_common_linux/src/physical_pointers.rs +++ b/litebox_common_linux/src/physical_pointers.rs @@ -3,67 +3,41 @@ //! Physical Pointer Abstraction with On-demand Mapping //! -//! This module adds supports for accessing physical addresses (e.g., VTL0 or -//! normal-world physical memory) from LiteBox with on-demand mapping. -//! In the context of LVBS and OP-TEE, accessing physical memory is necessary -//! because VTL0 and VTL1 as well as normal world and secure world do not share -//! the same virtual address space, but they still have to share data through memory. -//! VTL1 and secure world receive physical addresses from VTL0 and normal world, -//! respectively, and they need to read from or write to those addresses. +//! This module adds supports for accessing physical addresses (e.g., VTL0 +//! or normal-world physical memory) from LiteBox with on-demand mapping. +//! In the context of LVBS and OP-TEE, accessing physical memory is +//! necessary because VTL0 and VTL1 as well as normal world and secure +//! world exchange data using physical addresses. //! -//! To simplify all these, we could persistently map the entire VTL0/normal-world -//! physical memory into VTL1/secure-world address space at once and just access them -//! through corresponding virtual addresses. However, this module does not take these -//! approaches due to scalability (e.g., how to deal with a system with terabytes of -//! physical memory?) and security concerns (e.g., data corruption or information -//! leakage due to concurrent or persistent access). +//! The safe read/write APIs in this module follow the same safety model as +//! safe wrappers around DMA buffers or shared physical memory. The +//! physical memory is external to Rust's ordinary ownership model and may +//! be changed by hardware or another privilege level. These APIs remain +//! safe because they do not create Rust references into that external +//! memory; they only perform bounded copies between a temporary mapping +//! and memory owned by LiteBox. Cooperating LiteBox mappings are +//! serialized by the platform's physical range ownership policy so safe +//! callers cannot create conflicting mappings through this abstraction. //! -//! Instead, the approach this module takes is to map the required physical memory -//! region on-demand when accessing them while using a LiteBox-owned buffer to copy -//! data to/from those regions. This way, this module can ensure that data must be -//! copied into LiteBox-owned memory before being used while avoiding any unknown -//! side effects due to persistent memory mapping. -//! -//! Considerations: -//! -//! Ideally, this module should be able to validate whether a given physical address -//! is okay to access or even exists in the first place. For example, accessing -//! LiteBox's own memory with this physical pointer abstraction must be prohibited to -//! prevent the Boomerang attack and any other undefined memory access. Also, some -//! device memory is mapped to certain physical address ranges and LiteBox should not -//! touch them without in-depth knowledge. However, this is a bit tricky because, in -//! many cases, LiteBox does not directly interact with the underlying hardware or -//! BIOS/UEFI such that it does not have complete knowledge of the physical memory -//! layout. In the case of LVBS, LiteBox obtains the physical memory information -//! from VTL0 including the total physical memory size and the memory range assigned -//! to VTL1/LiteBox. Thus, this module can at least confirm a given physical address -//! does not belong to VTL1's physical memory. -//! -//! This module should allow byte-level access while transparently handling page -//! mapping and data access across page boundaries. This could become complicated -//! when we consider multiple page sizes (e.g., 4 KiB, 2 MiB, 1 GiB). Also, -//! unaligned access is a matter to be considered. -//! -//! In addition, often times, this physical pointer abstraction is involved with -//! a list of physical addresses (i.e., scatter-gather list). For example, in -//! the worse case, a two-byte value can span across two non-contiguous physical -//! pages (the last byte of the first page and the first byte of the second page). -//! Thus, to enhance the performance, we may need to consider mapping multiple pages -//! at once, copy data from/to them, and unmap them later. -//! -//! When this module needs to access data across physical page boundaries, it assumes -//! that those physical pages are virtually contiguous in VTL0 or normal-world address -//! space. Otherwise, this module could end up with accessing misordered data. This is -//! best-effort assumption and ensuring this is the caller's responsibility (e.g., even -//! if this module always requires a list of physical addresses, the caller might -//! provide a wrong list by mistake or intentionally). +//! The safe APIs should validate whether a given physical address is okay +//! to access. For example, accessing LiteBox's own memory through this +//! physical pointer abstraction is prohibited to avoid confused-deputy +//! attacks and to ensure Rust memory safety. In the case of LVBS, LiteBox +//! obtains the physical memory information from VTL0 like the total +//! physical memory range assigned to VTL1/LiteBox. Thus, this module can +//! confirm a given physical address does not belong to VTL1's physical +//! memory. use crate::vmap::{ GlobalVmapManager, PhysPageAddr, PhysPageMapInfo, PhysPageMapPermissions, PhysPointerError, VmapManager, }; use core::marker::PhantomData; -use zerocopy::FromBytes; +use zerocopy::{FromBytes, IntoBytes}; + +/// The concrete [`PhysPageMapInfo`] produced by the `VmapManager` behind a [`GlobalVmapManager`]. +type MapInfoOf = + <>::Manager as VmapManager>::MapInfo; /// Allocate a zeroed `Box` on the heap. /// @@ -98,6 +72,14 @@ fn align_up(len: usize, align: usize) -> usize { } /// Represent a physical pointer to an object with on-demand mapping. +/// +/// Safe methods on this type copy to or from a temporary mapping. They never expose +/// references or slices into the mapped physical memory. +/// +/// Read methods require `T: FromBytes` because external memory may contain any bit pattern. +/// Write methods require `T: IntoBytes` because values are written by copying their byte +/// representation. +/// /// - `pages`: An array of page-aligned physical addresses. We expect physical addresses in this array are /// virtually contiguous. /// - `offset`: The offset within `pages[0]` where the object starts. It should be smaller than `ALIGN`. @@ -105,13 +87,12 @@ fn align_up(len: usize, align: usize) -> usize { /// - `map_info`: The mapping information of the currently mapped physical pages, if any. /// - `T`: The type of the object being pointed to. `pages` with respect to `offset` should cover enough /// memory for an object of type `T`. -#[derive(Clone)] #[repr(C)] pub struct PhysMutPtr> { pages: alloc::boxed::Box<[PhysPageAddr]>, offset: usize, count: usize, - map_info: Option>, + map_info: Option>, _type: PhantomData, _vmap: PhantomData, } @@ -200,16 +181,8 @@ where /// Read the value at the given offset from the physical pointer. /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, it might read corrupt data. - /// `FromBytes` is required to ensure T is valid for any bit pattern from untrusted physical memory. - pub unsafe fn read_at_offset( - &mut self, - count: usize, - ) -> Result, PhysPointerError> + /// Returns an owned copy of the value read from physical memory. + pub fn read_at_offset(&mut self, count: usize) -> Result, PhysPointerError> where T: FromBytes, { @@ -239,13 +212,8 @@ where /// Read a slice of values at the given offset from the physical pointer. /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, it might read corrupt data. - /// `FromBytes` is required to ensure T is valid for any bit pattern from untrusted physical memory. - pub unsafe fn read_slice_at_offset( + /// Copies values from physical memory into the caller-provided slice. + pub fn read_slice_at_offset( &mut self, count: usize, values: &mut [T], @@ -283,17 +251,10 @@ where } /// Write the value at the given offset to the physical pointer. - /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, data it writes might be overwritten. - pub unsafe fn write_at_offset( - &mut self, - count: usize, - value: T, - ) -> Result<(), PhysPointerError> { + pub fn write_at_offset(&mut self, count: usize, value: T) -> Result<(), PhysPointerError> + where + T: IntoBytes, + { if count >= self.count { return Err(PhysPointerError::IndexOutOfBounds(count, self.count)); } @@ -318,17 +279,14 @@ where } /// Write a slice of values at the given offset to the physical pointer. - /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, data it writes might be overwritten. - pub unsafe fn write_slice_at_offset( + pub fn write_slice_at_offset( &mut self, count: usize, values: &[T], - ) -> Result<(), PhysPointerError> { + ) -> Result<(), PhysPointerError> + where + T: IntoBytes, + { if values.is_empty() { return Ok(()); } @@ -397,7 +355,7 @@ where .map_info .as_ref() .ok_or(PhysPointerError::NoMappingInfo)?; - let ptr = map_info.base.wrapping_add(skip % ALIGN).cast::(); + let ptr = map_info.base().wrapping_add(skip % ALIGN).cast::(); let _ = map_info; Ok(MappedGuard { owner: self, @@ -446,8 +404,11 @@ where /// requests for the same physical pages. unsafe fn unmap(&mut self) -> Result<(), PhysPointerError> { if let Some(map_info) = self.map_info.take() { - unsafe { - V::manager().vunmap(map_info)?; + // On failure, `vunmap` hands `map_info` back so the physical range ownership it + // carries is not lost; restore it so a later drop/retry can release it. + if let Err((err, map_info)) = unsafe { V::manager().vunmap(map_info) } { + self.map_info = Some(map_info); + return Err(err); } Ok(()) } else { @@ -472,8 +433,9 @@ impl> Drop for MappedGuard<'_, T, ALIGN, V> { fn drop(&mut self) { - // SAFETY: The platform is expected to handle unmapping safely, including - // the case where pages were never mapped (returns Unmapped error, ignored). + // SAFETY: The platform is expected to handle unmapping safely. Drop cannot + // report errors, so a failed unmap leaves map_info restored in the owner and + // is only reported by debug assertion. let result = unsafe { self.owner.unmap() }; debug_assert!( result.is_ok() || matches!(result, Err(PhysPointerError::Unmapped(_))), @@ -484,8 +446,9 @@ impl> Drop impl> Drop for PhysMutPtr { fn drop(&mut self) { - // SAFETY: The platform is expected to handle unmapping safely, including - // the case where pages were never mapped (returns Unmapped error, ignored). + // SAFETY: The platform is expected to handle unmapping safely. Drop cannot + // report errors, so a failed unmap leaves map_info restored for the remainder + // of this destructor and is only reported by debug assertion. let result = unsafe { self.unmap() }; debug_assert!( result.is_ok() || matches!(result, Err(PhysPointerError::Unmapped(_))), @@ -506,8 +469,7 @@ impl> core::fmt::Debug } /// Represent a physical pointer to a read-only object. This wraps around [`PhysMutPtr`] and -/// exposes only read access. -#[derive(Clone)] +/// exposes only copy-out access. #[repr(C)] pub struct PhysConstPtr> { inner: PhysMutPtr, @@ -554,29 +516,18 @@ where /// Read the value at the given offset from the physical pointer. /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, it might read corrupt data. - pub unsafe fn read_at_offset( - &mut self, - count: usize, - ) -> Result, PhysPointerError> + /// Returns an owned copy of the value read from physical memory. + pub fn read_at_offset(&mut self, count: usize) -> Result, PhysPointerError> where T: FromBytes, { - unsafe { self.inner.read_at_offset(count) } + self.inner.read_at_offset(count) } /// Read a slice of values at the given offset from the physical pointer. /// - /// # Safety - /// - /// The caller should be aware that the given physical address might be concurrently written by - /// other entities (e.g., the normal world kernel) if there is no extra security mechanism - /// in place (e.g., by the hypervisor or hardware). That is, it might read corrupt data. - pub unsafe fn read_slice_at_offset( + /// Copies values from physical memory into the caller-provided slice. + pub fn read_slice_at_offset( &mut self, count: usize, values: &mut [T], @@ -584,7 +535,7 @@ where where T: FromBytes, { - unsafe { self.inner.read_slice_at_offset(count, values) } + self.inner.read_slice_at_offset(count, values) } } diff --git a/litebox_common_linux/src/vmap.rs b/litebox_common_linux/src/vmap.rs index 102f0c9e3..9a08db5c0 100644 --- a/litebox_common_linux/src/vmap.rs +++ b/litebox_common_linux/src/vmap.rs @@ -12,37 +12,53 @@ use thiserror::Error; /// [`crate::physical_pointers::PhysConstPtr`]. It can benefit other modules which need /// Linux kernel's `vmap()` and `vunmap()` functionalities (e.g., HVCI/HEKI, drivers). pub trait VmapManager { + /// Mapping information returned by [`VmapManager::vmap`]. See [`PhysPageMapInfo`]. + /// + /// Implementors use this to carry the virtual mapping and any platform-specific physical + /// range ownership policy (e.g., an RAII ownership guard) for the lifetime of the mapping. + type MapInfo: PhysPageMapInfo; + /// Map the given `PhysPageAddrArray` into virtually contiguous addresses with the given - /// [`PhysPageMapPermissions`] while returning [`PhysPageMapInfo`]. + /// [`PhysPageMapPermissions`] while returning [`Self::MapInfo`]. /// /// This function is analogous to Linux kernel's `vmap()`. /// /// # Safety /// - /// The caller should ensure that `pages` are not in active use by other entities - /// (especially, there should be no read/write or write/write conflicts). - /// Unfortunately, LiteBox itself cannot fully guarantee this and it needs some helps - /// from the caller, hypervisor, or hardware. - /// Multiple LiteBox threads might concurrently call this function with overlapping - /// physical pages, so the implementation should safely handle such cases. + /// This function exposes raw mapped physical memory. The caller must not create Rust + /// references from the returned pointer unless it can uphold Rust's aliasing and validity + /// rules for the full lifetime of those references. + /// + /// Implementations should acquire platform-specific physical range ownership before installing + /// mappings so cooperating LiteBox mappings observe the platform's ownership policy. For copy + /// based physical pointer access, exclusive ownership is sufficient. That policy does not exclude + /// external agents such as DMA devices or another VM privilege level; callers must treat the + /// mapped memory like DMA/shared physical memory rather than ordinary Rust-owned RAM. unsafe fn vmap( &self, _pages: &PhysPageAddrArray, _perms: PhysPageMapPermissions, - ) -> Result, PhysPointerError> { + ) -> Result { Err(PhysPointerError::UnsupportedOperation) } - /// Unmap the previously mapped virtually contiguous addresses ([`PhysPageMapInfo`]). + /// Unmap the previously mapped virtually contiguous addresses ([`Self::MapInfo`]). /// /// This function is analogous to Linux kernel's `vunmap()`. /// + /// On failure, the unchanged `vmap_info` is returned alongside the error so the caller does + /// not lose the physical range ownership it carries and can retry or drop it explicitly. + /// /// # Safety /// - /// The caller should ensure that the virtual addresses in `vmap_info` are not in active - /// use by other entities. - unsafe fn vunmap(&self, _vmap_info: PhysPageMapInfo) -> Result<(), PhysPointerError> { - Err(PhysPointerError::UnsupportedOperation) + /// The caller must ensure there are no outstanding raw-pointer uses or Rust references derived + /// from `PhysPageMapInfo::base()`. After a successful call, the virtual mapping is invalid and + /// the physical range ownership carried by `vmap_info` is released. + unsafe fn vunmap( + &self, + vmap_info: Self::MapInfo, + ) -> Result<(), (PhysPointerError, Self::MapInfo)> { + Err((PhysPointerError::UnsupportedOperation, vmap_info)) } /// Validate that the given physical pages are not owned by LiteBox. @@ -106,13 +122,40 @@ pub type PhysPageAddr = litebox::mm::linux::NonZeroAddress = [PhysPageAddr]; -/// Data structure to maintain the mapping information returned by `vmap()`. -#[derive(Clone)] -pub struct PhysPageMapInfo { +/// Mapping information returned by `vmap()`. +/// +/// Implementors own the lifetime of the virtual mapping and any platform-specific physical range +/// ownership policy. Dropping the map info may release those resources, so callers must treat the +/// value as an RAII guard and pass it back to the same platform's `vunmap()` when explicitly +/// unmapping. +pub trait PhysPageMapInfo { /// Virtual address of the mapped region which is page aligned. - pub base: *mut u8, + fn base(&self) -> *mut u8; /// The size of the mapped region in bytes. - pub size: usize, + fn size(&self) -> usize; +} + +/// A no-op [`PhysPageMapInfo`] for platforms that do not support `vmap()`/`vunmap()`. +#[derive(Debug)] +pub struct NoopPhysPageMapInfo { + base: *mut u8, + size: usize, +} + +impl NoopPhysPageMapInfo { + pub fn new(base: *mut u8, size: usize) -> Self { + Self { base, size } + } +} + +impl PhysPageMapInfo for NoopPhysPageMapInfo { + fn base(&self) -> *mut u8 { + self.base + } + + fn size(&self) -> usize { + self.size + } } bitflags::bitflags! { @@ -196,4 +239,6 @@ pub enum PhysPointerError { VaSpaceExhausted, #[error("Page-table frame allocation failed (out of memory)")] FrameAllocationFailed, + #[error("Physical address range starting at {0:#x} is already locked")] + PhysicalAddressRangeLocked(usize), } diff --git a/litebox_common_optee/src/lib.rs b/litebox_common_optee/src/lib.rs index bcff985e1..8523c6061 100644 --- a/litebox_common_optee/src/lib.rs +++ b/litebox_common_optee/src/lib.rs @@ -2058,7 +2058,7 @@ impl From<&OpteeSmcArgsPage> for OpteeSmcArgs { } /// OP-TEE SMC call arguments. -#[derive(Clone, Copy, Default, FromBytes)] +#[derive(Clone, Copy, Default, FromBytes, IntoBytes, Immutable)] pub struct OpteeSmcArgs { args: [usize; Self::NUM_OPTEE_SMC_ARGS], } diff --git a/litebox_platform_linux_userland/src/lib.rs b/litebox_platform_linux_userland/src/lib.rs index d87e55fa4..d0a2c2481 100644 --- a/litebox_platform_linux_userland/src/lib.rs +++ b/litebox_platform_linux_userland/src/lib.rs @@ -2376,7 +2376,9 @@ impl litebox::platform::DerivedKeyProvider for LinuxUserland { /// In general, userland platforms do not support `vmap` and `vunmap` (which are kernel functions). /// We might need to emulate these functions' behaviors using virtual addresses for development or /// testing, or use a kernel module to provide this functionality (if needed). -impl VmapManager for LinuxUserland {} +impl VmapManager for LinuxUserland { + type MapInfo = litebox_common_linux::vmap::NoopPhysPageMapInfo; +} /// Dummy `VmemPageFaultHandler`. /// diff --git a/litebox_platform_lvbs/src/lib.rs b/litebox_platform_lvbs/src/lib.rs index cbfc58b5c..77cc70640 100644 --- a/litebox_platform_lvbs/src/lib.rs +++ b/litebox_platform_lvbs/src/lib.rs @@ -7,8 +7,11 @@ #![no_std] use crate::{host::per_cpu_variables::PerCpuVariablesAsm, mshv::vsm::Vtl0KernelInfo}; +use alloc::vec::Vec; use core::{ arch::asm, + mem::ManuallyDrop, + ops::Range, sync::atomic::{AtomicU32, AtomicU64}, }; use hashbrown::HashMap; @@ -49,6 +52,104 @@ pub mod mshv; pub mod syscall_entry; static CPU_MHZ: AtomicU64 = AtomicU64::new(0); +static PHYS_RANGE_LOCK: spin::Mutex = + spin::Mutex::new(PhysRangeLockInner::new()); + +struct PhysRangeLockInner { + entries: Vec>, +} + +/// Mapping info returned by [`LinuxKernel`]'s [`VmapManager::vmap`]. +/// +/// Carries the virtual base/size of the mapping together with an RAII ownership guard for the +/// physical ranges it covers. Dropping it (or passing it to `vunmap`) releases the ownership lock, +/// which is what lets the safe physical pointer APIs copy from the mapping without `unsafe`. +pub struct LvbsPhysPageMapInfo { + base: *mut u8, + size: usize, + _ownership_guard: LvbsPhysRangeOwnershipGuard, +} + +struct LvbsPhysRangeOwnershipGuard { + ranges: Vec>, +} + +impl LvbsPhysPageMapInfo { + fn new(base: *mut u8, size: usize, ownership_guard: LvbsPhysRangeOwnershipGuard) -> Self { + Self { + base, + size, + _ownership_guard: ownership_guard, + } + } +} + +impl PhysPageMapInfo for LvbsPhysPageMapInfo { + fn base(&self) -> *mut u8 { + self.base + } + + fn size(&self) -> usize { + self.size + } +} + +impl PhysRangeLockInner { + const fn new() -> Self { + Self { + entries: Vec::new(), + } + } +} + +fn ranges_overlap(left: &Range, right: &Range) -> bool { + left.start < right.end && right.start < left.end +} + +/// Acquire exclusive ownership of the ALIGN-sized physical range covering each entry in +/// `pages`. +/// +/// Precondition: `pages` must not contain duplicate addresses. Intra-call overlap is not +/// validated here. If two entries collide, both copies are pushed into the global lock list +/// and the drop path only removes one of them, permanently leaking the duplicate entry. All +/// current callers (`LinuxKernel::vmap`) pre-validate uniqueness — either by construction +/// (contiguous pages cannot repeat) or by an explicit `HashSet` check on non-contiguous +/// input — so this invariant is upheld today. Any new caller must do the same. +fn acquire_phys_range_ownership( + pages: &PhysPageAddrArray, +) -> Result { + let mut ranges = Vec::with_capacity(pages.len()); + for page in pages { + let start = page.as_usize(); + let end = start.checked_add(ALIGN).ok_or(PhysPointerError::Overflow)?; + ranges.push(start..end); + } + + let mut inner = PHYS_RANGE_LOCK.lock(); + for entry in &inner.entries { + for range in &ranges { + if ranges_overlap(entry, range) { + return Err(PhysPointerError::PhysicalAddressRangeLocked(range.start)); + } + } + } + + inner.entries.extend(ranges.iter().cloned()); + drop(inner); + + Ok(LvbsPhysRangeOwnershipGuard { ranges }) +} + +impl Drop for LvbsPhysRangeOwnershipGuard { + fn drop(&mut self) { + let mut inner = PHYS_RANGE_LOCK.lock(); + for range in &self.ranges { + if let Some(index) = inner.entries.iter().position(|entry| entry == range) { + inner.entries.swap_remove(index); + } + } + } +} /// Special page table ID for the base (kernel-only) page table. /// No real physical frame has address 0, so this is a safe sentinel. @@ -636,11 +737,12 @@ impl LinuxKernel { /// /// Allocator does not allocate memory frames for VTL0 pages, so frame deallocation is not needed. /// - /// Note: VTL0 physical memory is external memory not owned by LiteBox (similar to MMIO). - /// LiteBox accesses it by creating a temporary non-shared mapping, copying data to/from a - /// LiteBox-owned buffer, and unmapping immediately. No Rust references are created to the - /// mapped VTL0 memory; all accesses use raw pointer operations (read_volatile / - /// copy_nonoverlapping) to avoid violating Rust's aliasing model. + /// Note: VTL0 physical memory is external memory not owned by LiteBox, similar to DMA/shared + /// physical memory. Safe physical pointer APIs access it by creating a temporary mapping, + /// acquiring the cooperating LiteBox physical range ownership lock, copying data to/from a + /// LiteBox-owned buffer with fallible raw-pointer copies, and unmapping immediately. They do not + /// create Rust references to the mapped VTL0 memory. Direct `vmap()` remains unsafe because it + /// exposes raw mapped memory to the caller. fn unmap_vtl0_pages( &self, page_addr: *const u8, @@ -1138,11 +1240,13 @@ fn is_contiguous(addrs: &[PhysPageAddr]) -> bool { } impl VmapManager for LinuxKernel { + type MapInfo = LvbsPhysPageMapInfo; + unsafe fn vmap( &self, pages: &PhysPageAddrArray, perms: PhysPageMapPermissions, - ) -> Result, PhysPointerError> { + ) -> Result { if pages.is_empty() { return Err(PhysPointerError::InvalidPhysicalAddress(0)); } @@ -1151,6 +1255,23 @@ impl VmapManager for LinuxKernel unimplemented!("ALIGN other than 4KiB is not supported yet"); } + // Reject duplicate page addresses for non-contiguous input. Contiguous input cannot + // repeat by construction. This upholds `acquire_phys_range_ownership`'s no-duplicate + // precondition so its drop path can correctly release every acquired range. + if !is_contiguous(pages) { + let mut seen = hashbrown::HashSet::with_capacity(pages.len()); + for page in pages { + if !seen.insert(page.as_usize()) { + return Err(PhysPointerError::DuplicatePhysicalAddress(page.as_usize())); + } + } + } + + // Acquire exclusive ownership of the physical ranges before installing the mapping. + // The returned guard is carried by `LvbsPhysPageMapInfo` and released on drop/`vunmap`, + // which is what makes the copy-based physical pointer reads/writes safe. + let ownership_guard = acquire_phys_range_ownership(pages)?; + // VTL0 memory must never be executable from VTL1 (DEP). let mut flags = PageTableFlags::PRESENT | PageTableFlags::NO_EXECUTE; if perms.contains(PhysPageMapPermissions::WRITE) { @@ -1179,10 +1300,11 @@ impl VmapManager for LinuxKernel .current_page_table() .map_phys_frame_range_direct(frame_range, flags, None) { - Ok(page_addr) => Ok(PhysPageMapInfo { - base: page_addr, - size: pages.len() * ALIGN, - }), + Ok(page_addr) => Ok(LvbsPhysPageMapInfo::new( + page_addr, + pages.len() * ALIGN, + ownership_guard, + )), Err(MapToError::PageAlreadyMapped(_)) => { Err(PhysPointerError::AlreadyMapped(pages[0].as_usize())) } @@ -1194,16 +1316,6 @@ impl VmapManager for LinuxKernel ), } } else { - // Reject duplicate page addresses - { - let mut seen = hashbrown::HashSet::with_capacity(pages.len()); - for page in pages { - if !seen.insert(page.as_usize()) { - return Err(PhysPointerError::DuplicatePhysicalAddress(page.as_usize())); - } - } - } - let frames: alloc::vec::Vec> = pages .iter() .map(|p| PhysFrame::containing_address(x86_64::PhysAddr::new(p.as_usize() as u64))) @@ -1228,10 +1340,11 @@ impl VmapManager for LinuxKernel .current_page_table() .map_non_contiguous_phys_frames(&frames, base_va, flags) { - Ok(page_addr) => Ok(PhysPageMapInfo { - base: page_addr, - size: pages.len() * ALIGN, - }), + Ok(page_addr) => Ok(LvbsPhysPageMapInfo::new( + page_addr, + pages.len() * ALIGN, + ownership_guard, + )), Err(e) => { let _ = vmap_allocator().unregister_allocation(base_va); match e { @@ -1250,25 +1363,46 @@ impl VmapManager for LinuxKernel } } - unsafe fn vunmap(&self, vmap_info: PhysPageMapInfo) -> Result<(), PhysPointerError> { + unsafe fn vunmap( + &self, + vmap_info: Self::MapInfo, + ) -> Result<(), (PhysPointerError, Self::MapInfo)> { if ALIGN != PAGE_SIZE { unimplemented!("ALIGN other than 4KiB is not supported yet"); } - let base_va = x86_64::VirtAddr::new(vmap_info.base as u64); + // Hold the map info in `ManuallyDrop` so its physical range ownership guard is only + // released once unmapping actually succeeds; on failure we hand it back to the caller. + let vmap_info = ManuallyDrop::new(vmap_info); + let base = vmap_info.base(); + let size = vmap_info.size(); + let base_va = x86_64::VirtAddr::new(base as u64); // Unmap the page table entries first. Only release the VA range back // to the allocator when unmapping succeeds; if it fails, stale PTE // entries remain and recycling the VA would cause collisions. - self.unmap_vtl0_pages(vmap_info.base, vmap_info.size) - .map_err(|_| PhysPointerError::Unmapped(vmap_info.base as usize))?; + if self.unmap_vtl0_pages(base, size).is_err() { + return Err(( + PhysPointerError::Unmapped(base as usize), + ManuallyDrop::into_inner(vmap_info), + )); + } - if crate::mm::vmap::is_vmap_address(base_va) { - crate::mm::vmap::vmap_allocator() + // PTEs are already cleared at this point, so the mapping is functionally gone + // and a retry would only re-fail against empty page-table entries. If the VA + // allocator's bookkeeping is inconsistent, surface it via `debug_assert!` and + // drop `vmap_info` to release the physical range ownership guard. The VA region + // is leaked but cannot be safely recycled. + let unregister_ok = !crate::mm::vmap::is_vmap_address(base_va) + || crate::mm::vmap::vmap_allocator() .unregister_allocation(base_va) - .ok_or(PhysPointerError::Unmapped(vmap_info.base as usize))?; - } + .is_some(); + debug_assert!( + unregister_ok, + "vmap allocator unregister failed at {base_va:?}", + ); + ManuallyDrop::into_inner(vmap_info); Ok(()) } diff --git a/litebox_platform_lvbs/src/mshv/ringbuffer.rs b/litebox_platform_lvbs/src/mshv/ringbuffer.rs index 150f5b1e0..ad732ac6b 100644 --- a/litebox_platform_lvbs/src/mshv/ringbuffer.rs +++ b/litebox_platform_lvbs/src/mshv/ringbuffer.rs @@ -92,7 +92,7 @@ fn write_fast(rb_pa: PhysAddr, size: usize, write_offset: usize, buf: &[u8]) -> let Ok(mut ptr) = Vtl0PhysMutPtr::::new(&span, in_page_offset) else { return write_offset; }; - if unsafe { ptr.write_slice_at_offset(0, buf) }.is_ok() { + if ptr.write_slice_at_offset(0, buf).is_ok() { (start + buf.len()) % size } else { write_offset @@ -109,7 +109,7 @@ fn write_fast(rb_pa: PhysAddr, size: usize, write_offset: usize, buf: &[u8]) -> fn write_slow(rb_pa: PhysAddr, size: usize, write_offset: usize, buf: &[u8]) -> usize { let write_slice = |pa: PhysAddr, slice: &[u8]| -> bool { Vtl0PhysMutPtr::::with_contiguous_pages(pa.as_u64().trunc(), slice.len()) - .and_then(|mut ptr| unsafe { ptr.write_slice_at_offset(0, slice) }) + .and_then(|mut ptr| ptr.write_slice_at_offset(0, slice)) .is_ok() }; diff --git a/litebox_platform_lvbs/src/mshv/vsm.rs b/litebox_platform_lvbs/src/mshv/vsm.rs index 7888d3ab2..f13b57f58 100644 --- a/litebox_platform_lvbs/src/mshv/vsm.rs +++ b/litebox_platform_lvbs/src/mshv/vsm.rs @@ -128,8 +128,9 @@ pub fn mshv_vsm_boot_aps(cpu_online_mask_pfn: u64) -> Result { cpu_online_mask_page_addr.as_u64().trunc(), ) .map_err(|_| VsmError::CpuOnlineMaskCopyFailed)?; - let cpu_mask = - unsafe { cpu_mask_ptr.read_at_offset(0) }.map_err(|_| VsmError::CpuOnlineMaskCopyFailed)?; + let cpu_mask = cpu_mask_ptr + .read_at_offset(0) + .map_err(|_| VsmError::CpuOnlineMaskCopyFailed)?; #[cfg(debug_assertions)] { @@ -848,7 +849,7 @@ fn copy_heki_patch_from_vtl0(patch_pa_0: u64, patch_pa_1: u64) -> Result::with_usize(patch_pa_0.as_u64().trunc()) .map_err(|_| VsmError::Vtl0CopyFailed)?; - unsafe { ptr.read_at_offset(0) } + ptr.read_at_offset(0) .map(|boxed| *boxed) .map_err(|_| VsmError::Vtl0CopyFailed) } else { @@ -865,10 +866,8 @@ fn copy_heki_patch_from_vtl0(patch_pa_0: u64, patch_pa_1: u64) -> Result Result<(), VsmError> { patch.len(), ) .map_err(|_| VsmError::Vtl0CopyFailed)?; - unsafe { ptr.write_slice_at_offset(0, patch) }.map_err(|_| VsmError::Vtl0CopyFailed)?; + ptr.write_slice_at_offset(0, patch) + .map_err(|_| VsmError::Vtl0CopyFailed)?; } else { let pages = [ PhysPageAddr::::new( @@ -922,7 +922,8 @@ fn apply_vtl0_text_patch(heki_patch: HekiPatch) -> Result<(), VsmError> { (heki_patch_pa_0 - heki_patch_pa_0.align_down(Size4KiB::SIZE)).trunc(), ) .map_err(|_| VsmError::Vtl0CopyFailed)?; - unsafe { ptr.write_slice_at_offset(0, patch) }.map_err(|_| VsmError::Vtl0CopyFailed)?; + ptr.write_slice_at_offset(0, patch) + .map_err(|_| VsmError::Vtl0CopyFailed)?; } Ok(()) } @@ -963,7 +964,8 @@ fn mshv_vsm_set_platform_root_key(key_pa: u64) -> Result { let mut key_ptr = Vtl0PhysConstPtr::::with_contiguous_pages(key_pa.as_u64().trunc(), PRK_LEN) .map_err(|_| VsmError::Vtl0CopyFailed)?; - unsafe { key_ptr.read_slice_at_offset(0, &mut *keybuf) } + key_ptr + .read_slice_at_offset(0, &mut *keybuf) .map_err(|_| VsmError::Vtl0CopyFailed)?; set_platform_root_key(&*keybuf); Ok(0) @@ -1391,7 +1393,7 @@ fn copy_heki_pages_from_vtl0(pa: u64, nranges: u64) -> Option> { } let mut ptr = Vtl0PhysConstPtr::::with_usize(cur_pa.as_u64().trunc()).ok()?; - let heki_page = unsafe { ptr.read_at_offset(0) }.ok()?; + let heki_page = ptr.read_at_offset(0).ok()?; if !heki_page.is_valid() { return None; } @@ -1675,7 +1677,10 @@ impl MemoryContainer { let old_len = self.buf.len(); self.buf.resize(old_len + bytes_to_copy, 0); - if unsafe { ptr.read_slice_at_offset(0, &mut self.buf[old_len..]) }.is_err() { + if ptr + .read_slice_at_offset(0, &mut self.buf[old_len..]) + .is_err() + { self.buf.truncate(old_len); return Err(MemoryContainerError::CopyFromVtl0Failed); } diff --git a/litebox_runner_lvbs/src/lib.rs b/litebox_runner_lvbs/src/lib.rs index 0ae319e59..42a01372b 100644 --- a/litebox_runner_lvbs/src/lib.rs +++ b/litebox_runner_lvbs/src/lib.rs @@ -282,8 +282,8 @@ fn optee_smc_handler_entry_inner( // All OP-TEE return codes (success or error) are delivered via smc_args.args[0]. let mut smc_args_ptr = NormalWorldMutPtr::::with_usize(smc_args_addr) .map_err(|_| litebox_common_linux::errno::Errno::EINVAL)?; - // SAFETY: The SMC args are written back to normal world memory. - unsafe { smc_args_ptr.write_at_offset(0, smc_args_updated) } + smc_args_ptr + .write_at_offset(0, smc_args_updated) .map_err(|_| litebox_common_linux::errno::Errno::EFAULT)?; Ok(0) } @@ -427,8 +427,7 @@ fn optee_smc_handler(smc_args_addr: usize) -> OpteeSmcArgs { else { return make_error_response(OpteeSmcReturnCode::EBadAddr); }; - // SAFETY: The SMC args are read from normal world memory into an owned copy. - let Ok(mut smc_args) = (unsafe { smc_args_ptr.read_at_offset(0) }) else { + let Ok(mut smc_args) = smc_args_ptr.read_at_offset(0) else { return make_error_response(OpteeSmcReturnCode::EBadAddr); }; let Ok(smc_result) = handle_optee_smc_args(&mut smc_args) else { @@ -1301,9 +1300,7 @@ fn write_msg_args_to_normal_world( msg_args_phys_addr.trunc(), msg_args_size, )?; - // SAFETY: Writing msg_args back to normal world memory at a valid physical address. - // The blob contains the serialized variable-length optee_msg_arg structure(s). - unsafe { ptr.write_slice_at_offset(0, &blob) }?; + ptr.write_slice_at_offset(0, &blob)?; Ok(()) } @@ -1327,9 +1324,7 @@ fn write_non_ta_msg_args_to_normal_world( msg_args_phys_addr.trunc(), msg_args_size, )?; - // SAFETY: Writing msg_args back to normal world memory at a valid physical address. - // The blob contains the serialized variable-length optee_msg_arg structure(s). - unsafe { ptr.write_slice_at_offset(0, &blob) }?; + ptr.write_slice_at_offset(0, &blob)?; Ok(()) } @@ -1356,9 +1351,7 @@ fn write_rpc_args_to_normal_world( .checked_add(msg_args_size) .ok_or(OpteeSmcReturnCode::EBadAddr)?; // RPC args are placed right after the main msg_args blob let mut ptr = NormalWorldMutPtr::::with_contiguous_pages(rpc_pa, rpc_args_size)?; - // SAFETY: Writing rpc_args back to normal world memory at a valid physical address. - // The blob contains the serialized variable-length optee_msg_arg structure(s). - unsafe { ptr.write_slice_at_offset(0, &blob) }?; + ptr.write_slice_at_offset(0, &blob)?; Ok(()) } diff --git a/litebox_shim_optee/src/msg_handler.rs b/litebox_shim_optee/src/msg_handler.rs index b1c125101..fecd810a0 100644 --- a/litebox_shim_optee/src/msg_handler.rs +++ b/litebox_shim_optee/src/msg_handler.rs @@ -184,7 +184,8 @@ pub fn read_optee_msg_args_from_phys( let mut blob_ptr = NormalWorldConstPtr::::with_contiguous_pages(phys_addr, copy_size) .map_err(|_| OpteeSmcReturnCode::EBadAddr)?; - unsafe { blob_ptr.read_slice_at_offset(0, &mut blob) } + blob_ptr + .read_slice_at_offset(0, &mut blob) .map_err(|_| OpteeSmcReturnCode::EBadAddr)?; parse_optee_msg_args(&blob, has_rpc_arg) @@ -695,10 +696,8 @@ impl ShmInfo { return Err(OpteeSmcReturnCode::EBadAddr); } let mut ptr = NormalWorldConstPtr::::new(&self.page_addrs, self.page_offset)?; - // SAFETY: bounds validated above; copy lands in a buffer owned by LiteBox to avoid TOCTOU issues. - unsafe { - ptr.read_slice_at_offset(offset, buffer)?; - } + // Copy lands in a buffer owned by LiteBox to avoid TOCTOU issues. + ptr.read_slice_at_offset(offset, buffer)?; Ok(()) } @@ -710,10 +709,8 @@ impl ShmInfo { return Err(OpteeSmcReturnCode::EBadAddr); } let mut ptr = NormalWorldMutPtr::::new(&self.page_addrs, self.page_offset)?; - // SAFETY: bounds validated above; data comes from a buffer owned by LiteBox. - unsafe { - ptr.write_slice_at_offset(0, buffer)?; - } + // Data comes from a buffer owned by LiteBox. + ptr.write_slice_at_offset(0, buffer)?; Ok(()) } } @@ -792,8 +789,9 @@ impl ShmRefMap { visited_pages_data.insert(cur_addr); let mut cur_ptr = NormalWorldConstPtr::::with_usize(cur_addr) .map_err(|_| OpteeSmcReturnCode::EBadAddr)?; - let pages_data = - unsafe { cur_ptr.read_at_offset(0) }.map_err(|_| OpteeSmcReturnCode::EBadAddr)?; + let pages_data = cur_ptr + .read_at_offset(0) + .map_err(|_| OpteeSmcReturnCode::EBadAddr)?; let pages_len_before = pages.len(); for page in &pages_data.pages_list { if *page == 0 || pages.len() == num_pages {