Skip to content

Commit c896a2a

Browse files
committed
refactor: address PR 1270 review feedback for GuestCounter
Address review comments from syntactically and jsturtevant: - Doc: replace "backed by" with "exposed to the guest via" (syntactically) - Doc: remove "confused-deputy" framing; clarify that the host never reads back from guest memory so a malicious guest cannot influence the host's view (syntactically) - Doc: add Implementation Note explaining why raw write_volatile is used instead of SharedMemory methods — GuestCounter is created on UninitializedSandbox (ExclusiveSharedMemory) but must survive across evolve() into MultiUseSandbox (HostSharedMemory); the pointer stays valid because evolve() internally clones the Arc<HostMapping> (syntactically) - Doc: document single-instance requirement — multiple GuestCounter instances would cause cached values to diverge (jsturtevant) - Safety: move "must not outlive sandbox" and single-instance requirements into a structured # Safety doc on the constructor (jsturtevant) - Bounds check: remove sizeof(u64) addend since the offset constant (0x1008) already statically accounts for it (syntactically) - Remove unused SandboxMemoryLayout import Signed-off-by: danbugs <danilochiarlone@gmail.com>
1 parent c9c56f6 commit c896a2a

1 file changed

Lines changed: 31 additions & 13 deletions

File tree

src/hyperlight_host/src/sandbox/uninitialized.rs

Lines changed: 31 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -56,17 +56,33 @@ pub(crate) struct SandboxRuntimeConfig {
5656
pub(crate) entry_point: Option<u64>,
5757
}
5858

59-
/// A host-authoritative shared counter backed by a u64 in guest scratch memory.
59+
/// A host-authoritative shared counter exposed to the guest via a `u64`
60+
/// in guest scratch memory.
6061
///
6162
/// Created via [`UninitializedSandbox::guest_counter()`]. The host owns
6263
/// the counter value and is the only writer: [`increment()`](Self::increment)
6364
/// and [`decrement()`](Self::decrement) update the cached value and issue a
6465
/// single `write_volatile` to shared memory. [`value()`](Self::value) returns
65-
/// the cached value — the host never reads back from guest memory, preventing
66-
/// a confused-deputy attack where a malicious guest could manipulate the counter.
66+
/// the cached value — the host never reads back from guest memory, so a
67+
/// malicious guest cannot influence the host's view of the counter.
6768
///
6869
/// Thread safety is provided by an internal `Mutex`, so `increment()` and
6970
/// `decrement()` take `&self` rather than `&mut self`.
71+
///
72+
/// # Implementation note
73+
///
74+
/// `GuestCounter` uses raw `write_volatile` instead of going through
75+
/// [`SharedMemory`] methods because it is created on an
76+
/// [`UninitializedSandbox`] (which holds `ExclusiveSharedMemory`), but
77+
/// must survive across [`evolve()`](UninitializedSandbox::evolve) into
78+
/// `MultiUseSandbox` (which holds `HostSharedMemory`). The pointer
79+
/// remains valid because `evolve()` internally clones the backing
80+
/// `Arc<HostMapping>` into both `HostSharedMemory` and
81+
/// `GuestSharedMemory`, keeping the mmap alive.
82+
///
83+
/// Only **one** `GuestCounter` should be created per sandbox. Creating
84+
/// multiple instances is safe from a memory perspective, but the
85+
/// internal cached values will diverge, leading to incorrect writes.
7086
#[cfg(feature = "nanvix-unstable")]
7187
pub struct GuestCounter {
7288
inner: Mutex<GuestCounterInner>,
@@ -240,25 +256,27 @@ impl<'a> From<GuestBinary<'a>> for GuestEnvironment<'a, '_> {
240256
}
241257

242258
impl UninitializedSandbox {
243-
/// Creates a [`GuestCounter`] backed by a u64 at a fixed offset in
244-
/// scratch memory.
259+
/// Creates a [`GuestCounter`] at a fixed offset in scratch memory.
245260
///
246-
/// The counter lives at `SCRATCH_TOP_GUEST_COUNTER_OFFSET` from
247-
/// the top of scratch, so both host and guest can locate it without
248-
/// an explicit GPA parameter.
261+
/// The counter lives at `SCRATCH_TOP_GUEST_COUNTER_OFFSET` bytes from
262+
/// the top of scratch memory, so both host and guest can locate it
263+
/// without an explicit GPA parameter.
249264
///
250265
/// # Safety
251266
///
252-
/// The caller must ensure the returned `GuestCounter` does not
253-
/// outlive the sandbox's underlying shared memory mapping (which
254-
/// stays alive through `evolve()` into `MultiUseSandbox`).
267+
/// The caller must ensure:
268+
/// - The returned `GuestCounter` does not outlive the sandbox's
269+
/// underlying shared memory mapping (which stays alive through
270+
/// `evolve()` into `MultiUseSandbox`).
271+
/// - Only **one** `GuestCounter` is created per sandbox. Multiple
272+
/// instances are memory-safe but their cached values will diverge,
273+
/// leading to incorrect writes.
255274
#[cfg(feature = "nanvix-unstable")]
256275
pub unsafe fn guest_counter(&mut self) -> Result<GuestCounter> {
257276
use hyperlight_common::layout::SCRATCH_TOP_GUEST_COUNTER_OFFSET;
258277

259278
let scratch_size = self.mgr.scratch_mem.mem_size();
260-
if (SCRATCH_TOP_GUEST_COUNTER_OFFSET as usize) + core::mem::size_of::<u64>() > scratch_size
261-
{
279+
if (SCRATCH_TOP_GUEST_COUNTER_OFFSET as usize) > scratch_size {
262280
return Err(new_error!(
263281
"scratch memory too small for guest counter (size {:#x}, need offset {:#x})",
264282
scratch_size,

0 commit comments

Comments
 (0)