Skip to content
Open
12 changes: 11 additions & 1 deletion Justfile
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ build target=default-target:
{{ cargo-cmd }} build --profile={{ if target == "debug" { "dev" } else { target } }} {{ target-triple-flag }}

# build testing guest binaries
guests: build-and-move-rust-guests build-and-move-c-guests
guests: build-and-move-rust-guests-non-pie build-and-move-rust-guests build-and-move-c-guests

ensure-cargo-hyperlight:
cargo install --locked cargo-hyperlight
Expand All @@ -69,6 +69,16 @@ build-rust-guests target=default-target features="": (witguest-wit) (ensure-carg
build-and-move-rust-guests: (build-rust-guests "debug") (move-rust-guests "debug") (build-rust-guests "release") (move-rust-guests "release")
build-and-move-c-guests: (build-c-guests "debug") (move-c-guests "debug") (build-c-guests "release") (move-c-guests "release")

# Build non-PIE variants of rust guests for testing ELF VA mapping
build-rust-guests-non-pie target=default-target: (ensure-cargo-hyperlight)
{{ if os() == "windows" { "$env:RUSTFLAGS='-C relocation-model=static -C link-args=--no-pie -C link-args=--image-base=0x200000';" } else { "" } }} cd src/tests/rust_guests/simpleguest && {{ if os() == "windows" { "" } else { "RUSTFLAGS='-C relocation-model=static -C link-args=--no-pie -C link-args=--image-base=0x200000'" } }} cargo hyperlight build --profile={{ if target == "debug" { "dev" } else { target } }}

@move-rust-guests-non-pie target=default-target:
{{ if os() == "windows" { "New-Item -ItemType Directory -Path " + rust_guests_bin_dir + "/" + target + "/non_pie -Force | Out-Null" } else { "mkdir -p " + rust_guests_bin_dir + "/" + target + "/non_pie" } }}
cp {{ simpleguest_source }}/{{ target }}/simpleguest {{ rust_guests_bin_dir }}/{{ target }}/non_pie/

build-and-move-rust-guests-non-pie: (build-rust-guests-non-pie "debug") (move-rust-guests-non-pie "debug") (build-rust-guests-non-pie "release") (move-rust-guests-non-pie "release")

clean: clean-rust

clean-rust:
Expand Down
38 changes: 35 additions & 3 deletions src/hyperlight_host/src/sandbox/snapshot/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ use crate::Result;
use crate::hypervisor::regs::CommonSpecialRegisters;
use crate::mem::exe::{ExeInfo, LoadInfo};
use crate::mem::layout::SandboxMemoryLayout;
use crate::mem::memory_region::{GuestMemoryRegion, MemoryRegion, MemoryRegionFlags};
use crate::mem::memory_region::{
GuestMemoryRegion, MemoryRegion, MemoryRegionFlags, MemoryRegionType,
};
use crate::mem::mgr::{GuestPageTableBuffer, SnapshotSharedMemory};
use crate::mem::shared_mem::{ReadonlySharedMemory, SharedMemory};
use crate::sandbox::SandboxConfiguration;
Expand Down Expand Up @@ -308,6 +310,12 @@ impl Snapshot {
let base_va = exe_info.base_va();
let entrypoint_va: u64 = exe_info.entrypoint().into();

// Determine the virtual base address for the code region.
// For non-PIE binaries (base_va > 0), the code should appear at the
// ELF's declared virtual address. For PIE binaries (base_va == 0),
// we use the physical load address (identity mapping).
let code_virt_base = if base_va > 0 { base_va } else { load_addr };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this choice be made based on whether the executable has .et_type = ET_DYN vs ET_EXEC, rather than the first phdr VA?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed - now using elf.header.e_type == ET_DYN via an is_pie() method on ExeInfo/ElfInfo instead of checking base_va > 0. Much cleaner semantically.


let mut memory = vec![0; layout.get_memory_size()?];

let load_info = exe_info.load(
Expand Down Expand Up @@ -340,9 +348,25 @@ impl Snapshot {
executable,
})
};

// For the code region, use code_virt_base as the GVA.
// For non-PIE this is the ELF's declared base VA (non-identity mapping).
// For PIE this should equal the GPA (identity mapping).
let virt_base = if rgn.region_type == MemoryRegionType::Code {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should have some check that the executable mapping here does not conflict with any other mappings.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - added a conflict check that verifies the non-PIE code VA range doesn't overlap with any other memory region. This actually caught a real bug: the original --image-base=0x200000 produced a binary that extended into the PEB region. Bumped the base to 0x1000000 (16MB) to avoid this.

if base_va == 0 {
assert_eq!(
code_virt_base, rgn.guest_region.start as u64,
"PIE code region should be identity-mapped"
);
}
code_virt_base
} else {
rgn.guest_region.start as u64
};
Comment thread
cshung marked this conversation as resolved.

let mapping = Mapping {
phys_base: rgn.guest_region.start as u64,
virt_base: rgn.guest_region.start as u64,
virt_base,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach probably works for now, especially with executables where all the segments are contiguous. One other thing that we could do while we are already touching this code is switch to mapping the ELF a bit more "properly", by copying every section copied by a segment into the snapshot contiguously (similarly to how it is being done now, but without using phdr.p_vaddr, so that there is no risk of large gaps appearing for no reason) and then making one of these vmem::map calls for each segment (~= PT_LOAD program header). In addition to being more efficient if we encounter a file with gaps between segments (at least a few kilobytes of gaps often exist due to page-aligning the phdr VAs), this would allow us to have better default permissions in the guest page tables by using the permission information in phdr.p_flags. Probably the way to do that would be to remove this mapping of this region entirely and change the exe interface in exe.rs/implementation in elf.rs to replace load/load_at with functions that take a mapping operations and do the mapping as they walk over the phdrs. That would be my slight preference/how I would do it, but I will leave it up to you whether or not to pursue it, since it is a nontrivially-larger change.

(Unrelated, not-really-review note: the need to fix up the physical vs virtual discontinuity is continuing a trend of several things that makes me slightly question the actual utility of SandboxMemoryLayout::get_memory_regions. It might be worth thinking about whether it makes sense to get rid of that / some more bits of SandboxMemoryLayout at some point, but that's definitely not a discussion for this PR).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed this would be the more proper approach. I'll leave it for a follow-up since it's a larger change, but it's on the radar.

len: rgn.guest_region.len() as u64,
kind,
user_accessible: false,
Expand All @@ -361,13 +385,21 @@ impl Snapshot {
- hyperlight_common::layout::SCRATCH_TOP_EXN_STACK_OFFSET
+ 1;

let entrypoint_offset = entrypoint_va.checked_sub(base_va).ok_or_else(|| {
crate::new_error!(
"ELF entrypoint VA ({:#x}) is below base VA ({:#x})",
entrypoint_va,
base_va
)
})?;

Ok(Self {
memory: ReadonlySharedMemory::from_bytes(&memory, layout.snapshot_size)?,
layout,
load_info,
stack_top_gva: exn_stack_top_gva,
sregs: None,
entrypoint: NextAction::Initialise(load_addr + entrypoint_va - base_va),
entrypoint: NextAction::Initialise(code_virt_base + entrypoint_offset),
snapshot_generation: 0,
host_functions: HostFunctionDetails {
host_functions: None,
Expand Down
16 changes: 15 additions & 1 deletion src/hyperlight_host/tests/integration_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::time::Duration;
use hyperlight_common::flatbuffer_wrappers::guest_error::ErrorCode;
use hyperlight_common::log_level::GuestLogFilter;
use hyperlight_host::sandbox::SandboxConfiguration;
use hyperlight_host::{HyperlightError, MultiUseSandbox};
use hyperlight_host::{HyperlightError, MultiUseSandbox, UninitializedSandbox};
use hyperlight_testing::simplelogger::{LOGGER, SimpleLogger};
use serial_test::serial;
use tracing_core::LevelFilter;
Expand Down Expand Up @@ -1872,3 +1872,17 @@ fn hw_timer_interrupts() {
);
});
}

#[test]
#[serial]
Comment thread
cshung marked this conversation as resolved.
Outdated
fn non_pie_guest_hello_world() {
let path =
hyperlight_testing::simple_guest_non_pie_as_string().expect("non-PIE guest not found");
let sandbox =
UninitializedSandbox::new(hyperlight_host::GuestBinary::FilePath(path), None).unwrap();
let mut multi_use_sandbox: MultiUseSandbox = sandbox.evolve().unwrap();
let result: i32 = multi_use_sandbox
.call("PrintOutput", "Hello from non-PIE guest!\n".to_string())
.unwrap();
assert_eq!(result, 26);
}
31 changes: 31 additions & 0 deletions src/hyperlight_testing/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,37 @@ pub fn dummy_guest_as_string() -> Result<String> {
.ok_or_else(|| anyhow!("couldn't convert dummy guest PathBuf to string"))
}

/// Get a fully qualified OS-specific path to the non-PIE simpleguest elf binary
pub fn simple_guest_non_pie_as_string() -> Result<String> {
Comment thread
cshung marked this conversation as resolved.
let buf = rust_guest_non_pie_as_pathbuf("simpleguest");
buf.to_str()
.map(|s| s.to_string())
.ok_or_else(|| anyhow!("couldn't convert non-PIE simple guest PathBuf to string"))
}

/// Get a new `PathBuf` to a specified non-PIE Rust guest
/// $REPO_ROOT/src/tests/rust_guests/bin/${profile}/non_pie/
fn rust_guest_non_pie_as_pathbuf(guest: &str) -> PathBuf {
let build_dir_selector = if cfg!(debug_assertions) {
"debug"
} else {
"release"
};

join_to_path(
MANIFEST_DIR,
vec![
"..",
"tests",
"rust_guests",
"bin",
build_dir_selector,
"non_pie",
guest,
],
)
}

pub fn c_guest_as_pathbuf(guest: &str) -> PathBuf {
let build_dir_selector = if cfg!(debug_assertions) {
"debug"
Expand Down
Loading