[ownership] Fix protect_when_active behavior

1. When this feature was named `protect_when_primary`, I did the seemingly correct thing and protected the slot that was the primary boot slot. This is, in fact, not what this feature should actually do; it _should_ protect the active slot. 2. Update the test to verify the behavior of the feature in both slots. Signed-off-by: Chris Frantz <[email protected]> (cherry picked from commit 6cdc175)
lowRISC · Nov 13, 2024 · 30c46ae · 30c46ae
1 parent 73e897e
commit 30c46ae
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 44 deletions.
diff --git a/sw/device/silicon_creator/lib/ownership/ownership.c b/sw/device/silicon_creator/lib/ownership/ownership.c
@@ -272,14 +272,13 @@ rom_error_t ownership_init(boot_data_t *bootdata, owner_config_t *config,
 }
 
 rom_error_t ownership_flash_lockdown(boot_data_t *bootdata,
+                                     uint32_t active_slot,
                                      const owner_config_t *config) {
   if (bootdata->ownership_state == kOwnershipStateLockedOwner) {
-    HARDENED_RETURN_IF_ERROR(
-        owner_block_flash_apply(config->flash, kBootSlotA,
-                                /*lockdown=*/bootdata->primary_bl0_slot));
-    HARDENED_RETURN_IF_ERROR(
-        owner_block_flash_apply(config->flash, kBootSlotB,
-                                /*lockdown=*/bootdata->primary_bl0_slot));
+    HARDENED_RETURN_IF_ERROR(owner_block_flash_apply(config->flash, kBootSlotA,
+                                                     /*lockdown=*/active_slot));
+    HARDENED_RETURN_IF_ERROR(owner_block_flash_apply(config->flash, kBootSlotB,
+                                                     /*lockdown=*/active_slot));
   } else {
     HARDENED_CHECK_NE(bootdata->ownership_state, kOwnershipStateLockedOwner);
   }

diff --git a/sw/device/silicon_creator/lib/ownership/ownership.h b/sw/device/silicon_creator/lib/ownership/ownership.h
@@ -21,10 +21,12 @@ rom_error_t ownership_init(boot_data_t *bootdata, owner_config_t *config,
  * Lockdown the flash configuration.
  *
  * @param bootdata The current bootdata.
+ * @param active_slot The active slot.
  * @param config The current owner configuration.
  * @return error state.
  */
 rom_error_t ownership_flash_lockdown(boot_data_t *bootdata,
+                                     uint32_t active_slot,
                                      const owner_config_t *config);
 
 /**

diff --git a/sw/device/silicon_creator/rom_ext/e2e/ownership/BUILD b/sw/device/silicon_creator/rom_ext/e2e/ownership/BUILD
@@ -52,6 +52,7 @@ opentitan_binary(
     exec_env = {
         "//hw/top_earlgrey:fpga_cw310_rom_ext": None,
     },
+    linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_virtual",
     deps = [
         "//hw/top_earlgrey/sw/autogen:top_earlgrey",
         "//sw/device/lib/base:status",
@@ -397,34 +398,40 @@ ownership_transfer_test(
 # rom_ext_e2e_testplan.hjson%rom_ext_e2e_flash_permission_test
 # Note: rescue-after-activate tests that rescue correctly accesses regions with
 # different scrambling/ECC properties than the default flash configuration.
-ownership_transfer_test(
-    name = "flash_permission_test",
-    srcs = ["flash_regions.c"],
-    fpga = fpga_params(
-        binaries = {
-            ":flash_regions": "flash_regions",
-        },
-        test_cmd = """
-            --clear-bitstream
-            --bootstrap={firmware}
-            --unlock-mode=Any
-            --unlock-key=$(location //sw/device/silicon_creator/lib/ownership/keys/fake:unlock_key)
-            --next-owner-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:owner_key)
-            --next-unlock-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:unlock_key)
-            --next-activate-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:activate_key)
-            --next-application-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:app_prod_ecdsa_pub)
-            --config-kind=with-flash-locked
-            --rescue-after-activate={flash_regions}
-        """,
-        test_harness = "//sw/host/tests/ownership:flash_permission_test",
-    ),
-    deps = [
-        "//hw/top_earlgrey/sw/autogen:top_earlgrey",
-        "//sw/device/lib/base:status",
-        "//sw/device/lib/dif:flash_ctrl",
-        "//sw/device/lib/testing/test_framework:ottf_main",
-    ],
-)
+[
+    ownership_transfer_test(
+        name = "flash_permission_test_slot_{}".format(slot),
+        srcs = ["flash_regions.c"],
+        fpga = fpga_params(
+            binaries = {
+                ":flash_regions": "flash_regions",
+            },
+            slot = "Slot{}".format(slot.upper()),
+            test_cmd = """
+                --clear-bitstream
+                --bootstrap={firmware}
+                --unlock-mode=Any
+                --unlock-key=$(location //sw/device/silicon_creator/lib/ownership/keys/fake:unlock_key)
+                --next-owner-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:owner_key)
+                --next-unlock-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:unlock_key)
+                --next-activate-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:activate_key)
+                --next-application-key=$(location //sw/device/silicon_creator/lib/ownership/keys/dummy:app_prod_ecdsa_pub)
+                --config-kind=with-flash-locked
+                --rescue-after-activate={flash_regions}
+                --rescue-slot={slot}
+            """,
+            test_harness = "//sw/host/tests/ownership:flash_permission_test",
+        ),
+        linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_virtual",
+        deps = [
+            "//hw/top_earlgrey/sw/autogen:top_earlgrey",
+            "//sw/device/lib/base:status",
+            "//sw/device/lib/dif:flash_ctrl",
+            "//sw/device/lib/testing/test_framework:ottf_main",
+        ],
+    )
+    for slot in ("a", "b")
+]
 
 # Tests that a owner config with an improper flash configuration cannot be activated.
 ownership_transfer_test(

diff --git a/sw/device/silicon_creator/rom_ext/rom_ext.c b/sw/device/silicon_creator/rom_ext/rom_ext.c
@@ -260,6 +260,7 @@ void rom_ext_sram_exec(owner_sram_exec_mode_t mode) {
 OT_WARN_UNUSED_RESULT
 static rom_error_t rom_ext_verify(const manifest_t *manifest,
                                   const boot_data_t *boot_data) {
+  dbg_print_epmp();
   RETURN_IF_ERROR(rom_ext_boot_policy_manifest_check(manifest, boot_data));
   ownership_key_alg_t key_alg = kOwnershipKeyAlgEcdsaP256;
   RETURN_IF_ERROR(owner_keyring_find_key(
@@ -548,7 +549,7 @@ static rom_error_t rom_ext_attestation_owner(const boot_data_t *boot_data,
 }
 
 OT_WARN_UNUSED_RESULT
-static rom_error_t rom_ext_boot(boot_data_t *boot_data,
+static rom_error_t rom_ext_boot(boot_data_t *boot_data, boot_log_t *boot_log,
                                 const manifest_t *manifest) {
   // Generate CDI_1 attestation keys and certificate.
   HARDENED_RETURN_IF_ERROR(rom_ext_attestation_owner(boot_data, manifest));
@@ -692,7 +693,8 @@ static rom_error_t rom_ext_boot(boot_data_t *boot_data,
   ibex_addr_remap_lockdown(1);
 
   // Lock the flash according to the ownership configuration.
-  HARDENED_RETURN_IF_ERROR(ownership_flash_lockdown(boot_data, &owner_config));
+  HARDENED_RETURN_IF_ERROR(
+      ownership_flash_lockdown(boot_data, boot_log->bl0_slot, &owner_config));
 
   dbg_print_epmp();
 
@@ -881,7 +883,7 @@ static rom_error_t rom_ext_try_next_stage(boot_data_t *boot_data,
     boot_log_digest_update(boot_log);
 
     // Boot fails if a verified ROM_EXT cannot be booted.
-    RETURN_IF_ERROR(rom_ext_boot(boot_data, manifests.ordered[i]));
+    RETURN_IF_ERROR(rom_ext_boot(boot_data, boot_log, manifests.ordered[i]));
     // `rom_ext_boot()` should never return `kErrorOk`, but if it does
     // we must shut down the chip instead of trying the next ROM_EXT.
     return kErrorRomExtBootFailed;

diff --git a/sw/host/opentitanlib/src/chip/boot_svc.rs b/sw/host/opentitanlib/src/chip/boot_svc.rs
@@ -52,6 +52,16 @@ with_unknown! {
     }
 }
 
+impl BootSlot {
+    pub fn opposite(self) -> Result<Self> {
+        match self {
+            BootSlot::SlotA => Ok(BootSlot::SlotB),
+            BootSlot::SlotB => Ok(BootSlot::SlotA),
+            _ => Err(ChipDataError::BadSlot(self).into()),
+        }
+    }
+}
+
 /// The Boot Services header common to all boot services commands and responses.
 #[derive(Debug, Default, Serialize, Annotate)]
 pub struct Header {

diff --git a/sw/host/opentitanlib/src/chip/mod.rs b/sw/host/opentitanlib/src/chip/mod.rs
@@ -21,6 +21,8 @@ pub enum ChipDataError {
     Anyhow(#[from] anyhow::Error),
     #[error("bad size: expected {0} bytes, but found {1}")]
     BadSize(usize, usize),
+    #[error("bad slot: {0:x}")]
+    BadSlot(boot_svc::BootSlot),
     #[error("invalid digest")]
     InvalidDigest,
 }
diff --git a/sw/host/tests/ownership/flash_permission_test.rs b/sw/host/tests/ownership/flash_permission_test.rs
@@ -53,6 +53,8 @@ struct Opts {
         help = "Load a firmware payload via rescue after activating ownership"
     )]
     rescue_after_activate: Option<PathBuf>,
+    #[arg(long, default_value = "SlotA", help = "Which slot to rescue into")]
+    rescue_slot: BootSlot,
 
     #[arg(long, default_value_t = true, action = clap::ArgAction::Set, help = "Check the firmware boot in dual-owner mode")]
     dual_owner_boot_check: bool,
@@ -226,7 +228,12 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<()
     if let Some(fw) = &opts.rescue_after_activate {
         let data = std::fs::read(fw)?;
         rescue.enter(transport, /*reset_target=*/ true)?;
-        rescue.update_firmware(BootSlot::SlotA, &data)?;
+        rescue.wait()?;
+        rescue.update_firmware(opts.rescue_slot, &data)?;
+        // Clear the opposite slot because we changed the scrambling/ecc settings
+        // for the application area of flash.
+        rescue.update_firmware(opts.rescue_slot.opposite()?, &[0xFFu8])?;
+        rescue.reboot()?;
     }
 
     log::info!("###### Boot After Transfer Complete ######");
@@ -241,7 +248,18 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<()
         return RomError(u32::from_str_radix(&capture[2], 16)?).into();
     }
     let region = FlashRegion::find_all(&capture[1])?;
-    // Flash SideA is the primary side and has protect_when_active = true.
+    // The rescue_slot shoudl be the active side and has protect_when_active = true.
+    let (romext_region, app_region) = match opts.rescue_slot {
+        BootSlot::SlotA => (
+            ["RD-xx-xx-xx-xx-xx", "RD-WR-ER-xx-xx-xx"],
+            ["RD-xx-xx-SC-EC-xx", "RD-WR-ER-SC-EC-xx"],
+        ),
+        BootSlot::SlotB => (
+            ["RD-WR-ER-xx-xx-xx", "RD-xx-xx-xx-xx-xx"],
+            ["RD-WR-ER-SC-EC-xx", "RD-xx-xx-SC-EC-xx"],
+        ),
+        _ => return Err(anyhow!("Unknown boot slot {}", data.bl0_slot)),
+    };
     //
     // Since we are in a locked ownership state, we expect the region configuration
     // to reflect both the `protect_when_active` and `lock` properties of the
@@ -251,26 +269,27 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<()
     } else {
         "UN"
     };
+    // Flash Slot A:
     assert_eq!(
         region[0],
-        FlashRegion("data", 0, 0, 32, "RD-xx-xx-xx-xx-xx", locked)
+        FlashRegion("data", 0, 0, 32, romext_region[0], locked)
     );
     assert_eq!(
         region[1],
-        FlashRegion("data", 1, 32, 192, "RD-xx-xx-SC-EC-xx", locked)
+        FlashRegion("data", 1, 32, 192, app_region[0], locked)
     );
     assert_eq!(
         region[2],
         FlashRegion("data", 2, 224, 32, "RD-WR-ER-xx-xx-HE", locked)
     );
-    // Flash SideB is the secondary side, so protect_when_active doesn't apply.
+    // Flash Slot B:
     assert_eq!(
         region[3],
-        FlashRegion("data", 3, 256, 32, "RD-WR-ER-xx-xx-xx", locked)
+        FlashRegion("data", 3, 256, 32, romext_region[1], locked)
     );
     assert_eq!(
         region[4],
-        FlashRegion("data", 4, 288, 192, "RD-WR-ER-SC-EC-xx", locked)
+        FlashRegion("data", 4, 288, 192, app_region[1], locked)
     );
     assert_eq!(
         region[5],