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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

ASLR support #725

wants to merge 1 commit into from

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 90.55375% with 29 lines in your changes missing coverage. Please review.

Project coverage is 71.11%. Comparing base (ce8173c) to head (e4a1090).

Files with missing lines Patch % Lines
src/vm.rs 82.71% 14 Missing ⚠️
src/arch/x86_64/paging/mod.rs 90.83% 12 Missing ⚠️
src/bin/uhyve.rs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   70.26%   71.11%   +0.85%     
==========================================
  Files          24       25       +1     
  Lines        2959     3123     +164     
==========================================
+ Hits         2079     2221     +142     
- Misses        880      902      +22     

☔ 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
@jounathaen
Copy link
Member

Currently blocked as we need a release with hermit-os/kernel#1508

@jounathaen jounathaen force-pushed the aslr branch 5 times, most recently from 13a1799 to 74d0048 Compare January 29, 2025 13:25
@jounathaen jounathaen changed the title ASLR: Lay down the foundations ASLR support Jan 29, 2025
@jounathaen jounathaen force-pushed the aslr branch 2 times, most recently from 063fd95 to 78ada8f Compare January 29, 2025 20:46
@jounathaen
Copy link
Member

jounathaen commented Jan 29, 2025

Remaining tasks:

  • add a CLI option to disable ASLR
  • Implement ASLR on aarch64
  • remove duplicate kernel parsing
  • Add mapping of the first GiB to support legacy kernels, or add a version check to only support recent kernels and rebuild hello_world and rusty_demo

@jounathaen jounathaen force-pushed the aslr branch 6 times, most recently from 87e301f to 0aeaae6 Compare January 31, 2025 10:59
@jounathaen jounathaen marked this pull request as ready for review January 31, 2025 10:59
@jounathaen jounathaen force-pushed the aslr branch 7 times, most recently from a25f971 to e4a1090 Compare January 31, 2025 12:43
@jounathaen jounathaen requested a review from mkroening January 31, 2025 16:21
- 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>
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.

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