From 30c46aee86a807f0c1df6e539a48302a9e53abec Mon Sep 17 00:00:00 2001 From: Chris Frantz Date: Thu, 24 Oct 2024 15:22:28 -0700 Subject: [PATCH] [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 (cherry picked from commit 6cdc175ed13f2c8e78ea0087734da97d0251f9d9) --- .../silicon_creator/lib/ownership/ownership.c | 11 ++-- .../silicon_creator/lib/ownership/ownership.h | 2 + .../rom_ext/e2e/ownership/BUILD | 63 ++++++++++--------- sw/device/silicon_creator/rom_ext/rom_ext.c | 8 ++- sw/host/opentitanlib/src/chip/boot_svc.rs | 10 +++ sw/host/opentitanlib/src/chip/mod.rs | 2 + .../tests/ownership/flash_permission_test.rs | 33 +++++++--- 7 files changed, 85 insertions(+), 44 deletions(-) diff --git a/sw/device/silicon_creator/lib/ownership/ownership.c b/sw/device/silicon_creator/lib/ownership/ownership.c index 275e8c83dddb2..9a8fff7b2dedb 100644 --- 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 index a15f41d26f59f..1406d5e205c81 100644 --- 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 index b720fa4444587..a0715ec9c50d3 100644 --- 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 index bdc5c935eafa1..43db3c1e325a8 100644 --- 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 index 57819a2a9d3c3..3e4649f10485b 100644 --- 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 { + 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 index 551185ac24b20..1d32afc726eeb 100644 --- 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 index cf65dbc66a8eb..00a70165e32e7 100644 --- 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, + #[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],