Use physical pointer abstraction in LVBS by sangho2 · Pull Request #817 · microsoft/litebox

sangho2 · 2026-04-28T16:24:10Z

This PR lets LVBS use physical pointer abstraction to access VTL0 memory. This is equivalent to the OP-TEE shim's normal-world memory access such that it is not only safer than the legacy memory copy functions but also supporting non-contiguous physical pages. This PR and physical address range lock (next PR) will enable memory-safe VTL0 memory access (i.e., drop unsafe from read/write methods).

wdcui

LGTM. I left some comments. Thanks!

wdcui · 2026-06-11T22:48:11Z

    where
        T: FromBytes,
    {
+        if values.is_empty() {


Is count the offset?

Yes. This aligns with the litebox's RawConstPointer.

wdcui · 2026-06-11T22:52:38Z

+    const MAX_SPAN_PAGES: usize = 16;
+    // If `buf` would force the start page into the span twice, vmap rejects the
+    // duplicate. Hand off to the two-write slow path before truncating `buf`.
+    if buf.len() < size && write_offset % PAGE_SIZE + buf.len() > size {


I don't understand this line. Why do we modulo PAGE_SIZE then add buf.len() to compare with size?

Yes, this is confusing. Basically, it figures out whether wraparound requires duplicated mapping, which our page mapper doesn't allow.

PAGE_SIZE = 4096 size = 4096 write_offset = 4090 buf.len() = 16 Evaluate the condition: buf.len() < size 16 < 4096 true

Something like below would be more readable. Let me revise it.

let page_count = size / PAGE_SIZE; let page_offset = write_offset % PAGE_SIZE; let span_pages = (page_offset + buf.len()).div_ceil(PAGE_SIZE); if span_pages > page_count { return write_slow(rb_pa, size, write_offset, buf); }

lschuermann

I took a pass over the entirety of physical_pointers.rs, not just the diff of this PR. In summary, I think this is close to a neat and (in the future) sound abstraction with rich pointer semantics over dynamically mapped physical memory.

Happy to discuss my comments below, I'll go over the remaining changes soon. I don't think any of these is critical to be addressed in this PR, instead I'm happy to implement the changes in a follow-up PR!

lschuermann · 2026-06-15T22:01:08Z

 #[repr(C)]
-pub struct PhysMutPtr<T: Clone, const ALIGN: usize> {
+pub struct PhysMutPtr<T: Clone, const ALIGN: usize, V: GlobalVmapManager<ALIGN>> {
    pages: alloc::boxed::Box<[PhysPageAddr<ALIGN>]>,


pages is currently effectively an array of usizes with alignment constraints. For strict provenance support, this should be wrapping raw pointers instead. Not critical here.

lschuermann · 2026-06-15T22:01:50Z

+        if core::mem::size_of::<T>() == 0 {
+            return Err(PhysPointerError::UnsupportedZeroSizedType);
+        }


I'm wondering why ZSTs are not supported here. Given that this type can already be used to read n items out of foreign memory, shouldn't the implementation for ZSTs be "easy" in the sense that it would just allow reading / writing an infinite amount of ZSTs to/from any valid foreign address?

Perhaps there's an edge case that I'm missing here. But it would be nice if we could get rid of this runtime error. :)

lschuermann · 2026-06-15T22:32:58Z

I think this function can be made safe (along with all the other readers and writers in the file).

While is true that this can read corrupted memory---memcpy_fallible can race with another writer, but is implemented in assembly and therefore outside the Rust Abstract Machine and allowed to race---because T: FromBytes, reading any corrupted memory is actually safe. There's a concern about semantic correctness, but not about Rust soundness.

What would be unsound, however, is if these read and write operations are operating on memory that overlaps with any valid Rust allocation. That's discharged through the V::manager().validate_owned() call (provided it gets turned into an unsafe trait and its # Safety doc gets extended).

Addressed in #824.

lschuermann · 2026-06-15T22:40:37Z

    pub unsafe fn read_slice_at_offset(
        &mut self,
        count: usize,
        values: &mut [T],


We'll need to carefully reason about why this is safe. It could potentially leave T in a partially copied state. Given that it's T: FromBytes this is probably fine.

lschuermann · 2026-06-15T23:25:30Z

-impl<T: Clone, const ALIGN: usize> PhysMutPtr<T, ALIGN> {
+impl<T: Clone, const ALIGN: usize, V> PhysMutPtr<T, ALIGN, V>
+where
+    V: GlobalVmapManager<ALIGN>,


The soundness of this implementation relies on a correct implementation of the GlobalVmapManager, specifically that it must return the appropriate, active VmapManager for the current system. That means that GlobalVmapManager should be an unsafe trait. -> #824

~~Similarly, the soundness of this implementation relies on the fact that V::validate_unowned is correct. Therefore, VmapManager should also be an unsafe trait.~~ -> #824

The documentation of validate_unowned should additionally carry a safety invariant that states that any address range for which this function every returns Ok(()) must never be used to store any Rust allocation (i.e., never be part of the Rust heap) in the future. Otherwise, the soundness implications of https://github.com/microsoft/litebox/pull/817/changes#r3417155237 apply.

lschuermann · 2026-06-15T23:34:55Z

        if offset >= ALIGN {
            return Err(PhysPointerError::InvalidBaseOffset(offset, ALIGN));
        }


Should the function ensure that (ALIGN + offset) % std::mem::align_of::<T>() == 0? I.e., that Ts in this memory region are located at well-aligned pointers?

The answer is probably: no. This function uses an unaligned memcpy underneath; it never creates an (ephemeral) Rust reference to any foreign memory. Also, the above restriction would mean that Ts with large alignment constraints would be unsupported.

The constructor should then get a comment documenting that reads/writes of T don't have to be aligned, and why that's safe.

lschuermann · 2026-06-15T23:35:48Z

        if size < core::mem::size_of::<T>() {
            return Err(PhysPointerError::InsufficientPhysicalPages(
                size,
                core::mem::size_of::<T>(),
            ));
        }


Why this early error path? Given that the address range can contain a slice of types T, wouldn't it be easier to also allow constructing PhysMutPtr if zero instances of the type fit? Is there a good reason to ensure that this can never capture "zero elements of type T"?

lschuermann · 2026-06-15T23:37:01Z

    /// Create a new `PhysMutPtr` from the given contiguous physical address and length.
    ///
    /// This is a shortcut for
    /// `PhysMutPtr::new([align_down(pa), align_down(pa) + ALIGN, ..., align_up(pa + bytes) - ALIGN], pa % ALIGN)`.
    /// This function assumes that `pa`, ..., `pa+bytes` are both physically and virtually contiguous. If not,
    /// later accesses through `PhysMutPtr` may read/write data in a wrong order.
    pub fn with_contiguous_pages(pa: usize, bytes: usize) -> Result<Self, PhysPointerError> {


Should clarify that "this function assumes" means "this function will return an error if not".

lschuermann · 2026-06-15T23:53:25Z

This is the thorny part.

Something that would be a big soundness risk here: if another core unmaps this memory in parallel, but the memory is then re-mapped carrying some Rust allocation, then this write races with Rust accesses.

We can shift the burden of proof onto GlobalVmapManager and (by extension) VmapManager. By having them both be unsafe traits, and extending the safety documentation to promise that if it ever returns Ok(()) for validate_unowned this memory will never used as backing for any Rust allocation in the future, this asm!-backed memcpy can be unconditionally safe.

That is, of course, ignoring semantic correctness constraints that this function should document. But those don't affect the soundness of this function.

I guess, another way to ensure that this does not end up touching Rust memory would be adding a guard to the TLB shootdown handler, which detects whether the local core is currently in the memcpy_fallible assembly and checks if the memcpy would be affected by the new page table state. This could be used to guarantee that all concurrent memcpys operating on a now-revoked page terminate, before the virtual address is ever reused.

That seems hard though. It might be easier and cleaner to simply guarantee that such VAs are never re-used for Rust allocations.

lschuermann

Reviewed the rest of the PR. I realize some of my feedback might be obsolete due to #824, so I'll check that out next. Should I create a PR to address the left-over comments targeting this same sanghle/lvbs/vmap_copy branch?

lschuermann · 2026-06-16T16:53:50Z

 static RINGBUFFER_ONCE: Once<Mutex<RingBuffer>> = Once::new();
 pub(crate) fn set_ringbuffer(pa: PhysAddr, size: usize) -> &'static Mutex<RingBuffer> {
    RINGBUFFER_ONCE.call_once(|| {
        let ring_buffer = RingBuffer::new(pa, size);
        Mutex::new(ring_buffer)
    })
 }

 pub(crate) fn ringbuffer() -> Option<&'static Mutex<RingBuffer>> {
    RINGBUFFER_ONCE.get()
 }

 impl fmt::Write for RingBuffer {
    fn write_str(&mut self, s: &str) -> fmt::Result {
        self.write(s.as_bytes());
        Ok(())
    }
 }


I was initially confused by this, because it seemed weird that we'd just have one instance of this generic RingBuffer. Maybe it could help calling this DEBUG_RINGBUFFER?

lschuermann · 2026-06-16T16:55:25Z

Is there a documentation of the expected ring buffer semantics somewhere? I can see it's used for debug printing, but don't know what the consumer side looks like. Depending on the consumer, some of the semantics seem interesting (like, it resets the write_offset to zero after a large write).

lschuermann · 2026-06-16T17:01:17Z

    fn validate_unowned(&self, _pages: &PhysPageAddrArray<ALIGN>) -> Result<(), PhysPointerError> {
        Ok(())
    }


This default implementation should probably be removed. For (unsafe) traits that are load-bearing for soundness, there should either not be default implementations, or they should be conservative and unconditionally sound. If an implementer forgets this validate_unowned method, it will cause any address to be marked as unowned and eligible to be written to through the PhysPtr abstraction.

lschuermann · 2026-06-16T17:01:39Z

    ) -> Result<(), PhysPointerError> {
        Ok(())
    }


Same here, default impl should be removed or made conservative.

lschuermann · 2026-06-16T17:33:34Z

 /// This function apply the given `HekiPatch` patch data to VTL0 text.
 /// It assumes the caller has confirmed the validity of `HekiPatch` by invoking the `is_valid()` member function.
 fn apply_vtl0_text_patch(heki_patch: HekiPatch) -> Result<(), VsmError> {


Worth making this into a type state, perhaps?

I don't think heki_patch.is_valid() == false will allow this function to invoke undefined behavior. But if it does, it would need to either perform this check internally or be marked unsafe. A type state would avoid this.

lschuermann · 2026-06-16T17:42:15Z

+#[derive(Clone, Copy, Debug, Default)]
+pub struct Vmap;


Suggested change

#[derive(Clone, Copy, Debug, Default)]

pub struct Vmap;

#[derive(Clone, Copy, Debug, Default)]

pub enum Vmap {};

Might as well make this a variant-less enum? That's the idiomatic way to have the compiler guarantee this type is erased during compilation and not present in the final binary (or any artifacts associated with it, like vtables).

lschuermann · 2026-06-16T17:42:51Z

+#[derive(Clone, Copy, Debug, Default)]
+pub struct Vmap;


Same here, maybe worth making variant-less enum instead.

This PR refactors the physical pointer API. --------- Co-authored-by: Sangho Lee <sanghle@microsoft.com>

github-actions · 2026-06-18T17:01:46Z

🤖 SemverChecks 🤖 ⚠️ Potential breaking API changes detected ⚠️

Click for details

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_missing.ron

Failed in:
  struct litebox_common_linux::vmap::PhysPageMapInfo, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_common_linux/src/vmap.rs:95

--- failure trait_associated_type_added: non-sealed public trait added associated type without default value ---

Description:
A non-sealed trait has gained an associated type without a default value, which breaks downstream implementations of the trait
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#trait-new-item-no-default
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/trait_associated_type_added.ron

Failed in:
  trait associated type litebox_common_linux::vmap::VmapManager::MapInfo in file /home/runner/work/litebox/litebox/litebox_common_linux/src/vmap.rs:22

--- failure trait_unsafe_added: pub trait became unsafe ---

Description:
A publicly-visible trait became `unsafe`, so implementing it now requires an `unsafe impl` block.
        ref: https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html#implementing-an-unsafe-trait
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/trait_unsafe_added.ron

Failed in:
  trait litebox_common_linux::vmap::VmapManager in file /home/runner/work/litebox/litebox/litebox_common_linux/src/vmap.rs:19

--- failure feature_missing: package feature removed or renamed ---

Description:
A feature has been removed from this package's Cargo.toml. This will break downstream crates which enable that feature.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#cargo-feature-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/feature_missing.ron

Failed in:
  feature optee_syscall in the package's Cargo.toml

--- failure inherent_method_missing: pub method removed or renamed ---

Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/inherent_method_missing.ron

Failed in:
  LinuxKernel::copy_from_vtl0_phys, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_platform_lvbs/src/lib.rs:731
  LinuxKernel::copy_to_vtl0_phys, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_platform_lvbs/src/lib.rs:771
  LinuxKernel::copy_slice_to_vtl0_phys, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_platform_lvbs/src/lib.rs:806
  LinuxKernel::copy_slice_from_vtl0_phys, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_platform_lvbs/src/lib.rs:841

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/module_missing.ron

Failed in:
  mod litebox_shim_optee::ptr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_shim_optee/src/ptr.rs:4

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/struct_missing.ron

Failed in:
  struct litebox_shim_optee::ptr::PhysConstPtr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_shim_optee/src/ptr.rs:499
  struct litebox_shim_optee::NormalWorldConstPtr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_shim_optee/src/ptr.rs:499
  struct litebox_shim_optee::ptr::PhysMutPtr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_shim_optee/src/ptr.rs:107
  struct litebox_shim_optee::NormalWorldMutPtr, previously in file /home/runner/work/litebox/litebox/target/semver-checks/git-main/32beb58ba4b7135e6d38cb66b4ba0700ad7a89d9/litebox_shim_optee/src/ptr.rs:107

sangho2 added the must-not-merge:prototype An experimental/proof-of-concept PR that must not be merged. label Apr 28, 2026

sangho2 force-pushed the sanghle/lvbs/vmap_copy branch 2 times, most recently from 29c61b5 to e42c3a0 Compare May 1, 2026 15:51

sangho2 changed the title ~~[DRAFT] Use physical pointer abstraction in HVCI/HEKI~~ Use physical pointer abstraction in HVCI/HEKI May 1, 2026

sangho2 changed the title ~~Use physical pointer abstraction in HVCI/HEKI~~ Use safe physical pointer abstraction in HVCI/HEKI May 1, 2026

sangho2 removed the must-not-merge:prototype An experimental/proof-of-concept PR that must not be merged. label May 1, 2026

sangho2 marked this pull request as ready for review May 1, 2026 20:11

sangho2 added must-not-merge:prototype An experimental/proof-of-concept PR that must not be merged. and removed must-not-merge:prototype An experimental/proof-of-concept PR that must not be merged. labels May 1, 2026

sangho2 changed the title ~~Use safe physical pointer abstraction in HVCI/HEKI~~ Use physical pointer abstraction in HVCI/HEKI May 1, 2026

sangho2 force-pushed the sanghle/lvbs/vmap_copy branch from 8b9b1f7 to 3a833f2 Compare May 15, 2026 16:40

sangho2 changed the title ~~Use physical pointer abstraction in HVCI/HEKI~~ Use physical pointer abstraction in LVBS May 15, 2026

wdcui reviewed May 15, 2026

View reviewed changes

Comment thread litebox_common_linux/src/physical_pointers.rs Outdated

wdcui reviewed May 15, 2026

View reviewed changes

Comment thread litebox_platform_lvbs/src/mshv/vsm.rs

sangho2 added must-not-merge:undergoing-restructuring Known deeper set of changes are happening on this PR before it is mergeable again and removed must-not-merge:undergoing-restructuring Known deeper set of changes are happening on this PR before it is mergeable again labels May 15, 2026

sangho2 force-pushed the sanghle/lvbs/vmap_copy branch 2 times, most recently from c6fd071 to 24e1d87 Compare May 21, 2026 20:58

sangho2 force-pushed the sanghle/lvbs/vmap_copy branch from 24e1d87 to 9b0f0f0 Compare May 29, 2026 02:48

sangho2 mentioned this pull request May 29, 2026

Improve the safety and security of the Physical Pointer Abstraction #642

Open

wdcui approved these changes Jun 11, 2026

View reviewed changes

lschuermann mentioned this pull request Jun 15, 2026

litebox_{platform_lvbs,shim_optee}: simplify box_new_zeroed #919

Open

sangho2 added the must-not-merge:undergoing-restructuring Known deeper set of changes are happening on this PR before it is mergeable again label Jun 15, 2026

lschuermann reviewed Jun 15, 2026

View reviewed changes

lschuermann reviewed Jun 16, 2026

View reviewed changes

lschuermann mentioned this pull request Jun 16, 2026

Refactor physical pointer API #824

Merged

sangho2 mentioned this pull request Jun 18, 2026

Rollback support for critical kernel operations #936

Open

Sangho Lee added 2 commits June 18, 2026 16:05

use physcal pointer abstraction in HVCI/HEKI

e9bcdb8

refactoring: associated func and phantom data

2b36848

Sangho Lee and others added 16 commits June 18, 2026 16:05

use physical pointer's non-contiguous read/write

1025056

refactoring

ab131ca

ZST

6a26c58

revert

f6e9a2e

refactoring

6e3310d

refactoring

b7bb179

format

6b758af

rebase

ec77563

ringbuffer fast path

d5476b1

minor

2bac44f

collapse apply_vtl0_text_patch

095edef

simplify traits

93c4f17

rebase

51cf3dc

refactoring

361f944

rebase

5a18977

Refactor physical pointer API (#824)

0e0e307

This PR refactors the physical pointer API. --------- Co-authored-by: Sangho Lee <sanghle@microsoft.com>

sangho2 force-pushed the sanghle/lvbs/vmap_copy branch from fefb293 to 0e0e307 Compare June 18, 2026 16:12

fix rebase issue

146da5e

Conversation

sangho2 commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wdcui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lschuermann Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 18, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sangho2 commented Apr 28, 2026 •

edited

Loading

lschuermann Jun 15, 2026 •

edited

Loading