ASLR: Lay down the foundations #725

n0toose · 2024-07-06T17:35:02Z

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 · 2024-07-06T17:44:49Z

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

There has been a regression (commit currently unknown) that makes hello_c not run.

EDIT: ✅

n0toose

cargo test new_file_test (test kernel is written in Rust, should therefore be relocatable) fails:

EDIT: Unrelated to this change, also happens on main. See #726 (with log).

codecov · 2024-07-06T19:21:47Z

Codecov Report

Attention: Patch coverage is 96.98492% with 6 lines in your changes missing coverage. Please review.

Project coverage is 68.86%. Comparing base (e6ffffa) to head (08bfcfd).
Report is 10 commits behind head on main.

Files	Patch %	Lines
src/hypercall.rs	42.85%	4 Missing ⚠️
src/arch/x86_64/paging/mod.rs	99.15%	1 Missing ⚠️
src/vm.rs	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #725      +/-   ##
==========================================
+ Coverage   68.17%   68.86%   +0.68%     
==========================================
  Files          20       21       +1     
  Lines        2319     2370      +51     
==========================================
+ Hits         1581     1632      +51     
  Misses        738      738

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

n0toose

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

src/arch/x86_64/mod.rs

src/arch/aarch64/mod.rs

src/macos/aarch64/vcpu.rs

src/macos/x86_64/vcpu.rs

src/vm.rs

src/arch/x86_64/mod.rs

n0toose · 2024-07-10T20:20:39Z

src/vm.rs

+		// TODO: guest_address is only taken into account on Linux platforms.
+		// TODO: Before changing this, fix init_guest_mem in `src/arch/aarch64/mod.rs`
+		#[cfg(target_os = "linux")]
+		#[cfg(not(target_arch = "x86_64"))]


TODO: Remove this #[cfg... mess and replace it with a feature as soon as

https://github.com/hermit-os/uhyve/pull/725/files#r1672908739 is answered.

jounathaen · 2024-07-16T12:55:40Z

src/linux/x86_64/kvm_cpu.rs

+		&mut self,
+		entry_point: u64,
+		stack_address: u64,
+		guest_address: u64,


What is guest_address? I think we need some documentation here.

I could not clarify this in time, but guest_address is necessary for setting the registers to their correct values.

This was not done for every architecture and operating system - it was only done on x86_64 Linux and, using guesswork, on x86_64 macOS - however, I had to put it in there everywhere nevertheless, even if it has no effect on some architectures.

jounathaen · 2024-07-16T12:56:17Z

src/linux/x86_64/kvm_cpu.rs

+		entry_point: u64,
+		stack_address: u64,
+		guest_address: u64,


This could be GuestPhysicalAddresses, whilst we are on it, right?

Partially resolved, stack_address and entry_point possibly unnecessary?

jounathaen · 2024-07-16T12:57:22Z

src/macos/x86_64/vcpu.rs

@@ -233,7 +233,7 @@ impl XhyveCpu {
 		Ok(())
 	}

-	fn setup_system_64bit(&mut self) -> Result<(), xhypervisor::Error> {
+	fn setup_system_64bit(&mut self, guest_address: u64) -> Result<(), xhypervisor::Error> {


Please add a doc comment on what guest_address is

n0toose · 2024-07-22T18:19:09Z

src/vm.rs

@@ -219,7 +263,7 @@ impl<VCpuType: VirtualCPU> UhyveVm<VCpuType> {
 		};
 		unsafe {
 			let raw_boot_info_ptr =
-				self.mem.host_address.add(BOOT_INFO_ADDR.as_u64() as usize) as *mut RawBootInfo;
+				self.mem.host_address.add(BOOT_INFO_ADDR_OFFSET as usize) as *mut RawBootInfo;
 			*raw_boot_info_ptr = RawBootInfo::from(boot_info);
 			self.boot_info = raw_boot_info_ptr;
 		}


In the following line (270-271, GitHub won't let me comment) the self.stack_address = ... check is possibly unsound?

n0toose

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

src/vm.rs

n0toose · 2024-07-22T20:12:39Z

src/vm.rs

@@ -152,6 +231,10 @@ impl<VCpuType: VirtualCPU> UhyveVm<VCpuType> {
 		self.stack_address
 	}

+	pub fn guest_address(&self) -> u64 {
+		self.guest_address.as_u64()


This should be fixed, and start_address should be appended.

src/vm.rs

n0toose · 2024-07-23T08:21:39Z

src/vm.rs

-		let kernel_end_address = kernel_start_address + object.mem_size();
-		self.offset = kernel_start_address as u64;
+		let kernel_end_address = self.start_address.as_u64() as usize + object.mem_size();
+		self.offset = self.start_address.as_u64();


Currently, UhyveVm has two entries: self.offset, self.start_address, which effectively do the same thing. One of them should be removed, probably.

- 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 - Use u64 for arch::RAM_START instead of GuestVirtAddr - 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, but this is too complex for the time being because of the different architectures. Although this current revision does work with relocatable binaries, it is not making use of this functionality _just_ yet. Fixes hermit-os#719. Co-authored-by: Jonathan <github@jonathanklimt.de>

This will be used by other functions for now, instead of relying on mem.guest_address. guest_address should only be a GuestPhysAddr internally for now, as other functions in places like src/linux/x86_64/kvm_cpu.rs use it.

Removes the start_address entry and uses the existing offset entry instead. Plus some `cargo fmt` changes

…ry is not mapped

n0toose commented Jul 6, 2024

View reviewed changes

n0toose mentioned this pull request Jul 6, 2024

cargo test --test fs-test fails on main #726

Open

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

n0toose commented Jul 8, 2024

View reviewed changes

n0toose force-pushed the aslr branch 2 times, most recently from 3589fe8 to 08bfcfd Compare July 8, 2024 12:47

n0toose commented Jul 10, 2024

View reviewed changes

src/arch/x86_64/mod.rs Outdated Show resolved Hide resolved

n0toose commented Jul 10, 2024

View reviewed changes

jounathaen reviewed Jul 16, 2024

View reviewed changes

n0toose force-pushed the aslr branch from 8077f6d to 3775efa Compare July 22, 2024 17:50

n0toose commented Jul 22, 2024

View reviewed changes

n0toose commented Jul 23, 2024

View reviewed changes

jounathaen mentioned this pull request Jul 23, 2024

ASLR: PoC for generating random address before Uhyve launches #711

Closed

n0toose mentioned this pull request Jul 24, 2024

Implement FromStr / something that allows storing byte slices / yoke for KernelObject hermit-os/hermit-entry#36

Open

n0toose marked this pull request as draft July 28, 2024 14:03

jounathaen self-assigned this Aug 5, 2024

jounathaen force-pushed the aslr branch from 5f8e495 to d88aabd Compare August 5, 2024 16:22

n0toose mentioned this pull request Aug 17, 2024

Implement ASLR #707

Open

jounathaen force-pushed the aslr branch from 640e106 to dc70cc4 Compare September 18, 2024 16:57

n0toose and others added 3 commits September 18, 2024 19:01

ASLR: Use OnceLock for storing guest_address

6df01b7

This will be used by other functions for now, instead of relying on mem.guest_address. guest_address should only be a GuestPhysAddr internally for now, as other functions in places like src/linux/x86_64/kvm_cpu.rs use it.

WIP ASLR work

61effc2

n0toose and others added 9 commits September 18, 2024 19:01

ASLR: Remove superfluous start_address entry

931f6a4

Removes the start_address entry and uses the existing offset entry instead. Plus some `cargo fmt` changes

ASLR: Mark some variables as unused

9d7d7a4

Improved x86 register print, and print registers on internal errors

f2d6622

x86_64: allow MMIO reads for MP floating table search when lower memo…

abf2ec9

…ry is not mapped

Use GuestPhysAddress in the VM struct and the x86 paging initialization

e2ca713

Activated log macros in tests

57f01a7

Added pagetable pretty printing function for debugging

702bc0a

Added HugePageAlignedMem struct for tests

89ea5a6

Fixed virt_to_phys test failing due to uninitialized GUEST_ADDRESS

258784a

jounathaen force-pushed the aslr branch from dc70cc4 to 258784a Compare September 18, 2024 17:01

jounathaen mentioned this pull request Sep 18, 2024

Uhyve Interface V2 #754

Draft

n0toose mentioned this pull request Sep 18, 2024

feat(vm): change UhyveVm struct parameters #755

Closed

jounathaen mentioned this pull request Sep 19, 2024

Enhanced uhyve memory detection hermit-os/kernel#1391

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASLR: Lay down the foundations #725

ASLR: Lay down the foundations #725

n0toose commented Jul 6, 2024 •

edited

Loading

n0toose commented Jul 6, 2024 •

edited

Loading

n0toose left a comment •

edited

Loading

n0toose left a comment •

edited

Loading

codecov bot commented Jul 6, 2024 •

edited

Loading

n0toose left a comment •

edited

Loading

n0toose Jul 10, 2024 •

edited

Loading

jounathaen Jul 16, 2024

n0toose Jul 22, 2024

jounathaen Jul 16, 2024

n0toose Jul 22, 2024

jounathaen Jul 16, 2024

n0toose Jul 22, 2024 •

edited

Loading

n0toose left a comment

n0toose Jul 22, 2024

n0toose Jul 23, 2024

ASLR: Lay down the foundations #725

Are you sure you want to change the base?

ASLR: Lay down the foundations #725

Conversation

n0toose commented Jul 6, 2024 • edited Loading

n0toose commented Jul 6, 2024 • edited Loading

n0toose left a comment • edited Loading

Choose a reason for hiding this comment

n0toose left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 6, 2024 • edited Loading

Codecov Report

n0toose left a comment • edited Loading

Choose a reason for hiding this comment

n0toose Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

jounathaen Jul 16, 2024

Choose a reason for hiding this comment

n0toose Jul 22, 2024

Choose a reason for hiding this comment

jounathaen Jul 16, 2024

Choose a reason for hiding this comment

n0toose Jul 22, 2024

Choose a reason for hiding this comment

jounathaen Jul 16, 2024

Choose a reason for hiding this comment

n0toose Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

n0toose left a comment

Choose a reason for hiding this comment

n0toose Jul 22, 2024

Choose a reason for hiding this comment

n0toose Jul 23, 2024

Choose a reason for hiding this comment

n0toose commented Jul 6, 2024 •

edited

Loading

n0toose commented Jul 6, 2024 •

edited

Loading

n0toose left a comment •

edited

Loading

n0toose left a comment •

edited

Loading

codecov bot commented Jul 6, 2024 •

edited

Loading

n0toose left a comment •

edited

Loading

n0toose Jul 10, 2024 •

edited

Loading

n0toose Jul 22, 2024 •

edited

Loading