more output

security-monitor/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "security-monitor"
-version = "0.3.0"
+version = "0.6.0"
 authors = ["Wojciech Ozga <woz@zurich.ibm.com>"]
 description = "Assured Confidential Execution (ACE): security monitor implementation targetting RISC-V"
 edition = "2021"

security-monitor/src/confidential_flow/finite_state_machine.rs

@@ -26,7 +26,7 @@ use crate::core::architecture::riscv::sbi::RfenceExtension::*;
 use crate::core::architecture::riscv::sbi::SbiExtension::*;
 use crate::core::architecture::riscv::sbi::SrstExtension::*;
 use crate::core::architecture::TrapCause::*;
-use crate::core::architecture::{HartLifecycleState, TrapCause};
+use crate::core::architecture::{GeneralPurposeRegister, HartLifecycleState, TrapCause};
 use crate::core::control_data::{
     ConfidentialHart, ConfidentialHartRemoteCommand, ConfidentialVm, ConfidentialVmId, ControlDataStorage, HardwareHart, HypervisorHart,
     ResumableOperation,
@@ -67,7 +67,25 @@ impl<'a> ConfidentialFlow<'a> {
     unsafe extern "C" fn route_trap_from_confidential_hart(hardware_hart_pointer: *mut HardwareHart) -> ! {
         let flow = Self { hardware_hart: unsafe { hardware_hart_pointer.as_mut().expect(Self::CTX_SWITCH_ERROR_MSG) } };
         assert!(!flow.hardware_hart.confidential_hart().is_dummy());
-        match TrapCause::from_hart_architectural_state(flow.confidential_hart().confidential_hart_state()) {
+        let tt = TrapCause::from_hart_architectural_state(flow.confidential_hart().confidential_hart_state());
+        let a7 = flow.confidential_hart().gprs().read(GeneralPurposeRegister::a7);
+        let a6 = flow.confidential_hart().gprs().read(GeneralPurposeRegister::a6);
+        match tt {
+            Interrupt => {}
+            _ => {
+                debug!(
+                    "Enter a7={:x} a6={:x} mcause={} htinst={:x} mepc={:x}",
+                    a7,
+                    a6,
+                    flow.confidential_hart().csrs().mcause.read(),
+                    flow.confidential_hart().csrs().htinst.read(),
+                    flow.confidential_hart().csrs().mepc.read_from_main_memory()
+                );
+                crate::debug::__print_hart_state(flow.confidential_hart().confidential_hart_state());
+            }
+        }
+
+        match tt {
             Interrupt => HandleInterrupt::from_confidential_hart(flow.confidential_hart()).handle(flow),
             VsEcall(Base(GetSpecVersion)) => SbiGetSpecVersion::from_confidential_hart(flow.confidential_hart()).handle(flow),
             VsEcall(Base(GetImplId)) => SbiGetImplId::from_confidential_hart(flow.confidential_hart()).handle(flow),

security-monitor/src/confidential_flow/handlers/interrupts/handle_interrupt.rs

@@ -19,17 +19,17 @@ impl HandleInterrupt {
     }
 
     pub fn handle(self, confidential_flow: ConfidentialFlow) -> ! {
-        if self.pending_interrupts & MIE_SSIP_MASK > 0 {
-            // One of the reasons why the confidential hart was interrupted with SSIP is that it got an `ConfidentialHartRemoteCommand` from
-            // another confidential hart. If this is the case, we must process all queued requests before resuming confidential
-            // hart's execution. This is done as part of the procedure that resumes confidential hart execution.
-            confidential_flow.resume_confidential_hart_execution();
-        } else {
-            // The only interrupts that we can see here are:
-            // * M-mode timer that the security monitor set to preemt execution of a confidential VM
-            // * M-mode software or external interrupt
-            confidential_flow.into_non_confidential_flow().declassify_and_exit_to_hypervisor(DeclassifyToHypervisor::Interrupt(self))
-        }
+        // if self.pending_interrupts & MIE_SSIP_MASK > 0 {
+        // One of the reasons why the confidential hart was interrupted with SSIP is that it got an `ConfidentialHartRemoteCommand` from
+        // another confidential hart. If this is the case, we must process all queued requests before resuming confidential
+        // hart's execution. This is done as part of the procedure that resumes confidential hart execution.
+        //     confidential_flow.resume_confidential_hart_execution();
+        // } else {
+        // The only interrupts that we can see here are:
+        // * M-mode timer that the security monitor set to preemt execution of a confidential VM
+        // * M-mode software or external interrupt
+        confidential_flow.into_non_confidential_flow().declassify_and_exit_to_hypervisor(DeclassifyToHypervisor::Interrupt(self))
+        // }
     }
 
     pub fn declassify_to_hypervisor_hart(&self, hypervisor_hart: &mut HypervisorHart) {

security-monitor/src/confidential_flow/handlers/mmio/mmio_load_request.rs

@@ -15,25 +15,77 @@ pub struct MmioLoadRequest {
     mtval: usize,
     mtval2: usize,
     mtinst: usize,
+    instruction_alignment: Option<usize>,
 }
 
 impl MmioLoadRequest {
     pub fn from_confidential_hart(confidential_hart: &ConfidentialHart) -> Self {
+        let mut instruction_alignment = None;
+        let mut mtinst = confidential_hart.csrs().mtinst.read();
+        if mtinst == 0 {
+            let mepc = confidential_hart.csrs().mepc.read_from_main_memory();
+            confidential_hart.csrs().mstatus.read_and_set_bits((1 << 17));
+            confidential_hart.csrs().mstatus.read_and_set_bits((1 << 19));
+            debug!("Load mepc={:x} mastatus={:x}", mepc, confidential_hart.csrs().mstatus.read());
+            // enable MPRV and MXR
+            // Safety: Below memory reads using raw pointer are safe because:
+            //  * RISC-V requires that instructions are properly aligned
+            //  * It is the processor who writes mepc with the address of the trapped instruction
+            //  * Processor will perform memory address translation using guest's MMU configuration because of mstatus::MPRV
+            if mepc % 8 == 0 {
+                mtinst = unsafe { (mepc as *const u64).read_volatile() } as usize;
+                instruction_alignment = Some(8);
+                debug!("Load 8B alignment");
+            } else if mepc % 4 == 0 {
+                mtinst = unsafe { (mepc as *const u32).read_volatile() } as usize;
+                instruction_alignment = Some(4);
+                debug!("Load 4B alignment");
+            } else if mepc % 2 == 0 {
+                mtinst = unsafe { (mepc as *const u16).read_volatile() } as usize;
+                instruction_alignment = Some(2);
+                debug!("Load 2B alignment");
+            } else {
+                debug!("Load invalid alignment");
+            }
+            confidential_hart.csrs().mstatus.read_and_clear_bits((1 << 17));
+            confidential_hart.csrs().mstatus.read_and_clear_bits((1 << 19));
+            debug!("Load mtinst={:x} mastatus={:x}", mtinst, confidential_hart.csrs().mstatus.read());
+        }
+
         Self {
             mcause: confidential_hart.csrs().mcause.read(),
             mtval: confidential_hart.csrs().mtval.read(),
             mtval2: confidential_hart.csrs().mtval2.read(),
-            mtinst: confidential_hart.csrs().mtinst.read(),
+            mtinst,
+            instruction_alignment,
         }
     }
 
     pub fn handle(self, confidential_flow: ConfidentialFlow) -> ! {
         // According to the RISC-V privilege spec, mtinst encodes faulted instruction (bit 0 is 1) or a pseudo instruction
-        assert!(self.mtinst & 0x1 > 0);
-        let instruction = self.mtinst | 0x3;
+        if self.mtinst & 0x1 == 0 {
+            debug!("Detected pseudo instruction, should never happen!");
+        }
+
+        let mut instruction = self.mtinst | 0x3;
         let instruction_length = if is_bit_enabled(self.mtinst, 1) { riscv_decode::instruction_length(instruction as u16) } else { 2 };
+        debug!("Load instr len: {} instruction_alignment = {:?}", instruction_length, self.instruction_alignment);
+
+        // Make sure that we do not expose via mtinst more than the trapped instruction. This could happen when security monitor reads
+        // the trapped instruction direcly from the guest's main memory but the instruction size is smaller than its alignment.
+        // For example, consider that security monitor reads instruction of size 2B that is aligned to 8B. It would read the entire 8B
+        // containing 2B instruction + 6B of the next instruction. It must expose only 2B. So, it must determine the size instruction
+        // and then mask only bits belonging to this instruction. Otherwise, we would create a cover channel.
+        if let Some(alignment) = self.instruction_alignment {
+            if alignment < instruction_length {
+                debug!("Not aligned instruction read from mepc for mmio load: {} {:x}", instruction_length, instruction);
+            } else if alignment > instruction_length {
+                instruction = instruction & ((1 << 8 * instruction_length) - 1);
+            }
+        }
 
         let fault_address = (self.mtval2 << 2) | (self.mtval & 0x3);
+        debug!("Load fault address {:x}", fault_address);
         if !MmioAccessFault::tried_to_access_valid_mmio_region(confidential_flow.confidential_vm_id(), fault_address) {
             let mmio_access_fault_handler = MmioAccessFault::new(CAUSE_LOAD_ACCESS.into(), self.mtval, instruction_length);
             confidential_flow.apply_and_exit_to_confidential_hart(ApplyToConfidentialHart::MmioAccessFault(mmio_access_fault_handler));

security-monitor/src/confidential_flow/handlers/mmio/mmio_store_request.rs

@@ -24,15 +24,58 @@ pub struct MmioStoreRequest {
 impl MmioStoreRequest {
     pub fn from_confidential_hart(confidential_hart: &ConfidentialHart) -> Self {
         let mcause = confidential_hart.csrs().mcause.read();
-        let mtinst = confidential_hart.csrs().mtinst.read();
         let mtval = confidential_hart.csrs().mtval.read();
         let mtval2 = confidential_hart.csrs().mtval2.read();
 
+        let mut instruction_alignment = None;
+        let mut mtinst = confidential_hart.csrs().mtinst.read();
+        if mtinst == 0 {
+            let mepc = confidential_hart.csrs().mepc.read_from_main_memory();
+            confidential_hart.csrs().mstatus.read_and_set_bits((1 << 17));
+            confidential_hart.csrs().mstatus.read_and_set_bits((1 << 19));
+            // enable MPRV and MXR
+            // Safety: Below memory reads using raw pointer are safe because:
+            //  * RISC-V requires that instructions are properly aligned
+            //  * It is the processor who writes mepc with the address of the trapped instruction
+            //  * Processor will perform memory address translation using guest's MMU configuration because of mstatus::MPRV
+            if mepc % 8 == 0 {
+                mtinst = unsafe { (mepc as *const u64).read_volatile() } as usize;
+                instruction_alignment = Some(8);
+            } else if mepc % 4 == 0 {
+                mtinst = unsafe { (mepc as *const u32).read_volatile() } as usize;
+                instruction_alignment = Some(4);
+            } else if mepc % 2 == 0 {
+                mtinst = unsafe { (mepc as *const u16).read_volatile() } as usize;
+                instruction_alignment = Some(2);
+            } else {
+                debug!("Store invalid alignment");
+            }
+            confidential_hart.csrs().mstatus.read_and_clear_bits((1 << 17));
+            confidential_hart.csrs().mstatus.read_and_clear_bits((1 << 19));
+        }
+
         // According to the RISC-V privilege spec, mtinst encodes faulted instruction when bit 0 is 1.
         // Otherwise it is a pseudo instruction.
-        assert!(mtinst & 0x1 > 0);
-        let instruction = mtinst | 0x3;
+        if mtinst & 0x1 == 0 {
+            debug!("Detected pseudo instruction, should never happen!");
+        }
+
+        let mut instruction = mtinst | 0x3;
         let instruction_length = if is_bit_enabled(mtinst, 1) { riscv_decode::instruction_length(instruction as u16) } else { 2 };
+
+        // Make sure that we do not expose via mtinst more than the trapped instruction. This could happen when security monitor reads
+        // the trapped instruction direcly from the guest's main memory but the instruction size is smaller than its alignment.
+        // For example, consider that security monitor reads instruction of size 2B that is aligned to 8B. It would read the entire 8B
+        // containing 2B instruction + 6B of the next instruction. It must expose only 2B. So, it must determine the size instruction
+        // and then mask only bits belonging to this instruction. Otherwise, we would create a cover channel.
+        if let Some(alignment) = instruction_alignment {
+            if alignment < instruction_length {
+                debug!("Not aligned instruction read from mepc for mmio load: {} {:x}", instruction_length, instruction);
+            } else if alignment > instruction_length {
+                instruction = instruction & ((1 << 8 * instruction_length) - 1);
+            }
+        }
+
         let gpr = crate::core::architecture::decode_result_register(instruction);
         let gpr_value = gpr.as_ref().and_then(|ref gpr| Ok(confidential_hart.gprs().read(**gpr))).unwrap_or(0);
 
@@ -41,6 +84,7 @@ impl MmioStoreRequest {
 
     pub fn handle(self, confidential_flow: ConfidentialFlow) -> ! {
         let fault_address = (self.mtval2 << 2) | (self.mtval & 0x3);
+
         if !MmioAccessFault::tried_to_access_valid_mmio_region(confidential_flow.confidential_vm_id(), fault_address) {
             let mmio_access_fault_handler = MmioAccessFault::new(CAUSE_STORE_ACCESS.into(), self.mtval, self.instruction_length);
             confidential_flow.apply_and_exit_to_confidential_hart(ApplyToConfidentialHart::MmioAccessFault(mmio_access_fault_handler));

security-monitor/src/confidential_flow/handlers/sbi_base_extension/get_impl_id.rs

@@ -13,6 +13,7 @@ impl SbiGetImplId {
     }
 
     pub fn handle(self, confidential_flow: ConfidentialFlow) -> ! {
+        debug!("SbiGetImplId");
         let impl_id = 1;
         let transformation = ApplyToConfidentialHart::SbiResponse(SbiResponse::success_with_code(impl_id));
         confidential_flow.apply_and_exit_to_confidential_hart(transformation)

security-monitor/src/confidential_flow/handlers/shared_page/share_page_request.rs

@@ -29,6 +29,7 @@ impl SharePageRequest {
     }
 
     pub fn handle(self, confidential_flow: ConfidentialFlow) -> ! {
+        debug!("SharePageRequest");
         match self.share_page_sbi_request() {
             Ok(sbi_request) => confidential_flow
                 .set_resumable_operation(ResumableOperation::SharePage(self))

security-monitor/src/core/initialization/mod.rs

@@ -40,7 +40,7 @@ static HARTS_STATES: Once<Mutex<Vec<HardwareHart>>> = Once::new();
 /// function, the security properties of ACE hold.
 #[no_mangle]
 extern "C" fn init_security_monitor_asm(cold_boot: bool, flattened_device_tree_address: *const u8) {
-    debug!("Initializing the CoVE security monitor for SiFive P550: v6");
+    debug!("Initializing the CoVE security monitor for SiFive P550: v7");
     if cold_boot {
         if let Err(error) = init_security_monitor(flattened_device_tree_address) {
             // TODO: lock access to attestation keys/seed/credentials.

security-monitor/src/debug.rs

@@ -54,14 +54,14 @@ pub fn __print_pmp_configuration() {}
 #[cfg(feature = "verbose")]
 pub fn __print_hart_state(state: &HartArchitecturalState) {
     debug!("Hart state:");
-    for i in 0..8 {
-        let gpr_id = i * 4;
-        let v0 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id).unwrap());
-        let v1 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 1).unwrap());
-        let v2 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 2).unwrap());
-        let v3 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 3).unwrap());
-        debug!("x{}: {:16x}\tx{}: {:16x}\tx{}: {:16x}\tx{}: {:16x}", gpr_id, v0, gpr_id + 1, v1, gpr_id + 2, v2, gpr_id + 3, v3);
-    }
+    // for i in 0..8 {
+    //     let gpr_id = i * 4;
+    //     let v0 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id).unwrap());
+    //     let v1 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 1).unwrap());
+    //     let v2 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 2).unwrap());
+    //     let v3 = state.gprs().read(GeneralPurposeRegister::try_from(gpr_id + 3).unwrap());
+    //     debug!("x{}: {:16x}\tx{}: {:16x}\tx{}: {:16x}\tx{}: {:16x}", gpr_id, v0, gpr_id + 1, v1, gpr_id + 2, v2, gpr_id + 3, v3);
+    // }
 
     debug!("mepc (from main memory) = {:x}", state.csrs().mepc.read_from_main_memory());
     debug!("mepc = {:16x}", state.csrs().mepc.read());
@@ -74,30 +74,30 @@ pub fn __print_hart_state(state: &HartArchitecturalState) {
     debug!("mtinst = {:16x}", state.csrs().mtinst.read());
     debug!("mtval = {:16x}", state.csrs().mtval.read());
     debug!("mtval2 = {:16x}", state.csrs().mtval2.read());
-    debug!("mtvec = {:16x}", state.csrs().mtvec.read());
-    debug!("mscratch = {:16x}", state.csrs().mscratch.read());
-    debug!("sstatus = {:16x}", state.csrs().sstatus.read());
-    debug!("sie = {:16x}", state.csrs().sie.read());
-    debug!("stvec = {:16x}", state.csrs().stvec.read());
-    debug!("scounteren = {:16x}", state.csrs().scounteren.read());
+    // debug!("mtvec = {:16x}", state.csrs().mtvec.read());
+    // debug!("mscratch = {:16x}", state.csrs().mscratch.read());
+    // debug!("sstatus = {:16x}", state.csrs().sstatus.read());
+    // debug!("sie = {:16x}", state.csrs().sie.read());
+    // debug!("stvec = {:16x}", state.csrs().stvec.read());
+    // debug!("scounteren = {:16x}", state.csrs().scounteren.read());
     // debug!("senvcfg = {:16x}", state.csrs().senvcfg.read());
-    debug!("sscratch = {:16x}", state.csrs().sscratch.read());
-    debug!("sepc = {:16x}", state.csrs().sepc.read());
-    debug!("scause = {:16x}", state.csrs().scause.read());
-    debug!("stval = {:16x}", state.csrs().stval.read());
-    debug!("sip = {:16x}", state.csrs().sip.read());
-    debug!("satp = {:16x}", state.csrs().satp.read());
+    // debug!("sscratch = {:16x}", state.csrs().sscratch.read());
+    // debug!("sepc = {:16x}", state.csrs().sepc.read());
+    // debug!("scause = {:16x}", state.csrs().scause.read());
+    // debug!("stval = {:16x}", state.csrs().stval.read());
+    // debug!("sip = {:16x}", state.csrs().sip.read());
+    // debug!("satp = {:16x}", state.csrs().satp.read());
     // debug!("scontext = {:16x}", state.csrs().scontext.read());
     // debug!("hstatus = {:16x}", state.csrs().hstatus.read());
-    // debug!("hedeleg = {:16x}", state.csrs().hedeleg.read());
+    debug!("hedeleg = {:16x}", state.csrs().hedeleg.read());
     // debug!("hideleg = {:16x}", state.csrs().hideleg.read());
     // debug!("hie = {:16x}", state.csrs().hie.read());
     // debug!("hcounteren = {:16x}", state.csrs().hcounteren.read());
     // debug!("hgeie = {:16x}", state.csrs().hgeie.read());
     // debug!("htval = {:16x}", state.csrs().htval.read());
     // debug!("hip = {:16x}", state.csrs().hip.read());
     // debug!("hvip = {:16x}", state.csrs().hvip.read());
-    // debug!("htinst = {:16x}", state.csrs().htinst.read());
+    debug!("htinst = {:16x}", state.csrs().htinst.read());
     // debug!("hgeip = {:16x}", state.csrs().hgeip.read());
     // debug!("hgatp = {:16x}", state.csrs().hgatp.read());
     // debug!("hcontext = {:16x}", state.csrs().hcontext.read());