Skip to content

Make ELF loading respect program header virtual addresses for non-PIE binaries - #1530

Open
cshung wants to merge 1 commit into
hyperlight-dev:mainfrom
cshung:cshung/non-pie-elf-loading
Open

Make ELF loading respect program header virtual addresses for non-PIE binaries#1530
cshung wants to merge 1 commit into
hyperlight-dev:mainfrom
cshung:cshung/non-pie-elf-loading

Conversation

@cshung

@cshung cshung commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Summary

For non-PIE (ET_EXEC) ELF binaries, the guest page table now maps the code region at the ELF's declared virtual address rather than identity-mapping it at the GPA. This allows statically-linked binaries with a fixed load address (e.g., --image-base=0x200000) to execute correctly.

Problem

Previously, Hyperlight assumed code GVA == code GPA (identity mapping). Non-PIE binaries that declare a non-zero base virtual address (via program header p_vaddr) would triple-fault because the guest CPU jumped to the ELF's declared entrypoint VA, which wasn't mapped in the page tables.

Solution

  • Compute code_virt_base from the ELF's lowest LOAD segment p_vaddr
  • For non-PIE (base_va > 0): map code at the declared VA in the guest page tables
  • For PIE (base_va == 0): preserve existing identity mapping behavior (with assertion to guard the invariant)
  • Compute entrypoint as code_virt_base + (entrypoint_va - base_va)

The fix leverages the existing Mapping struct's support for phys_base != virt_base — no changes to the page table code itself.

Testing

  • New non_pie_guest_hello_world integration test exercises full guest lifecycle (init, COW, function call, return value) with a non-PIE simpleguest built at --image-base=0x200000
  • All existing PIE guest tests continue to pass (identity mapping preserved)
  • Tested on Windows/WHP and Linux/KVM

Build infrastructure

  • Added build-rust-guests-non-pie Justfile targets
  • Non-PIE build runs first in guests recipe to avoid clobbering normal guest binaries
  • Added simple_guest_non_pie_as_string() test helper

Contributes to: #1408

Copilot AI review requested due to automatic review settings June 14, 2026 22:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR adds support for running and testing non-PIE Rust guest binaries, including updating snapshot virtual-address mapping so non-PIE guests execute at their declared ELF virtual addresses.

Changes:

  • Add a helper in hyperlight_testing to locate the non-PIE simpleguest binary.
  • Update snapshot mapping/entrypoint calculation to support non-identity VA mappings for non-PIE code regions.
  • Add a new integration test and build automation (Justfile) to produce and run a non-PIE guest.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/hyperlight_testing/src/lib.rs Adds path helper(s) for locating non-PIE Rust guest binaries.
src/hyperlight_host/tests/integration_test.rs Adds an integration test that boots and calls into a non-PIE guest.
src/hyperlight_host/src/sandbox/snapshot/mod.rs Adjusts snapshot mappings and entrypoint VA calculation to support non-PIE guests.
Justfile Adds tasks to build and stage non-PIE Rust guest artifacts.

Comment thread Justfile Outdated
Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
Comment thread src/hyperlight_testing/src/lib.rs
@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch from 7468e06 to 40c43be Compare June 14, 2026 23:10
@cshung

cshung commented Jun 14, 2026

Copy link
Copy Markdown
Contributor Author

Addressed Copilot review feedback:

  1. Justfile (RUSTFLAGS on Linux) — Fixed. \RUSTFLAGS\ now prefixes \cargo\ directly after \cd &&\ so the env var applies to the build command.

  2. snapshot/mod.rs (virt_base mapping) — No change needed. The code region is a single contiguous block where
    gn.guest_region.start == load_addr\ — there is no offset to lose. The \�ssert_eq!\ is deliberate per codebase convention.

  3. snapshot/mod.rs (entrypoint underflow) — Fixed. Now uses \checked_sub\ with a proper error for malformed ELFs.

  4. lib.rs (doc says 'elf binary') — No change needed. Hyperlight guests are always ELF regardless of host OS.

@ludfjig ludfjig added the kind/enhancement For PRs adding features, improving functionality, docs, tests, etc. label Jun 15, 2026
@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch 2 times, most recently from 3fa110a to 35fef7b Compare June 15, 2026 19:43

@ludfjig ludfjig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @syntactically could you have a look too

@syntactically syntactically left a comment

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.

Thanks for doing this! It looks like it is moving in a good direction. I have a couple of minor suggestions/comments, as well as a bit of an alternative design that you could take (but totally up to you on that one!)

Comment thread Justfile Outdated
Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
@cshung cshung added the ready-for-review PR is ready for (re-)review label Jun 23, 2026
Comment thread src/hyperlight_host/tests/integration_test.rs Outdated

@ludfjig ludfjig left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM but will once again defer to @syntactically for final review

syntactically
syntactically previously approved these changes Jul 15, 2026

@syntactically syntactically left a comment

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 looks good to me, thank you and sorry for the delay reviewing!

I do think that there is one minor move that would make the code a little bit cleaner / better fitting with existing conventions, which I've commented on inline. Feel free to push back with a reason why this code belongs here, though!

Comment thread src/hyperlight_host/src/sandbox/snapshot/mod.rs Outdated
@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch 2 times, most recently from 52a8d4f to b53a7c5 Compare July 18, 2026 13:54
@cshung

cshung commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

All review comments have been addressed. The suggestion to move conflict-check logic into \layout.rs\ (from the Jul 15 review) is implemented in the follow-up PR #1655 via the \code_virt_base()\ method which handles both VA selection and overlap validation.

@syntactically — ready for re-review when you get a chance!

syntactically
syntactically previously approved these changes Jul 20, 2026

@syntactically syntactically left a comment

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.

If you've already done that, I think it would be slightly nice to get it cherry-picked here without the ASLR changes, since I'm not totally sure we are decided on doing ASLR---it has pretty limited benefits for the modal hyperlight use case because one can't (generally) re-slide an image after a snapshot is taken, and in practice almost all images of a given binary are expected to descend from one snapshot.

As I said before I don't feel incredibly strongly though---I will leave the decision up to you.

@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch 2 times, most recently from b3d461d to dfed7ca Compare July 20, 2026 20:52
Comment thread src/hyperlight_host/src/sandbox/snapshot/file/mod.rs Outdated
jsturtevant
jsturtevant previously approved these changes Jul 21, 2026
@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch 2 times, most recently from 1799d75 to 623a290 Compare July 22, 2026 15:39
Add support for running non-PIE (ET_EXEC) guest binaries by mapping
code at the ELF's declared virtual address rather than assuming
identity mapping (physical == virtual).

Changes:
- Add is_pie() and base_va() methods to ExeInfo/ElfInfo to detect
  ET_DYN vs ET_EXEC binaries and extract the base virtual address
- Add SandboxMemoryLayout::code_virt_base() to compute the correct
  virtual base for the code region and validate it doesn't conflict
  with other memory regions
- Update snapshot creation to use non-identity virtual mapping for
  non-PIE code regions
- Add non-PIE guest build step to CI (cargo hyperlight with
  -C relocation-model=static -C link-args=--no-pie)
- Add integration test verifying non-PIE guest execution
- Add test helper for locating non-PIE guest binaries

Signed-off-by: cshung <3410332+cshung@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6f20a05d-6bee-4e2e-b320-12f8d9759bbc
Signed-off-by: cshung <3410332+cshung@users.noreply.github.com>
@cshung
cshung force-pushed the cshung/non-pie-elf-loading branch from 623a290 to e630b8d Compare July 27, 2026 19:30
let mut regions = self.get_memory_regions_::<GuestMemoryRegion>(())?;

if !is_pie {
let code_virt_end = code_virt_base + loaded_size;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will wrap and panic if the elf file is mal formed

@syntactically

Copy link
Copy Markdown
Member

Thanks for all the great work here & putting up with so many rounds of iteration @cshung!

The addition of guest_virt_addr made me a bit uncomfortable, because I knew that we were using memory regions that had guest virtual addresses in them already in other places. I took a little bit more of a look at it, and I realised that this PR has exposed an existing semantic inconsistency in the way that we use MemoryRegions.

As you probably noticed, we have several different variants of MemoryRegion, defined as a base MemoryRegion_<T> struct that is parameterised by a marker type. There are IIRC three of those marker types that we are using right now, and it turns out that all of them have a different idea of what "host" and "guest" mean:

  • HostGuestMemoryRegion maps host virtual addresses to guest physical addresses
  • GuestMemoryRegion, which presently is not a mapping at all---just legitimately a region chunk of a single address space
  • CrashDumpMemoryRegion, which maps host virtual addresses to guest virtual addresses.

Adding another field for GVA to the base struct is not ideal, then, because it's semantically confusing for the other variants (one of which even already contains GVAs!). I think what this PR really needs is for GuestMemoryRegion to be a mapping, the same as the other memory regions are, which is a nice increase in consistency. We would then switch to using the two sides of the mapping as the guest phys/virt addresses.

The obvious way to fix this is just by changing <GuestMemoryRegion as MemoryRegionKind>::HostBaseType to u64 and using it as the guest physical address. The big downside is of course that the naming becomes even more misleading: whilst "host_region" would be "towards the host side" (on a semantic line of 'host physical address <- host virtual address <- guest physical address <- guest virtual address'), it's not actually located on the host.

If you wanted to make another change to rename that type and fields to something more sensible (either as another PR using stacked PRs to keep this on top, or as an early commit in this PR which we must keep without squashing when merging, since it's a noisy, large, and semantically distinct change), that would be great, but is by no means required/expected. If you did do that, taking the opportunity to (again, as distinct commits that we must not squash on merge) to also remove one of the redundant size fields in the structure (so changing from host range/guest range to host base/guest base/size) would be great.

A couple more minor nits:

  • I believe that there are no longer any users of SandboxMemoryLayout::get_memory_regions_ using a region kind other than GuestMemoryRegion. So, we could rename it to get_memory_regions and concretise its type. Since it would then always be returning things with the correct host-side address type, we could avoid adding the new _with_code_va wrapper method, and just make the changes directly to get_memory_regions
  • Why is code_virt_base pushed through snapshots / into the mgr? I see a comment about it being needed for gdb and tracing to resolve the offsets to the binary, but I don't see that happening
  • If other regions intersect the code VA, should they be pushed around to make room, instead of signalling an error? I don't recall off the top of my head whether there is anything making assumptions about the layout of those regions, but I would rather hope not. There aren't many of them, and I hope they will all go away quite soon, so it does not matter too much, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement For PRs adding features, improving functionality, docs, tests, etc. ready-for-review PR is ready for (re-)review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants