Make ELF loading respect program header virtual addresses for non-PIE binaries - #1530
Make ELF loading respect program header virtual addresses for non-PIE binaries#1530cshung wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
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_testingto locate the non-PIEsimpleguestbinary. - 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. |
7468e06 to
40c43be
Compare
|
Addressed Copilot review feedback:
|
3fa110a to
35fef7b
Compare
ludfjig
left a comment
There was a problem hiding this comment.
LGTM. @syntactically could you have a look too
syntactically
left a comment
There was a problem hiding this comment.
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!)
ludfjig
left a comment
There was a problem hiding this comment.
LGTM but will once again defer to @syntactically for final review
syntactically
left a comment
There was a problem hiding this comment.
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!
52a8d4f to
b53a7c5
Compare
|
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
left a comment
There was a problem hiding this comment.
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.
b3d461d to
dfed7ca
Compare
623a290 to
e630b8d
Compare
| let mut regions = self.get_memory_regions_::<GuestMemoryRegion>(())?; | ||
|
|
||
| if !is_pie { | ||
| let code_virt_end = code_virt_base + loaded_size; |
There was a problem hiding this comment.
This will wrap and panic if the elf file is mal formed
|
Thanks for all the great work here & putting up with so many rounds of iteration @cshung! The addition of As you probably noticed, we have several different variants of
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 The obvious way to fix this is just by changing 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:
|
|
Thanks for the thorough review @syntactically! The two refactor commits address the main feedback:
Responding to the remaining questions:
It is used by the GDB debug path (\GetCodeSectionOffset\ in \x86_64.rs) to report where the code section lives in guest virtual space. Without it, GDB cannot resolve symbols after snapshot restore. It is also needed for
For non-PIE, the code VA is fixed by the ELF linker, so the user controls it. If it conflicts, that is a build-time problem the user should fix (e.g., pick a different --image-base). Silently relocating other regions would make behavior hard to reason about, and those regions have fixed GPAs that the PEB and guest runtime depend on. I think erroring is the right call here. |
46c86bc to
93e5b1d
Compare
93e5b1d to
c87a4cd
Compare
syntactically
left a comment
There was a problem hiding this comment.
It is used by the GDB debug path (\GetCodeSectionOffset\ in \x86_64.rs) to report where the code section lives in guest virtual space. Without it, GDB cannot resolve symbols after snapshot restore. It is also needed for
ead_guest_memory_by_gva\ (tracing feature) to translate GVAs back to physical offsets.
Ah, of course. I think the SandboxMemoryLayout is already accessible in the relevant places and a lot of the information about the layout is already centralised there; could we just move this into that?
Silently relocating other regions would make behavior hard to reason about, and those regions have fixed GPAs that the PEB and guest runtime depend on. I think erroring is the right call here
Well, in the non-PIE mode, those gpas are not independent of the executable, because the code segment is near the bottom of the initial setup snapshot region. So, it's kind of being consistent to move them. However, I don't feel strongly.
| host_base: K::HostBaseType, | ||
| ) -> Result<Vec<MemoryRegion_<K>>> { | ||
| let mut builder = MemoryRegionVecBuilder::new(Self::BASE_ADDRESS, host_base); | ||
| pub(crate) fn get_memory_regions(&self) -> Result<Vec<MemoryRegion_<GuestMemoryRegion>>> { |
There was a problem hiding this comment.
Is there ever a good reason to call this instead of get_guest_regions_with_code_va below? I see that there is a caller in hyperlight_vm/x86_64.rs, but it looks like that is in a test where the distinction does not matter.
If not, can we either inline the adjustment from get_guest_regions_with_code_va, or make this function private so that nobody calls it by accident?
| let mut builder = MemoryRegionVecBuilder::new(Self::BASE_ADDRESS, Self::BASE_ADDRESS); | ||
|
|
||
| // code | ||
| let peb_offset = builder.push_page_aligned( |
There was a problem hiding this comment.
There's no reason for this to exist in the non-PIE binary case?
| /// represents a single memory region inside the guest. All memory within a region has | ||
| /// the same memory permissions | ||
| #[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
| pub struct MemoryRegion_<K: MemoryRegionKind> { |
There was a problem hiding this comment.
I think it would be nice to be more careful about naming+documentation here, but that can wait if you would prefer.
| // we know this is safe because we check if the regions are empty above | ||
| let last_region = self.regions.last().unwrap(); | ||
| let host_end = <K as MemoryRegionKind>::add(last_region.host_region.end, size); | ||
| let guest_start = last_region.guest_region.end; |
There was a problem hiding this comment.
Does this actually change any behaviour, or is it just a functionally identical incidental formatting cleanup?
| @@ -337,6 +342,14 @@ impl Snapshot { | |||
| let load_addr = layout.get_guest_code_address() as u64; | |||
There was a problem hiding this comment.
Is layout.get_guest_code_address() meaningful at all anymore? I guess it still points to the GPA for the code region---is that actually used anywhere?
There was a problem hiding this comment.
(I am imagining an approach here where you, early on, do layout.set_code_va(Some(the appropriate thing)) and then get_guest_code_address and get_memory_regions both are made to incidentally do the correct thing so that further on code does not have to fix up the layout with an extra value that is threaded around next to it).
| entrypoint_addr, | ||
| original_entrypoint_addr: self.original_entrypoint, | ||
| code_virt_base: self.code_virt_base, | ||
| code_virt_base: self.layout.get_guest_code_gva() as u64, |
There was a problem hiding this comment.
This should probably go in the MemoryLayout structure below, which is more-or-less the serialisation proxy for SandboxMemoryLayout
| )?; | ||
|
|
||
| let load_addr = layout.get_guest_code_address() as u64; | ||
| let load_addr = layout.get_guest_code_gpa() as u64; |
There was a problem hiding this comment.
This load_addr is used in relocation processing, so it seems wrong for it to be the gpa? I think we only support R_<cls>_RELATIVE right now, which is probably not used in the non-PIC binaries that are relevant, but we should probably still do the correct math if we did encounter one.
Actually, it looks like there is a latent bug here even in the PIC code (since a PIC executable doesn't actually have to have a link address of 0): I think probably load_at needs to be adjusted to properly take into account the offset between load_addr and get_base_va, since for e.g. R_AARCH64_RELATIVE, the ABI notes that it "[...] represents a relative adjustment to the place based on the load address of the object relative to its original link address" (emphasis added).
So, probably load_at should take the gva, but also change its relocation process to use a (signed) load_addr - get_base_va() delta as the adjustment to the addend.
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>
Change GuestMemoryRegion::HostBaseType from () to usize so that GuestMemoryRegion becomes a proper mapping: host_region carries guest physical addresses (GPA) and guest_region carries guest virtual addresses (GVA). For identity-mapped regions both are the same. For non-PIE code the Code region's guest_region is overridden to the ELF-declared virtual address. Remove the guest_virt_addr field from MemoryRegion_ since its role is now served by the guest_region/host_region split in GuestMemoryRegion. Use checked_add for the code VA overlap check. 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>
Rename get_memory_regions_ to get_memory_regions and remove the generic type parameter. All callers use GuestMemoryRegion, so the generic is unnecessary. The host_base argument is now always BASE_ADDRESS, supplied internally. 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>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: cshung <3410332+cshung@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: cshung <3410332+cshung@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: cshung <3410332+cshung@users.noreply.github.com>
43036c8 to
d9129d0
Compare
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
code_virt_basefrom the ELF's lowest LOAD segmentp_vaddrbase_va > 0): map code at the declared VA in the guest page tablesbase_va == 0): preserve existing identity mapping behavior (with assertion to guard the invariant)code_virt_base + (entrypoint_va - base_va)The fix leverages the existing
Mappingstruct's support forphys_base != virt_base— no changes to the page table code itself.Testing
non_pie_guest_hello_worldintegration test exercises full guest lifecycle (init, COW, function call, return value) with a non-PIE simpleguest built at--image-base=0x200000Build infrastructure
build-rust-guests-non-pieJustfile targetsguestsrecipe to avoid clobbering normal guest binariessimple_guest_non_pie_as_string()test helperContributes to: #1408