Skip to content

cvm guest vsm: protections applied to overlay pages shouldn't affect bitmaps #1557

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 20, 2025
Merged

cvm guest vsm: protections applied to overlay pages shouldn't affect bitmaps #1557

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
059bace
protections applied to overlay pages shouldn't affect bitmaps
sluck-msft Jun 19, 2025
b65df0d
PR feedback: clarifying comment
sluck-msft Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 55 additions & 20 deletions openhcl/underhill_mem/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use hvdef::HV_PAGE_SIZE;
use hvdef::HvError;
use hvdef::HvMapGpaFlags;
use hvdef::HypercallCode;
use hvdef::Vtl;
use hvdef::hypercall::AcceptMemoryType;
use hvdef::hypercall::HostVisibilityType;
use hvdef::hypercall::HvInputVtl;
Expand Down Expand Up @@ -393,15 +394,16 @@ impl HardwareIsolatedMemoryProtector {
fn apply_protections_with_overlay_handling(
&self,
range: MemoryRange,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
protections: HvMapGpaFlags,
inner: &mut MutexGuard<'_, HardwareIsolatedMemoryProtectorInner>,
) -> Result<(), ApplyVtlProtectionsError> {
let mut range_queue = VecDeque::new();
range_queue.push_back(range);

'outer: while let Some(range) = range_queue.pop_front() {
for overlay_page in inner.overlay_pages[vtl].iter_mut() {
for overlay_page in inner.overlay_pages[target_vtl].iter_mut() {
let overlay_addr = overlay_page.gpn * HV_PAGE_SIZE;
if range.contains_addr(overlay_addr) {
// If the overlay page is within the range, update the
Expand All @@ -424,7 +426,7 @@ impl HardwareIsolatedMemoryProtector {
}
// We can only reach here if the range does not contain any overlay
// pages, so now we can apply the protections to the range.
self.apply_protections(range, vtl, protections)?
self.apply_protections(range, calling_vtl, target_vtl, protections)?
}

Ok(())
Expand All @@ -433,14 +435,16 @@ impl HardwareIsolatedMemoryProtector {
fn apply_protections(
&self,
range: MemoryRange,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
protections: HvMapGpaFlags,
) -> Result<(), ApplyVtlProtectionsError> {
if vtl == GuestVtl::Vtl0 {
// Only VTL 0 vtl permissions are explicitly tracked
if calling_vtl == Vtl::Vtl1 && target_vtl == GuestVtl::Vtl0 {
// Only VTL 1 permissions imposed on VTL 0 are explicitly tracked
self.vtl0.update_permission_bitmaps(range, protections);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we call update_permission_bitmaps in one other spot below, does it need a similar guard?

Copy link
Contributor Author

@sluck-msft sluck-msft Jun 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

That's the one in change_host_visibility? I think we want to remove the permissions there because it's the encrypted mapping and needs to reflect the underlying partition-wide mapping, I can add a comment.

}
self.acceptor.apply_protections(range, vtl, protections)
self.acceptor
.apply_protections(range, target_vtl, protections)
}

/// Get the permissions that the given VTL has to the given GPN.
Expand Down Expand Up @@ -691,11 +695,20 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
// overlay pages won't be host visible, so just apply the default
// protections directly without handling them.
for &range in &ranges {
self.apply_protections(range, GuestVtl::Vtl0, inner.default_vtl_permissions.vtl0)
.expect("should be able to apply default protections");
self.apply_protections(
range,
if self.vtl1_protections_enabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels odd. I feel like this should be another caller_vtl being passed in, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thinking is this:

  1. For change_host_visibility, the calling VTL is itself the "target" vtl (although that doesn't make much sense for host visibility since it applies partition-wide). This is different from modify_vtl_protection_mask, where it should only be a higher VTL targeting a lower VTL.
  2. When changing from shared to private, we apply the default protections on the page. Either there's a VTL 1 that configured default protections, and therefore we're applying the protections on behalf of VTL 1, or there's no VTL 1 (or it hasn't yet configured default protections) and we're applying the protections on behalf of VTL 2.

Vtl::Vtl1
} else {
Vtl::Vtl2
},
GuestVtl::Vtl0,
inner.default_vtl_permissions.vtl0,
)
.expect("should be able to apply default protections");

if let Some(vtl1_protections) = inner.default_vtl_permissions.vtl1 {
self.apply_protections(range, GuestVtl::Vtl1, vtl1_protections)
self.apply_protections(range, Vtl::Vtl2, GuestVtl::Vtl1, vtl1_protections)
.expect(
"everything should be in a state where we can apply VTL protections",
);
Expand Down Expand Up @@ -744,7 +757,8 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {

fn change_default_vtl_protections(
&self,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
vtl_protections: HvMapGpaFlags,
tlb_access: &mut dyn TlbFlushLockAccess,
) -> Result<(), HvError> {
Expand All @@ -756,7 +770,9 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
// finishes last will control the outcome.
let mut inner = self.inner.lock();

inner.default_vtl_permissions.set(vtl, vtl_protections);
inner
.default_vtl_permissions
.set(target_vtl, vtl_protections);

let mut ranges = Vec::new();
for ram_range in self.layout.ram().iter() {
Expand Down Expand Up @@ -789,8 +805,14 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
}

for range in ranges {
self.apply_protections_with_overlay_handling(range, vtl, vtl_protections, &mut inner)
.unwrap();
self.apply_protections_with_overlay_handling(
range,
calling_vtl,
target_vtl,
vtl_protections,
&mut inner,
)
.unwrap();
}

// Flush any threads accessing pages that had their VTL protections
Expand All @@ -806,7 +828,8 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {

fn change_vtl_protections(
&self,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
gpns: &[u64],
protections: HvMapGpaFlags,
tlb_access: &mut dyn TlbFlushLockAccess,
Expand Down Expand Up @@ -844,8 +867,14 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
.unwrap(); // Ok to unwrap, we've validated the gpns above.

for range in ranges {
self.apply_protections_with_overlay_handling(range, vtl, protections, &mut inner)
.unwrap();
self.apply_protections_with_overlay_handling(
range,
calling_vtl,
target_vtl,
protections,
&mut inner,
)
.unwrap();
}

// Flush any threads accessing pages that had their VTL protections
Expand All @@ -857,7 +886,7 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
// other CPUs to release all guest mappings before declaring that the VTL
// protection change has completed.
tlb_access.flush(GuestVtl::Vtl0);
tlb_access.set_wait_for_tlb_locks(vtl);
tlb_access.set_wait_for_tlb_locks(target_vtl);

Ok(())
}
Expand Down Expand Up @@ -892,8 +921,13 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {

// Everything's validated, change the permissions.
if let Some(new_perms) = new_perms {
self.apply_protections(MemoryRange::from_4k_gpn_range(gpn..gpn + 1), vtl, new_perms)
.map_err(|_| HvError::OperationDenied)?;
self.apply_protections(
MemoryRange::from_4k_gpn_range(gpn..gpn + 1),
Vtl::Vtl2,
vtl,
new_perms,
)
.map_err(|_| HvError::OperationDenied)?;
}

// Nothing from this point on can fail, so we can safely register the overlay page.
Expand Down Expand Up @@ -934,6 +968,7 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
// Restore its permissions.
self.apply_protections(
MemoryRange::from_4k_gpn_range(gpn..gpn + 1),
Vtl::Vtl2,
vtl,
overlay_pages[index].previous_permissions,
)
Expand Down
6 changes: 4 additions & 2 deletions openhcl/virt_mshv_vtl/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,15 +1387,17 @@ pub trait ProtectIsolatedMemory: Send + Sync {
/// hardware-isolated VMs, they apply just to the given vtl.
fn change_default_vtl_protections(
&self,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
protections: HvMapGpaFlags,
tlb_access: &mut dyn TlbFlushLockAccess,
) -> Result<(), HvError>;

/// Changes the vtl protections on a range of guest memory.
fn change_vtl_protections(
&self,
vtl: GuestVtl,
calling_vtl: Vtl,
target_vtl: GuestVtl,
gpns: &[u64],
protections: HvMapGpaFlags,
tlb_access: &mut dyn TlbFlushLockAccess,
Expand Down
3 changes: 3 additions & 0 deletions openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1210,6 +1210,7 @@ impl<T, B: HardwareIsolatedBacking> hv1_hypercall::ModifyVtlProtectionMask
// 1 can set the protections, the permissions should be changed for VTL
// 0.
protector.change_vtl_protections(
self.intercepted_vtl.into(),
GuestVtl::Vtl0,
gpa_pages,
map_flags,
Expand Down Expand Up @@ -1291,6 +1292,7 @@ impl<T, B: HardwareIsolatedBacking> hv1_hypercall::EnablePartitionVtl
// Grant VTL 1 access to lower VTL memory
tracing::debug!("Granting VTL 1 access to lower VTL memory");
protector.change_default_vtl_protections(
Vtl::Vtl2,
GuestVtl::Vtl1,
hvdef::HV_MAP_GPA_PERMISSIONS_ALL,
&mut self.vp.tlb_flush_lock_access(),
Expand Down Expand Up @@ -1764,6 +1766,7 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
}

protector.change_default_vtl_protections(
vtl.into(),
targeted_vtl,
protections,
&mut self.tlb_flush_lock_access(),
Expand Down