Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASLR support #725

Merged
merged 1 commit into from
Feb 5, 2025
Merged

ASLR support #725

merged 1 commit into from
Feb 5, 2025

Conversation

n0toose
Copy link
Member

@n0toose n0toose commented Jul 6, 2024

  • Move x86_64-related paging code to src/arch/x86_64/paging
  • Tests: x86_64-related paging tests should use a guest_address that is
    not 0
  • Tests: Move them in separate files, use appropriate 'use' directives
  • Tests: Use init_guest_mem wrapper in test_virt_to_phys
  • Fix kernel memory loading
  • Add guest_address getter in UhyveVm
  • Change names of constants to clarify their purpose
  • Remove pagetable_l0 from virt_to_phys function
  • Various cargo fmt-related changes
  • aarch64: Blindly replace constant names and similar RAM_START change

We currently rely on guest_address in MmapMemory to calculate the
offsets during the initialization of the VM and when converting
virtual addresses to physical addresses. The latter case is intended
to be temporary - we should read the value from the CR3 register at
a later point.

Although this current revision does work with relocatable binaries, it
is not making use of this functionality just yet.

Fixes #719.
Fixes #713.

@n0toose
Copy link
Member Author

n0toose commented Jul 6, 2024

This change was not tested on macOS (x86_64, aarch64) because of a lack of hardware, and, in the case of macOS (x86_64), there's no CI - I may have to bring up a VM for this, but it will take at least an hour to prepare it. This involved some guess work in both aarch64 and x86_64 - because of this, I will explicitly disable the upcoming ASLR feature in both of these cases.

The change looks artificially bigger than it is because of the x86_64 paging code being moved to a separate file. This is easy for me to undo.

  • This is a rebased and somewhat "cleaned up" version of https://github.com/n0toose/uhyve/tree/make-it-boot (might help with the review), plus lots of tiny changes:
    • I had to fix lots of things in aarch64 (very often, formatting mistakes caught by cargo fmt).
    • Everything aarch64 and macOS-related is not part of the tree
  • Despite the refactoring, the guest_address should always be zero for the time being. The work to change this was done here: ASLR: PoC for generating random address before Uhyve launches #711
  • These two changes were kept separate because of the very big scope of this change - and because some further thought has to be poured into the case of the hello_c case.

n0toose

This comment was marked as outdated.

n0toose

This comment was marked as outdated.

@n0toose n0toose force-pushed the aslr branch 3 times, most recently from 80d7f57 to 28b8dcf Compare July 6, 2024 18:55
Copy link

codecov bot commented Jul 6, 2024

Codecov Report

Attention: Patch coverage is 91.55844% with 26 lines in your changes missing coverage. Please review.

Project coverage is 71.02%. Comparing base (0bfe983) to head (9448b19).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/arch/x86_64/paging/mod.rs 90.83% 12 Missing ⚠️
src/vm.rs 86.58% 11 Missing ⚠️
src/bin/uhyve.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   70.07%   71.02%   +0.95%     
==========================================
  Files          24       25       +1     
  Lines        3014     3179     +165     
==========================================
+ Hits         2112     2258     +146     
- Misses        902      921      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member Author

@n0toose n0toose left a comment

Choose a reason for hiding this comment

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

I spent 15 minutes trying to give more context behind the macOS changes on x86_64 and aarch64 to help the review process - where I tried to change the least amount of things possible until the compiler stopped being mad at me (and as a way of double-checking the changes).

The goal is to have this changed merged directly into main or in a feature branch, so that I can continue working on top of these foundations. Hope this helps.

(EDIT: I marked all of these comments as "resolved" to keep the PR log cleaner.)

src/arch/aarch64/mod.rs Outdated Show resolved Hide resolved
src/arch/aarch64/mod.rs Show resolved Hide resolved
src/arch/x86_64/mod.rs Outdated Show resolved Hide resolved
src/arch/aarch64/mod.rs Outdated Show resolved Hide resolved
src/macos/aarch64/vcpu.rs Outdated Show resolved Hide resolved
src/macos/x86_64/vcpu.rs Outdated Show resolved Hide resolved
src/macos/x86_64/vcpu.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
@n0toose n0toose force-pushed the aslr branch 2 times, most recently from 3589fe8 to 08bfcfd Compare July 8, 2024 12:47
src/arch/x86_64/mod.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
Copy link
Member Author

@n0toose n0toose left a comment

Choose a reason for hiding this comment

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

Some notes on the latest revision which does not fully work...

src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
Copy link
Member

@mkroening mkroening left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, but I have a few remarks.

I did not understand all the details, but that's fine. :)

Cargo.toml Outdated Show resolved Hide resolved
src/arch/x86_64/paging/mod.rs Outdated Show resolved Hide resolved
src/arch/x86_64/paging/mod.rs Outdated Show resolved Hide resolved
src/macos/aarch64/vcpu.rs Outdated Show resolved Hide resolved
src/paging.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Outdated Show resolved Hide resolved
src/vm.rs Show resolved Hide resolved
@jounathaen jounathaen force-pushed the aslr branch 2 times, most recently from 32f5634 to adf379c Compare February 5, 2025 10:19
- Kernel is loaded to a random physical address
- Pagetables are created for the kernel region instead of just the first
  gigabyte

Fixes hermit-os#719.

Co-authored-by: Jonathan <github@jonathanklimt.de>
@jounathaen jounathaen dismissed mkroening’s stale review February 5, 2025 11:02

All requested changes implemented

@jounathaen jounathaen enabled auto-merge February 5, 2025 11:02
@jounathaen jounathaen added this pull request to the merge queue Feb 5, 2025
Merged via the queue into hermit-os:main with commit 9448b19 Feb 5, 2025
19 checks passed
@jounathaen jounathaen deleted the aslr branch February 5, 2025 11:05
@n0toose
Copy link
Member Author

n0toose commented Feb 5, 2025

🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove pagetable_l0 parameter in virt_to_phys. virt_to_phys: breaks if guest_address is not equal to 0
3 participants