Repository: chipsalliance/caliptra-sw
Pull request #440 (2023-06-29 00:28:38)

[Bug Fix] Fixes for Image Validation Failure on hitless updates
This PR resolves the following:
1. When the update reset fails for some reason [like invalid image], the ROM was not transferring the control to the FMC. 
2. The Manifest regions were smaller than the actual manifest size. 
3. The registers were not getting locked in Update_Reset case


**NOTE**: 
We realized that the integration test coverage for the update_reset is not there in our integration test suite. This PR is just a Bug-Fix  and we did not want have integration test cases clubbed with this PR. Expect another PR which will contain additional integration tests to cover the update-reset test case. 

[https://github.com/chipsalliance/caliptra-sw/issues/438]




Patch:
diff --git a/Cargo.lock b/Cargo.lock
index ba037658..ca620855 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -619,6 +619,7 @@ version = "0.1.0"
 dependencies = [
  "bitfield",
  "caliptra-drivers",
+ "caliptra-registers",
  "ufmt",
  "zerocopy",
 ]
diff --git a/common/Cargo.toml b/common/Cargo.toml
index 59b59533..fb5e746b 100644
--- a/common/Cargo.toml
+++ b/common/Cargo.toml
@@ -10,6 +10,7 @@ edition = "2021"
 ufmt = "0.2.0"
 zerocopy = "0.6.1"
 caliptra-drivers = { path = "../drivers" }
+caliptra-registers = { path = "../registers" }
 bitfield = "0.14.0"
 
 
diff --git a/common/src/hand_off.rs b/common/src/hand_off.rs
index f7a174df..fc6e882c 100644
--- a/common/src/hand_off.rs
+++ b/common/src/hand_off.rs
@@ -1,8 +1,9 @@
 // Licensed under the Apache-2.0 license.
+use crate::helpers::*;
 use bitfield::{bitfield_bitrange, bitfield_fields};
 use caliptra_drivers::{
     report_fw_error_non_fatal, ColdResetEntry4, ColdResetEntry48, Ecc384PubKey, Ecc384Signature,
-    KeyId, WarmResetEntry4, WarmResetEntry48,
+    KeyId, ResetReason, WarmResetEntry4, WarmResetEntry48,
 };
 use zerocopy::{AsBytes, FromBytes};
 
@@ -377,8 +378,12 @@ impl FirmwareHandoffTable {
     /// Perform valdity check of the table's data.
     /// The fields below should have been populated by ROM with
     /// valid data before it transfers control to mutable code.
+    /// This function can only be called from non test case environment
+    /// as this function accesses the registers to get the reset_reason.
     pub fn is_valid(&self) -> bool {
-        self.fht_marker == FHT_MARKER
+        let reset_reason = reset_reason();
+
+        let mut valid = self.fht_marker == FHT_MARKER
             && self.fmc_cdi_kv_hdl != FHT_INVALID_HANDLE
             && self.manifest_load_addr != FHT_INVALID_ADDRESS
             && self.fmc_pub_key_x_dv_hdl != FHT_INVALID_HANDLE
@@ -390,13 +395,21 @@ impl FirmwareHandoffTable {
             && self.rt_fw_entry_point_hdl != FHT_INVALID_HANDLE
             // This is for Gen1 POR.
             && self.fips_fw_load_addr_hdl == FHT_INVALID_HANDLE
-            && self.ldevid_tbs_size != 0
-            && self.fmcalias_tbs_size != 0
             && self.ldevid_tbs_addr != 0
             && self.fmcalias_tbs_addr != 0
             && self.pcr_log_addr != 0
-            && self.fuse_log_addr != 0
+            && self.fuse_log_addr != 0;
+
+        if valid
+            && reset_reason == ResetReason::ColdReset
+            && (self.ldevid_tbs_size == 0 || self.fmcalias_tbs_size == 0)
+        {
+            valid = false;
+        }
+
+        valid
     }
+
     /// Load FHT from its fixed address and perform validity check of
     /// its data.
     pub fn try_load() -> Option<FirmwareHandoffTable> {
@@ -467,10 +480,31 @@ mod tests {
 
     #[test]
     fn test_fht_is_valid() {
-        let fht = FirmwareHandoffTable::default();
-        assert!(!fht.is_valid());
+        let fht = crate::hand_off::FirmwareHandoffTable::default();
+
+        let valid = fht.fht_marker == FHT_MARKER
+            && fht.fmc_cdi_kv_hdl != FHT_INVALID_HANDLE
+            && fht.manifest_load_addr != FHT_INVALID_ADDRESS
+            && fht.fmc_pub_key_x_dv_hdl != FHT_INVALID_HANDLE
+            && fht.fmc_pub_key_y_dv_hdl != FHT_INVALID_HANDLE
+            && fht.fmc_cert_sig_r_dv_hdl != FHT_INVALID_HANDLE
+            && fht.fmc_cert_sig_s_dv_hdl != FHT_INVALID_HANDLE
+            && fht.rt_fw_load_addr_hdl != FHT_INVALID_HANDLE
+            && fht.rt_tci_dv_hdl != FHT_INVALID_HANDLE
+            && fht.rt_fw_entry_point_hdl != FHT_INVALID_HANDLE
+            // This is for Gen1 POR.
+            && fht.fips_fw_load_addr_hdl == FHT_INVALID_HANDLE
+            && fht.ldevid_tbs_size == 0
+            && fht.fmcalias_tbs_size == 0
+            && fht.ldevid_tbs_addr != 0
+            && fht.fmcalias_tbs_addr != 0
+            && fht.pcr_log_addr != 0
+            && fht.fuse_log_addr != 0;
+
+        assert!(!valid);
         assert_eq!(FHT_SIZE, mem::size_of::<FirmwareHandoffTable>());
     }
+
     #[test]
     fn test_dv_nonsticky_384bit_set() {
         let fht = crate::hand_off::FirmwareHandoffTable {
diff --git a/common/src/helpers.rs b/common/src/helpers.rs
new file mode 100644
index 00000000..fa3a4f9e
--- /dev/null
+++ b/common/src/helpers.rs
@@ -0,0 +1,29 @@
+/*++
+Licensed under the Apache-2.0 license.
+
+File Name:
+
+    helpers.rs
+
+Abstract:
+
+    Helper Functions
+
+--*/
+
+use caliptra_drivers::ResetReason;
+
+pub fn reset_reason() -> ResetReason {
+    let soc_ifc = unsafe { caliptra_registers::soc_ifc::SocIfcReg::new() };
+
+    let soc_ifc_regs = soc_ifc.regs();
+    let bit0 = soc_ifc_regs.cptra_reset_reason().read().fw_upd_reset();
+    let bit1 = soc_ifc_regs.cptra_reset_reason().read().warm_reset();
+
+    match (bit0, bit1) {
+        (true, true) => ResetReason::Unknown,
+        (false, true) => ResetReason::WarmReset,
+        (true, false) => ResetReason::UpdateReset,
+        (false, false) => ResetReason::ColdReset,
+    }
+}
diff --git a/common/src/lib.rs b/common/src/lib.rs
index 25dec6a3..cd10aa6e 100644
--- a/common/src/lib.rs
+++ b/common/src/lib.rs
@@ -10,6 +10,7 @@ pub mod printer;
 pub mod boot_status;
 pub mod checksum;
 pub mod fuse;
+pub mod helpers;
 pub mod pcr;
 
 ///merge imports
diff --git a/fmc/memory.x b/fmc/memory.x
index 6b8edfd3..2075332d 100644
--- a/fmc/memory.x
+++ b/fmc/memory.x
@@ -4,7 +4,7 @@ ROM_ORG   	     = 0x00000000;
 META_ORG  		 = ROM_SIZE;
 ICCM_ORG  		 = 0x40000000;
 DCCM_ORG  		 = 0x50000000;
-FHT_ORG   		 = 0x50002000;
+FHT_ORG   		 = 0x50002800;
 DATA_ORG  		 = 0x50004400;
 STACK_ORG 		 = 0x5001C000;
 ESTACK_ORG  	 = 0x5001F800;
@@ -27,9 +27,9 @@ ROM_SIZE    = 32K;
 MBOX_SIZE   = 128K;
 ICCM_SIZE   = 128K;
 DCCM_SIZE   = 128K;
-MAN1_SIZE   = 4K;
-MAN2_SIZE   = 4K;
-FHT_SIZE    = 4K;
+MAN1_SIZE   = 5K;
+MAN2_SIZE   = 5K;
+FHT_SIZE    = 2K;
 DATA_SIZE   = 95K;
 STACK_SIZE  = 14K;
 ESTACK_SIZE = 1K;
diff --git a/rom/dev/src/fht.rs b/rom/dev/src/fht.rs
index f8b74519..85bc2f02 100644
--- a/rom/dev/src/fht.rs
+++ b/rom/dev/src/fht.rs
@@ -183,14 +183,14 @@ pub fn make_fht(env: &RomEnv) -> FirmwareHandoffTable {
     }
 }
 
-pub fn load_fht(fht: FirmwareHandoffTable) {
+pub fn store(fht: FirmwareHandoffTable) {
     extern "C" {
         static mut FHT_ORG: u8;
     }
 
     let slice = unsafe {
         let ptr = &mut FHT_ORG as *mut u8;
-        cprintln!("[fht] Loading FHT @ 0x{:08X}", ptr as u32);
+        cprintln!("[fht] Storing FHT @ 0x{:08X}", ptr as u32);
         core::slice::from_raw_parts_mut(ptr, core::mem::size_of::<FirmwareHandoffTable>())
     };
     caliptra_common::print_fht(&fht);
diff --git a/rom/dev/src/flow/update_reset.rs b/rom/dev/src/flow/update_reset.rs
index c40eeadc..10140c9d 100644
--- a/rom/dev/src/flow/update_reset.rs
+++ b/rom/dev/src/flow/update_reset.rs
@@ -76,7 +76,7 @@ impl UpdateResetFlow {
 
         Self::load_image(&manifest, recv_txn)?;
 
-        Self::copy_regions(&manifest);
+        Self::copy_regions();
         cprintln!("[update-reset Success] --");
         Ok(fht::make_fht(env))
     }
@@ -107,32 +107,18 @@ impl UpdateResetFlow {
     ///
     /// * `manifest` - Manifest
     ///
-    fn copy_regions(manifest: &ImageManifest) {
+    fn copy_regions() {
         cprintln!("[update-reset] Copying MAN_2 To MAN_1");
 
         let dst = unsafe {
             let ptr = &mut MAN1_ORG as *mut u32;
-            core::slice::from_raw_parts_mut(
-                ptr,
-                (core::mem::size_of::<ImageManifest>()
-                    + manifest.fmc.size as usize
-                    + manifest.runtime.size as usize
-                    + 3)
-                    / 4,
-            )
+            core::slice::from_raw_parts_mut(ptr, core::mem::size_of::<ImageManifest>())
         };
 
         let src = unsafe {
             let ptr = &mut MAN2_ORG as *mut u32;
 
-            core::slice::from_raw_parts_mut(
-                ptr,
-                (core::mem::size_of::<ImageManifest>()
-                    + manifest.fmc.size as usize
-                    + manifest.runtime.size as usize
-                    + 3)
-                    / 4,
-            )
+            core::slice::from_raw_parts_mut(ptr, core::mem::size_of::<ImageManifest>())
         };
         dst.clone_from_slice(src);
     }
diff --git a/rom/dev/src/lock.rs b/rom/dev/src/lock.rs
index ee7a957e..d975db57 100644
--- a/rom/dev/src/lock.rs
+++ b/rom/dev/src/lock.rs
@@ -28,10 +28,11 @@ pub fn lock_registers(env: &mut RomEnv, reset_reason: ResetReason) {
     cprintln!("[state] Locking Datavault");
     if reset_reason == ResetReason::ColdReset {
         lock_cold_reset_reg(env);
-        lock_warm_reset_reg(env);
-    } else if reset_reason == ResetReason::WarmReset {
-        lock_warm_reset_reg(env);
-    } else if reset_reason == ResetReason::UpdateReset {
+        lock_common_reg_set(env);
+    } else {
+        // For both UpdateReset and WarmReset, we lock the comm
+        // set of registers
+        lock_common_reg_set(env);
     }
 
     // Lock the PCR0 from clear
@@ -73,29 +74,29 @@ fn lock_cold_reset_reg(env: &mut RomEnv) {
         .lock_cold_reset_entry4(ColdResetEntry4::VendorPubKeyIndex);
 }
 
-/// Lock registers on a warm reset
+/// Lock all common registers across all reset types
 ///
 /// # Arguments
 ///
 /// * `env` - ROM Environment
-fn lock_warm_reset_reg(env: &mut RomEnv) {
-    // Lock the Runtime TCI in data vault until next warm reset
+fn lock_common_reg_set(env: &mut RomEnv) {
+    // Lock the Runtime TCI in data vault until next reset
     env.data_vault
         .lock_warm_reset_entry48(WarmResetEntry48::RtTci);
 
-    // Lock the Runtime SVN  in data vault until next warm reset
+    // Lock the Runtime SVN  in data vault until next reset
     env.data_vault
         .lock_warm_reset_entry4(WarmResetEntry4::RtSvn);
 
-    // Lock the Runtime load address in data vault until next warm reset
+    // Lock the Runtime load address in data vault until next reset
     env.data_vault
         .lock_warm_reset_entry4(WarmResetEntry4::RtLoadAddr);
 
-    // Lock the Runtime entry point in data vault until next warm reset
+    // Lock the Runtime entry point in data vault until next reset
     env.data_vault
         .lock_warm_reset_entry4(WarmResetEntry4::RtEntryPoint);
 
-    // Lock the Manifest addr in data vault until next warm reset
+    // Lock the Manifest addr in data vault until next reset
     env.data_vault
         .lock_warm_reset_entry4(WarmResetEntry4::ManifestAddr);
 }
diff --git a/rom/dev/src/main.rs b/rom/dev/src/main.rs
index ff130cb2..716ca4fe 100644
--- a/rom/dev/src/main.rs
+++ b/rom/dev/src/main.rs
@@ -18,8 +18,8 @@ use crate::lock::lock_registers;
 use core::hint::black_box;
 
 use caliptra_drivers::{
-    report_fw_error_non_fatal, CaliptraError, Ecc384, Hmac384, Mailbox, Sha256, Sha384, Sha384Acc,
-    SocIfc,
+    report_fw_error_non_fatal, CaliptraError, Ecc384, Hmac384, Mailbox, ResetReason, Sha256,
+    Sha384, Sha384Acc, SocIfc,
 };
 use rom_env::RomEnv;
 
@@ -82,22 +82,35 @@ pub extern "C" fn rom_entry() -> ! {
         report_error(err.into());
     }
 
+    let reset_reason = env.soc_ifc.reset_reason();
+
     let result = flow::run(&mut env);
     match result {
         Ok(fht) => {
-            // Lock the datavault registers.
-            let reset_reason = env.soc_ifc.reset_reason();
-            lock_registers(&mut env, reset_reason);
+            fht::store(fht);
+        }
 
-            fht::load_fht(fht);
+        Err(err) => {
+            //
+            // For the update reset case, when we fail the image validation
+            // we will need to continue to jump to the FMC after
+            // reporting the error in the registers.
+            //
+            if reset_reason == ResetReason::UpdateReset {
+                report_error_update_reset(err.into());
+            } else {
+                report_error(err.into());
+            }
         }
-        Err(err) => report_error(err.into()),
     }
 
     // Stop the watchdog timer.
     // [TODO] Reset the watchdog timer and let FMC take ownership of it.
     wdt::stop_wdt(&mut env.soc_ifc);
 
+    // Lock the datavault registers.
+    lock_registers(&mut env, reset_reason);
+
     #[cfg(not(feature = "no-fmc"))]
     launch_fmc(&mut env);
 
@@ -163,6 +176,11 @@ fn rom_panic(_: &core::panic::PanicInfo) -> ! {
     report_error(CaliptraError::ROM_GLOBAL_PANIC.into());
 }
 
+fn report_error_update_reset(code: u32) {
+    cprintln!("ROM Error: 0x{:08X}", code);
+    report_fw_error_non_fatal(code);
+}
+
 #[allow(clippy::empty_loop)]
 fn report_error(code: u32) -> ! {
     cprintln!("ROM Error: 0x{:08X}", code);
diff --git a/rom/dev/src/rom.ld b/rom/dev/src/rom.ld
index 4756a613..8e6343e2 100644
--- a/rom/dev/src/rom.ld
+++ b/rom/dev/src/rom.ld
@@ -21,8 +21,8 @@ MBOX_ORG         = 0x30000000;
 ICCM_ORG         = 0x40000000;
 DCCM_ORG         = 0x50000000;
 MAN1_ORG         = 0x50000000;
-MAN2_ORG         = 0x50001000;
-FHT_ORG          = 0x50002000;
+MAN2_ORG         = 0x50001400;
+FHT_ORG          = 0x50002800;
 LDEVID_TBS_ORG   = 0x50003000;
 FMCALIAS_TBS_ORG = 0x50003400;
 PCR_LOG_ORG      = 0x50003800;
@@ -42,9 +42,9 @@ ROM_SIZE          = 32K;
 MBOX_SIZE         = 128K;
 ICCM_SIZE         = 128K;
 DCCM_SIZE         = 128K;
-MAN1_SIZE         = 4K;
-MAN2_SIZE         = 4K;
-FHT_SIZE          = 4K;
+MAN1_SIZE         = 5K;
+MAN2_SIZE         = 5K;
+FHT_SIZE          = 2K;
 LDEVID_TBS_SIZE   = 1K;
 FMCALIAS_TBS_SIZE = 1K;
 PCR_LOG_SIZE      = 1K;
diff --git a/runtime/memory.x b/runtime/memory.x
index a71e8f5e..79925453 100644
--- a/runtime/memory.x
+++ b/runtime/memory.x
@@ -4,7 +4,7 @@ ROM_ORG   = 0x00000000;
 META_ORG  = ROM_SIZE;
 ICCM_ORG  = 0x40004000; /* Range [0x40000000 - 0x40003FFF] is reserved for FMC */
 DCCM_ORG  = 0x50000000;
-FHT_ORG   = 0x50002000;
+FHT_ORG   = 0x50002800;
 LDEVID_TBS_ORG   = 0x50003000;
 FMCALIAS_TBS_ORG = 0x50003400;
 RTALIAS_TBS_ORG  = 0x50004000;
@@ -20,9 +20,9 @@ ROM_SIZE    = 32K;
 MBOX_SIZE   = 128K;
 ICCM_SIZE   = 112K;
 DCCM_SIZE   = 128K;
-MAN1_SIZE   = 4K;
-MAN2_SIZE   = 4K;
-FHT_SIZE    = 4K;
+MAN1_SIZE   = 5K;
+MAN2_SIZE   = 5K;
+FHT_SIZE    = 2K;
 LDEVID_TBS_SIZE   = 1K;
 FMCALIAS_TBS_SIZE = 1K;
 RTALIAS_TBS_SIZE  = 1K;


Test patch:
diff --git a/fmc/test-fw/test-rt/memory.x b/fmc/test-fw/test-rt/memory.x
index 8449c09a..6cccf49f 100644
--- a/fmc/test-fw/test-rt/memory.x
+++ b/fmc/test-fw/test-rt/memory.x
@@ -4,7 +4,7 @@ ROM_ORG   = 0x00000000;
 META_ORG  = ROM_SIZE;
 ICCM_ORG  = 0x40004000; /* Range [0x40000000 - 0x40003FFF] is reserved for FMC */
 DCCM_ORG  = 0x50000000;
-FHT_ORG   = 0x50002000;
+FHT_ORG   = 0x50002800;
 DATA_ORG  = 0x50003000;
 STACK_ORG = 0x5001C000;
 ESTACK_ORG  = 0x5001F800;
@@ -15,9 +15,9 @@ ROM_SIZE    = 32K;
 MBOX_SIZE   = 128K;
 ICCM_SIZE   = 112K;
 DCCM_SIZE   = 128K;
-MAN1_SIZE   = 4K;
-MAN2_SIZE   = 4K;
-FHT_SIZE    = 4K;
+MAN1_SIZE   = 5K;
+MAN2_SIZE   = 5K;
+FHT_SIZE    = 2K;
 DATA_SIZE   = 100K;
 STACK_SIZE  = 14K;
 ESTACK_SIZE = 1K;
diff --git a/rom/dev/tools/test-fmc/src/fmc.ld b/rom/dev/tools/test-fmc/src/fmc.ld
index 9d25923b..c02995be 100644
--- a/rom/dev/tools/test-fmc/src/fmc.ld
+++ b/rom/dev/tools/test-fmc/src/fmc.ld
@@ -20,8 +20,8 @@ MBOX_ORG         = 0x30000000;
 ICCM_ORG         = 0x40000000;
 DCCM_ORG         = 0x50000000;
 MAN1_ORG         = 0x50000000;
-MAN2_ORG         = 0x50001000;
-FHT_ORG          = 0x50002000;
+MAN2_ORG         = 0x50001400;
+FHT_ORG          = 0x50002800;
 LDEVID_TBS_ORG   = 0x50003000;
 FMCALIAS_TBS_ORG = 0x50003400;
 PCR_LOG_ORG      = 0x50003800;
@@ -34,9 +34,9 @@ NSTACK_ORG       = 0x5001FC00;
 MBOX_SIZE         = 128K;
 ICCM_SIZE         = 128K;
 DCCM_SIZE         = 128K;
-MAN1_SIZE         = 4K;
-MAN2_SIZE         = 4K;
-FHT_SIZE          = 4K;
+MAN1_SIZE         = 5K;
+MAN2_SIZE         = 5K;
+FHT_SIZE          = 2K;
 LDEVID_TBS_SIZE   = 1K;
 FMCALIAS_TBS_SIZE = 1K;
 PCR_LOG_SIZE      = 1K;
diff --git a/rom/dev/tools/test-fmc/src/main.rs b/rom/dev/tools/test-fmc/src/main.rs
index 4e644ccc..af635a1b 100644
--- a/rom/dev/tools/test-fmc/src/main.rs
+++ b/rom/dev/tools/test-fmc/src/main.rs
@@ -25,8 +25,7 @@ extern "C" {
 use caliptra_common::FirmwareHandoffTable;
 use caliptra_common::{FuseLogEntry, FuseLogEntryId};
 use caliptra_common::{PcrLogEntry, PcrLogEntryId};
-use caliptra_drivers::DataVault;
-use caliptra_drivers::Mailbox;
+use caliptra_drivers::{DataVault, Mailbox};
 use caliptra_registers::dv::DvReg;
 use caliptra_x509::{Ecdsa384CertBuilder, Ecdsa384Signature, FmcAliasCertTbs, LocalDevIdCertTbs};
 use ureg::RealMmioMut;
diff --git a/rom/dev/tools/test-rt/src/rt.ld b/rom/dev/tools/test-rt/src/rt.ld
index 6de316b6..963e0aa3 100644
--- a/rom/dev/tools/test-rt/src/rt.ld
+++ b/rom/dev/tools/test-rt/src/rt.ld
@@ -4,7 +4,7 @@ Licensed under the Apache-2.0 license.
 
 File Name:
 
-	fmc.ld
+	rt.ld
 
 Abstract:
 
@@ -20,8 +20,8 @@ MBOX_ORG    = 0x30000000;
 ICCM_ORG    = 0x40004000; /* Range [0x40000000 - 0x40003FFF] is reserved for FMC */
 DCCM_ORG    = 0x50000000;
 MAN1_ORG    = 0x50000000;
-MAN2_ORG    = 0x50001000;
-FHT_ORG     = 0x50002000;
+MAN2_ORG    = 0x50001400;
+FHT_ORG     = 0x50002800;
 DATA_ORG    = 0x50003000;
 STACK_ORG   = 0x5001C000;
 ESTACK_ORG  = 0x5001F800;
@@ -30,9 +30,9 @@ NSTACK_ORG  = 0x5001FC00;
 MBOX_SIZE   = 128K;
 ICCM_SIZE   = 112K;
 DCCM_SIZE   = 128K;
-MAN1_SIZE   = 4K;
-MAN2_SIZE   = 4K;
-FHT_SIZE    = 4K;
+MAN1_SIZE   = 5K;
+MAN2_SIZE   = 5K;
+FHT_SIZE    = 2K;
 DATA_SIZE   = 100K;
 STACK_SIZE  = 14K;
 ESTACK_SIZE = 1K;