uh_mem: Properly reset permissions when unsharing pages (#1891)

When we are sharing a page we remove all VTL 0 permissions to that page.
Later on, when we re-private the page, we were failing to reset these
permissions, which led to failures when the guest tried to use pages it
should have had access to. This code is a bit confusing due to
conflating private/shared with VTL 1 access permissions. Add a bunch of
comments, and fix the reset.

Author: smalis-msft

openhcl/underhill_mem/src/lib.rs

@@ -32,7 +32,6 @@ 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;
@@ -399,7 +398,6 @@ impl HardwareIsolatedMemoryProtector {
     fn apply_protections_with_overlay_handling(
         &self,
         range: MemoryRange,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         protections: HvMapGpaFlags,
         inner: &mut MutexGuard<'_, HardwareIsolatedMemoryProtectorInner>,
@@ -431,7 +429,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, calling_vtl, target_vtl, protections)?
+            self.apply_protections(range, target_vtl, protections)?
         }
 
         Ok(())
@@ -440,12 +438,11 @@ impl HardwareIsolatedMemoryProtector {
     fn apply_protections(
         &self,
         range: MemoryRange,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         protections: HvMapGpaFlags,
     ) -> Result<(), ApplyVtlProtectionsError> {
-        if calling_vtl == Vtl::Vtl1 && target_vtl == GuestVtl::Vtl0 {
-            // Only VTL 1 permissions imposed on VTL 0 are explicitly tracked
+        if target_vtl == GuestVtl::Vtl0 {
+            // Only permissions imposed on VTL 0 are explicitly tracked
             self.vtl0.update_permission_bitmaps(range, protections);
         }
         self.acceptor
@@ -590,7 +587,10 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
         for &range in &ranges {
             if shared && vtl == GuestVtl::Vtl0 {
                 // Accessing these pages through the encrypted mapping is now
-                // invalid. Make sure the VTL bitmaps reflect this.
+                // invalid. Make sure the VTL bitmaps reflect this. We could
+                // call apply_protections here but that would result in an extra
+                // hardware interaction that we don't need since we're about to
+                // unaccept the pages anyways.
                 self.vtl0
                     .update_permission_bitmaps(range, HV_MAP_GPA_PERMISSIONS_NONE);
             }
@@ -722,20 +722,12 @@ 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,
-                    if self.vtl1_protections_enabled() {
-                        Vtl::Vtl1
-                    } else {
-                        Vtl::Vtl2
-                    },
-                    GuestVtl::Vtl0,
-                    inner.default_vtl_permissions.vtl0,
-                )
-                .expect("should be able to apply default protections");
+                // Make sure we reset the permissions bitmaps for VTL 0.
+                self.apply_protections(range, 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, Vtl::Vtl2, GuestVtl::Vtl1, vtl1_protections)
+                    self.apply_protections(range, GuestVtl::Vtl1, vtl1_protections)
                         .expect(
                             "everything should be in a state where we can apply VTL protections",
                         );
@@ -784,7 +776,6 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
 
     fn change_default_vtl_protections(
         &self,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         vtl_protections: HvMapGpaFlags,
         tlb_access: &mut dyn TlbFlushLockAccess,
@@ -837,7 +828,6 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
         for range in ranges {
             self.apply_protections_with_overlay_handling(
                 range,
-                calling_vtl,
                 target_vtl,
                 vtl_protections,
                 &mut inner,
@@ -858,7 +848,6 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
 
     fn change_vtl_protections(
         &self,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         gpns: &[u64],
         protections: HvMapGpaFlags,
@@ -901,7 +890,6 @@ impl ProtectIsolatedMemory for HardwareIsolatedMemoryProtector {
         for range in ranges {
             self.apply_protections_with_overlay_handling(
                 range,
-                calling_vtl,
                 target_vtl,
                 protections,
                 &mut inner,
@@ -964,13 +952,8 @@ 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::Vtl2,
-                vtl,
-                new_perms,
-            )
-            .map_err(|_| HvError::OperationDenied)?;
+            self.apply_protections(MemoryRange::from_4k_gpn_range(gpn..gpn + 1), vtl, new_perms)
+                .map_err(|_| HvError::OperationDenied)?;
         }
 
         // Nothing from this point on can fail, so we can safely register the overlay page.
@@ -1022,7 +1005,6 @@ 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,
         )

openhcl/virt_mshv_vtl/src/lib.rs

@@ -1380,7 +1380,6 @@ pub trait ProtectIsolatedMemory: Send + Sync {
     /// hardware-isolated VMs, they apply just to the given vtl.
     fn change_default_vtl_protections(
         &self,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         protections: HvMapGpaFlags,
         tlb_access: &mut dyn TlbFlushLockAccess,
@@ -1389,7 +1388,6 @@ pub trait ProtectIsolatedMemory: Send + Sync {
     /// Changes the vtl protections on a range of guest memory.
     fn change_vtl_protections(
         &self,
-        calling_vtl: Vtl,
         target_vtl: GuestVtl,
         gpns: &[u64],
         protections: HvMapGpaFlags,

openhcl/virt_mshv_vtl/src/processor/hardware_cvm/mod.rs

@@ -1217,7 +1217,6 @@ 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,
@@ -1299,7 +1298,6 @@ 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(),
@@ -1773,7 +1771,6 @@ impl<B: HardwareIsolatedBacking> UhProcessor<'_, B> {
         }
 
         protector.change_default_vtl_protections(
-            vtl.into(),
             targeted_vtl,
             protections,
             &mut self.tlb_flush_lock_access(),