Improve pmp interface

This new interface allow to interact with the pmp module in a more friendly way. It avoids confusion such as forgetting dividing the values by 4 in the pmp entries. Overall it should make the pmp code easier to read and debug.

src/arch/pmp.rs

@@ -7,6 +7,7 @@ use core::fmt;
 use core::fmt::Formatter;
 
 use super::Architecture;
+use crate::arch::pmp::pmpcfg::{INACTIVE, NAPOT, TOR};
 use crate::arch::pmp::pmplayout::{
     ALL_CATCH_OFFSET, DEVICES_OFFSET, INACTIVE_ENTRY_OFFSET, MIRALIS_OFFSET, MIRALIS_TOTAL_PMP,
     POLICY_OFFSET, POLICY_SIZE, VIRTUAL_PMP_OFFSET,
@@ -58,6 +59,8 @@ pub mod pmpcfg {
     pub const X: u8 = 0b00000100;
     /// Read, Write, and Execute access
     pub const RWX: u8 = R | W | X;
+    /// No permissions
+    pub const NO_PERMISSIONS: u8 = 0x0;
 
     /// Address is Top Of Range (TOP)
     pub const TOR: u8 = 0b00001000;
@@ -85,6 +88,10 @@ pub mod pmpcfg {
 /// This function checks for a minimum size of 8 and for proper alignment. If the requirements are
 /// not satisfied None is returned instead.
 pub const fn build_napot(start: usize, size: usize) -> Option<usize> {
+    if start == 0 && size == usize::MAX {
+        return Some(usize::MAX);
+    }
+
     if size < 8 {
         // Minimum NAPOT size is 8
         return None;
@@ -101,6 +108,11 @@ pub const fn build_napot(start: usize, size: usize) -> Option<usize> {
     Some((start >> 2) | ((size - 1) >> 3))
 }
 
+/// Build a valid TOR pmpaddr value from a provided until memory location.
+pub const fn build_tor(until: usize) -> usize {
+    until >> 2
+}
+
 // ——————————————————————————————— PMP Group ———————————————————————————————— //
 
 pub struct PmpGroup {
@@ -161,44 +173,38 @@ impl PmpGroup {
         // Configure PMP registers, if available
         if pmp.nb_pmp >= 8 {
             // By activating this entry it's possible to catch all memory accesses
-            pmp.set(ALL_CATCH_OFFSET, usize::MAX, pmpcfg::INACTIVE);
+            pmp.set_inactive(ALL_CATCH_OFFSET, 0);
 
             // Protect Miralis
             let (start, size) = Plat::get_miralis_memory_start_and_size();
-            pmp.set(
-                MIRALIS_OFFSET,
-                build_napot(start, size).unwrap(),
-                pmpcfg::NAPOT,
-            );
+            pmp.set_napot(MIRALIS_OFFSET, start, size, pmpcfg::NO_PERMISSIONS);
 
             // Protect virtual devices
-            pmp.set(
+            pmp.set_napot(
                 DEVICES_OFFSET,
-                build_napot(virtual_devices[0].start_addr, virtual_devices[0].size).unwrap(),
-                pmpcfg::NAPOT,
+                virtual_devices[0].start_addr,
+                virtual_devices[0].size,
+                pmpcfg::NO_PERMISSIONS,
             );
 
-            pmp.set(
+            pmp.set_napot(
                 DEVICES_OFFSET + 1,
-                build_napot(virtual_devices[1].start_addr, virtual_devices[1].size).unwrap(),
-                pmpcfg::NAPOT,
+                virtual_devices[1].start_addr,
+                virtual_devices[1].size,
+                pmpcfg::NO_PERMISSIONS,
             );
 
             // This PMP entry is used by the policy module for its own purpose
             #[allow(clippy::reversed_empty_ranges)]
             for idx in 0..POLICY_SIZE {
-                pmp.set(POLICY_OFFSET + idx, 0, pmpcfg::INACTIVE);
+                pmp.set_inactive(POLICY_OFFSET + idx, 0);
             }
 
             // Add an inactive 0 entry so that the next PMP sees 0 with TOR configuration
-            pmp.set(INACTIVE_ENTRY_OFFSET, 0, pmpcfg::INACTIVE);
+            pmp.set_inactive(INACTIVE_ENTRY_OFFSET, 0);
 
             // Finally, set the last PMP to grant access to the whole memory
-            pmp.set(
-                (pmp.nb_pmp - 1) as usize,
-                usize::MAX,
-                pmpcfg::RWX | pmpcfg::NAPOT,
-            );
+            pmp.set_napot((pmp.nb_pmp - 1) as usize, 0, usize::MAX, pmpcfg::RWX);
 
             // Compute the number of virtual PMPs available
             // It's whatever is left after setting pmp's for devices, pmp for address translation,
@@ -219,8 +225,31 @@ impl PmpGroup {
         pmp
     }
 
+    /// This function builds a PMP Napot entry, note that the caller must only set the permissions bits and don't have to care about the low level formatting details to build the napot entry.
+    pub fn set_napot(&mut self, idx: usize, from: usize, to: usize, permissions: u8) {
+        assert!(
+            permissions < 8,
+            "Permissions should not set NAPOT or TOP bits"
+        );
+        self.set(idx, build_napot(from, to).unwrap(), permissions | NAPOT);
+    }
+
+    /// This function builds a PMP Tor entry, note that the caller must only set the permissions bits and don't have to care about the low level formatting details such as dividing the address by 4.
+    pub fn set_tor(&mut self, idx: usize, until: usize, permissions: u8) {
+        assert!(
+            permissions < 8,
+            "Permissions should not set NAPOT or TOP bits"
+        );
+        self.set(idx, build_tor(until), permissions | TOR);
+    }
+
+    /// This function builds a PMP inactive entry, note that the caller must not set the permission bits and can set a base address for the next pmp entry and it can simply give the address without dividing by 4.
+    pub fn set_inactive(&mut self, idx: usize, addr: usize) {
+        self.set(idx, build_tor(addr), INACTIVE);
+    }
+
     /// Set a pmpaddr and its corresponding pmpcfg.
-    pub fn set(&mut self, idx: usize, addr: usize, cfg: u8) {
+    fn set(&mut self, idx: usize, addr: usize, cfg: u8) {
         // Sanitize CFG
         let cfg = cfg & pmpcfg::VALID_BITS;
         assert!(cfg & pmpcfg::L == 0, "Lock bit not yet supported on PMPs");

src/policy/protect_payload.rs

@@ -3,6 +3,7 @@
 use miralis_core::abi_protect_payload;
 
 use crate::arch::pmp::pmpcfg;
+use crate::arch::pmp::pmplayout::POLICY_OFFSET;
 use crate::arch::{MCause, Register};
 use crate::host::MiralisContext;
 use crate::policy::{PolicyHookResult, PolicyModule};
@@ -82,9 +83,9 @@ impl PolicyModule for ProtectPayloadPolicy {
         }
 
         // Lock memory
+        mctx.pmp.set_inactive(POLICY_OFFSET, 0x80400000);
         mctx.pmp
-            .set_from_policy(0, 0x80400000 / 4, pmpcfg::INACTIVE);
-        mctx.pmp.set_from_policy(1, usize::MAX / 4, pmpcfg::TOR);
+            .set_tor(POLICY_OFFSET + 1, usize::MAX, pmpcfg::NO_PERMISSIONS);
 
         self.last_cause = ctx.trap_info.get_cause();
     }
@@ -102,11 +103,13 @@ impl PolicyModule for ProtectPayloadPolicy {
             }
         }
 
-        // Unlock memory
-        mctx.pmp
-            .set_from_policy(0, 0x80400000 / 4, pmpcfg::INACTIVE);
-        mctx.pmp
-            .set_from_policy(1, usize::MAX / 4, pmpcfg::TOR | pmpcfg::RWX);
+        // TODO: Work on that section
+        // Step 2: Restore sensitives privileged registers
+        /*self.write_confidential_registers(ctx, true);*/
+
+        // Step 3: Unlock memory
+        mctx.pmp.set_inactive(POLICY_OFFSET, 0x80400000);
+        mctx.pmp.set_tor(POLICY_OFFSET + 1, usize::MAX, pmpcfg::RWX);
     }
 
     const NUMBER_PMPS: usize = 2;

src/virt.rs

@@ -4,6 +4,7 @@ use miralis_core::abi;
 
 use crate::arch::mstatus::{MBE_FILTER, SBE_FILTER, UBE_FILTER};
 use crate::arch::pmp::pmpcfg;
+use crate::arch::pmp::pmpcfg::NO_PERMISSIONS;
 use crate::arch::{
     hstatus, mie, misa, mstatus, mtvec, parse_mpp_return_mode, satp, Arch, Architecture, Csr,
     ExtensionsCapability, MCause, Mode, Register, TrapInfo,
@@ -899,7 +900,8 @@ impl VirtContext {
         // Deny all addresses by default if at least one PMP is implemented
         if self.nb_pmp > 0 {
             let last_pmp_idx = mctx.pmp.nb_pmp as usize - 1;
-            mctx.pmp.set(last_pmp_idx, usize::MAX, pmpcfg::NAPOT);
+            mctx.pmp
+                .set_napot(last_pmp_idx, 0, usize::MAX, NO_PERMISSIONS);
         }
     }
 
@@ -984,8 +986,7 @@ impl VirtContext {
         mctx.pmp.clear_range(mctx.pmp.virt_pmp_offset, self.nb_pmp);
         // Allow all addresses by default
         let last_pmp_idx = mctx.pmp.nb_pmp as usize - 1;
-        mctx.pmp
-            .set(last_pmp_idx, usize::MAX, pmpcfg::RWX | pmpcfg::NAPOT);
+        mctx.pmp.set_napot(last_pmp_idx, 0, usize::MAX, pmpcfg::RWX);
     }
 }
 
@@ -1237,12 +1238,11 @@ impl HwRegisterContextSetter<Csr> for VirtContext {
                 // pMPRV is never set to 1 outside of a virtual access handler.
                 if mprv != previous_mprv {
                     log::trace!("vMPRV set to {:b}", mprv);
-                    let config = if mprv != 0 {
-                        pmpcfg::TOR | pmpcfg::X
+                    if mprv != 0 {
+                        pmp.set_tor(0, usize::MAX, pmpcfg::X);
                     } else {
-                        pmpcfg::INACTIVE
-                    };
-                    pmp.set(0, usize::MAX, config);
+                        pmp.set_inactive(0, usize::MAX);
+                    }
                     unsafe { Arch::sfencevma(None, None) };
                 }