From 96782d9ff520f95e7d093a2950dcfc53c73882ea Mon Sep 17 00:00:00 2001 From: Collin MacDonald Date: Mon, 14 Jul 2025 15:12:53 -0700 Subject: [PATCH 1/3] [rom_ext] Add proper bounds to max_sec_ver Adds an additional check to ensure that the requested security version is not higher than the lowest security version in Slot A or Slot B. Also corrects the requested_min_sec_ver variable to use the proper field name. Signed-off-by: Collin MacDonald --- .../rom_ext/rom_ext_boot_services.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/sw/device/silicon_creator/rom_ext/rom_ext_boot_services.c b/sw/device/silicon_creator/rom_ext/rom_ext_boot_services.c index 91403b4145e8f..97d62bf6a626b 100644 --- a/sw/device/silicon_creator/rom_ext/rom_ext_boot_services.c +++ b/sw/device/silicon_creator/rom_ext/rom_ext_boot_services.c @@ -80,13 +80,14 @@ static rom_error_t boot_svc_min_sec_ver_handler( uint32_t *isfb_check_count) { const uint32_t current_min_sec_ver = boot_data->min_security_version_bl0; const uint32_t requested_min_sec_ver = - boot_svc_msg->next_boot_bl0_slot_req.next_bl0_slot; + boot_svc_msg->min_bl0_sec_ver_req.min_bl0_sec_ver; // Ensure the requested minimum security version isn't lower than the current // minimum security version. if (launder32(requested_min_sec_ver) > current_min_sec_ver) { HARDENED_CHECK_GT(requested_min_sec_ver, current_min_sec_ver); - uint32_t max_sec_ver = current_min_sec_ver; + uint32_t slot_a_max_sec_ver = UINT32_MAX; + uint32_t slot_b_max_sec_ver = UINT32_MAX; // Check the two flash slots for valid manifests and determine the maximum // value of the new minimum_security_version. This prevents a malicious @@ -96,18 +97,21 @@ static rom_error_t boot_svc_min_sec_ver_handler( rom_error_t error = rom_ext_verify(manifest, boot_data, &flash_exec, keyring, verify_key, owner_config, isfb_check_count); - if (error == kErrorOk && manifest->security_version > max_sec_ver) { - max_sec_ver = manifest->security_version; + if (error == kErrorOk) { + slot_a_max_sec_ver = manifest->security_version; } manifest = rom_ext_boot_policy_manifest_b_get(); error = rom_ext_verify(manifest, boot_data, &flash_exec, keyring, verify_key, owner_config, isfb_check_count); - if (error == kErrorOk && manifest->security_version > max_sec_ver) { - max_sec_ver = manifest->security_version; + if (error == kErrorOk) { + slot_b_max_sec_ver = manifest->security_version; } - if (requested_min_sec_ver <= max_sec_ver) { + uint32_t max_sec_ver = MIN(slot_a_max_sec_ver, slot_b_max_sec_ver); + + if (requested_min_sec_ver <= max_sec_ver && max_sec_ver != UINT32_MAX) { HARDENED_CHECK_LE(requested_min_sec_ver, max_sec_ver); + HARDENED_CHECK_NE(max_sec_ver, UINT32_MAX); // Update boot data to the requested minimum BL0 security version. boot_data->min_security_version_bl0 = requested_min_sec_ver; From fa47ca2715aab7d657556e189a3c17cc925cd74c Mon Sep 17 00:00:00 2001 From: Collin MacDonald Date: Mon, 21 Jul 2025 14:07:49 -0700 Subject: [PATCH 2/3] [rom_ext] Update Next Security Version unit tests Updates the existing unit test to use the proper field name, and adds new unit tests to verify that invalid security versions are correctly rejected. Signed-off-by: Collin MacDonald --- .../rom_ext/rom_ext_boot_services_unittest.cc | 171 +++++++++++++++++- 1 file changed, 162 insertions(+), 9 deletions(-) diff --git a/sw/device/silicon_creator/rom_ext/rom_ext_boot_services_unittest.cc b/sw/device/silicon_creator/rom_ext/rom_ext_boot_services_unittest.cc index ba1a9589dc8f5..f44590ecda0ca 100644 --- a/sw/device/silicon_creator/rom_ext/rom_ext_boot_services_unittest.cc +++ b/sw/device/silicon_creator/rom_ext/rom_ext_boot_services_unittest.cc @@ -191,13 +191,13 @@ TEST_F(RomExtBootServicesTest, BootSvcNextBl0Slot) { EXPECT_EQ(boot_svc_msg.next_boot_bl0_slot_res.status, kErrorOk); } -TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVer) { +TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVerValid) { boot_svc_msg.header.identifier = kBootSvcIdentifier; boot_svc_msg.header.type = kBootSvcMinBl0SecVerReqType; - boot_svc_msg.header.length = sizeof(boot_svc_next_boot_bl0_slot_req_t); + boot_svc_msg.header.length = sizeof(boot_svc_min_bl0_sec_ver_req_t); boot_svc_msg.header.digest = hmac_digest_t{0x1234}; - boot_svc_msg.next_boot_bl0_slot_req.next_bl0_slot = 2; + boot_svc_msg.min_bl0_sec_ver_req.min_bl0_sec_ver = 2; boot_data.min_security_version_bl0 = 1; @@ -215,8 +215,6 @@ TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVer) { keyring.length = 1; keyring.key[0] = &key; - manifest_ext_spx_key_t spx_key = {.key{.data = {0}}}; - manifest_t manifest_a{}; manifest_t manifest_b{}; @@ -248,8 +246,9 @@ TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVer) { EXPECT_CALL(rom_ext_boot_policy_ptrs_, ManifestA) .WillOnce(Return(&manifest_a)); EXPECT_CALL(mock_manifest_, SpxKey) - .WillOnce(DoAll(SetArgPointee<1>(&spx_key), Return(kErrorOk))); - EXPECT_CALL(mock_manifest_, SpxSignature).WillOnce(Return(kErrorOk)); + .WillOnce(Return(kErrorManifestBadExtension)); + EXPECT_CALL(mock_manifest_, SpxSignature) + .WillOnce(Return(kErrorManifestBadExtension)); EXPECT_CALL(mock_rnd_, Uint32); EXPECT_CALL(mock_hmac_, sha256_init); @@ -268,8 +267,9 @@ TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVer) { EXPECT_CALL(rom_ext_boot_policy_ptrs_, ManifestB) .WillOnce(Return(&manifest_b)); EXPECT_CALL(mock_manifest_, SpxKey) - .WillOnce(DoAll(SetArgPointee<1>(&spx_key), Return(kErrorOk))); - EXPECT_CALL(mock_manifest_, SpxSignature).WillOnce(Return(kErrorOk)); + .WillOnce(Return(kErrorManifestBadExtension)); + EXPECT_CALL(mock_manifest_, SpxSignature) + .WillOnce(Return(kErrorManifestBadExtension)); EXPECT_CALL(mock_rnd_, Uint32); EXPECT_CALL(mock_hmac_, sha256_init); @@ -304,6 +304,159 @@ TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVer) { EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.status, kErrorOk); } +TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVerInvalidLow) { + boot_svc_msg.header.identifier = kBootSvcIdentifier; + boot_svc_msg.header.type = kBootSvcMinBl0SecVerReqType; + boot_svc_msg.header.length = sizeof(boot_svc_min_bl0_sec_ver_req_t); + boot_svc_msg.header.digest = hmac_digest_t{0x1234}; + + boot_svc_msg.min_bl0_sec_ver_req.min_bl0_sec_ver = 0; + + boot_data.min_security_version_bl0 = 1; + + owner_config.isfb = (owner_isfb_config_t *)kHardenedBoolFalse; + + owner_application_key_t key = { + .header = + { + .tag = kTlvTagApplicationKey, + .length = sizeof(owner_application_key_t), + }, + .key_alg = kOwnershipKeyAlgEcdsaP256, + }; + + keyring.length = 1; + keyring.key[0] = &key; + + EXPECT_CALL(mock_hmac_, sha256) + .WillOnce(SetArgPointee<2>(hmac_digest_t{0x1234})); + + EXPECT_CALL(mock_hmac_, sha256) + .WillOnce(SetArgPointee<2>(hmac_digest_t{0x1234})); + + EXPECT_EQ( + boot_svc_handler(&boot_svc_msg, &boot_data, &boot_log, lc_state, &keyring, + &verify_key, &owner_config, &isfb_check_count), + kErrorOk); + + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.header.identifier, + kBootSvcIdentifier); + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.header.type, + kBootSvcMinBl0SecVerResType); + + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.status, kErrorBootSvcBadSecVer); +} + +TEST_F(RomExtBootServicesTest, BootSvcMinBl0SecVerInvalidHigh) { + boot_svc_msg.header.identifier = kBootSvcIdentifier; + boot_svc_msg.header.type = kBootSvcMinBl0SecVerReqType; + boot_svc_msg.header.length = sizeof(boot_svc_min_bl0_sec_ver_req_t); + boot_svc_msg.header.digest = hmac_digest_t{0x1234}; + + boot_svc_msg.min_bl0_sec_ver_req.min_bl0_sec_ver = 3; + + boot_data.min_security_version_bl0 = 1; + + owner_config.isfb = (owner_isfb_config_t *)kHardenedBoolFalse; + + owner_application_key_t key = { + .header = + { + .tag = kTlvTagApplicationKey, + .length = sizeof(owner_application_key_t), + }, + .key_alg = kOwnershipKeyAlgEcdsaP256, + }; + + keyring.length = 1; + keyring.key[0] = &key; + + manifest_t manifest_a{}; + manifest_t manifest_b{}; + + manifest_a.identifier = CHIP_BL0_IDENTIFIER; + manifest_a.length = CHIP_BL0_SIZE_MIN; + manifest_a.security_version = 2; + manifest_a.manifest_version.major = kManifestVersionMajor2; + manifest_a.length = sizeof(manifest_t) + 0x1000; + manifest_a.signed_region_end = sizeof(manifest_t) + 0x900; + manifest_a.code_start = sizeof(manifest_t); + manifest_a.code_end = sizeof(manifest_t) + 0x800; + manifest_a.entry_point = 0x500; + manifest_a.ecdsa_public_key.x[0] = 0; + + manifest_b.identifier = CHIP_BL0_IDENTIFIER; + manifest_b.length = CHIP_BL0_SIZE_MIN; + manifest_b.security_version = 3; + manifest_b.manifest_version.major = kManifestVersionMajor2; + manifest_b.length = sizeof(manifest_t) + 0x1000; + manifest_b.signed_region_end = sizeof(manifest_t) + 0x900; + manifest_b.code_start = sizeof(manifest_t); + manifest_b.code_end = sizeof(manifest_t) + 0x800; + manifest_b.entry_point = 0x500; + manifest_b.ecdsa_public_key.x[0] = 0; + + EXPECT_CALL(mock_hmac_, sha256) + .WillOnce(SetArgPointee<2>(hmac_digest_t{0x1234})); + + EXPECT_CALL(rom_ext_boot_policy_ptrs_, ManifestA) + .WillOnce(Return(&manifest_a)); + EXPECT_CALL(mock_manifest_, SpxKey) + .WillOnce(Return(kErrorManifestBadExtension)); + EXPECT_CALL(mock_manifest_, SpxSignature) + .WillOnce(Return(kErrorManifestBadExtension)); + + EXPECT_CALL(mock_rnd_, Uint32); + EXPECT_CALL(mock_hmac_, sha256_init); + EXPECT_CALL(mock_lifecycle_, DeviceId); + EXPECT_CALL(mock_otp_, read32); + EXPECT_CALL(mock_otp_, read32); + EXPECT_CALL(mock_lifecycle_, State); + EXPECT_CALL(mock_hmac_, sha256_update); + EXPECT_CALL(mock_manifest_, DigestRegion); + EXPECT_CALL(mock_hmac_, sha256_update); + EXPECT_CALL(mock_hmac_, sha256_process); + EXPECT_CALL(mock_hmac_, sha256_final); + + EXPECT_CALL(mock_owner_verify_, verify).WillOnce(Return(kErrorOk)); + + EXPECT_CALL(rom_ext_boot_policy_ptrs_, ManifestB) + .WillOnce(Return(&manifest_b)); + EXPECT_CALL(mock_manifest_, SpxKey) + .WillOnce(Return(kErrorManifestBadExtension)); + EXPECT_CALL(mock_manifest_, SpxSignature) + .WillOnce(Return(kErrorManifestBadExtension)); + + EXPECT_CALL(mock_rnd_, Uint32); + EXPECT_CALL(mock_hmac_, sha256_init); + EXPECT_CALL(mock_lifecycle_, DeviceId); + EXPECT_CALL(mock_otp_, read32); + EXPECT_CALL(mock_otp_, read32); + EXPECT_CALL(mock_lifecycle_, State); + EXPECT_CALL(mock_hmac_, sha256_update); + EXPECT_CALL(mock_manifest_, DigestRegion); + EXPECT_CALL(mock_hmac_, sha256_update); + EXPECT_CALL(mock_hmac_, sha256_process); + EXPECT_CALL(mock_hmac_, sha256_final); + + EXPECT_CALL(mock_owner_verify_, verify).WillOnce(Return(kErrorOk)); + + EXPECT_CALL(mock_hmac_, sha256) + .WillOnce(SetArgPointee<2>(hmac_digest_t{0x1234})); + + EXPECT_EQ( + boot_svc_handler(&boot_svc_msg, &boot_data, &boot_log, lc_state, &keyring, + &verify_key, &owner_config, &isfb_check_count), + kErrorOk); + + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.header.identifier, + kBootSvcIdentifier); + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.header.type, + kBootSvcMinBl0SecVerResType); + + EXPECT_EQ(boot_svc_msg.min_bl0_sec_ver_res.status, kErrorBootSvcBadSecVer); +} + TEST_F(RomExtBootServicesTest, BootSvcOwnershipUnlock) { boot_svc_msg.header.identifier = kBootSvcIdentifier; boot_svc_msg.header.type = kBootSvcOwnershipUnlockReqType; From 1819bfc19126eede0c9d1a48c7365532bc6efc10 Mon Sep 17 00:00:00 2001 From: Collin MacDonald Date: Mon, 21 Jul 2025 14:08:30 -0700 Subject: [PATCH 3/3] [rom_ext] Update Next Security Version e2e tests Updates the existing e2e test with additional log statements, and update the build file to use the correct manifest versions. Signed-off-by: Collin MacDonald --- .../silicon_creator/rom_ext/e2e/boot_svc/BUILD | 17 ++++++++++++----- .../e2e/boot_svc/boot_svc_min_sec_ver_test.c | 7 +++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD b/sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD index 8828a83be5259..a47fdf87ae17e 100644 --- a/sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD +++ b/sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD @@ -221,8 +221,15 @@ manifest({ "identifier": hex(CONST.OWNER), }) +manifest({ + "name": "manifest_version_5", + "address_translation": hex(CONST.HARDENED_TRUE), + "security_version": "5", + "identifier": hex(CONST.OWNER), +}) + opentitan_binary( - name = "min_sec_ver_4", + name = "min_sec_ver_5", testonly = True, srcs = ["boot_svc_min_sec_ver_test.c"], exec_env = [ @@ -230,7 +237,7 @@ opentitan_binary( "//hw/top_earlgrey:fpga_cw340_rom_ext", ], linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_virtual", - manifest = ":manifest_version_4", + manifest = ":manifest_version_5", deps = [ ":boot_svc_test_lib", "//sw/device/lib/base:status", @@ -253,9 +260,9 @@ opentitan_test( "//hw/top_earlgrey:fpga_cw340_rom_ext": None, }, fpga = fpga_params( - assemble = "{rom_ext}@0 {firmware}@0x10000 {min_sec_ver_4:signed_bin}@0x90000", + assemble = "{rom_ext}@0 {firmware}@0x10000 {min_sec_ver_5:signed_bin}@0x90000", binaries = { - ":min_sec_ver_4": "min_sec_ver_4", + ":min_sec_ver_5": "min_sec_ver_5", }, test_cmd = """ --exec="transport init" @@ -268,7 +275,7 @@ opentitan_test( """, ), linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_virtual", - manifest = "//sw/device/silicon_owner:manifest", + manifest = ":manifest_version_4", deps = [ ":boot_svc_test_lib", "//sw/device/lib/base:status", diff --git a/sw/device/silicon_creator/rom_ext/e2e/boot_svc/boot_svc_min_sec_ver_test.c b/sw/device/silicon_creator/rom_ext/e2e/boot_svc/boot_svc_min_sec_ver_test.c index dfe79e77a0d5d..ff5fced071ee1 100644 --- a/sw/device/silicon_creator/rom_ext/e2e/boot_svc/boot_svc_min_sec_ver_test.c +++ b/sw/device/silicon_creator/rom_ext/e2e/boot_svc/boot_svc_min_sec_ver_test.c @@ -17,6 +17,7 @@ OTTF_DEFINE_TEST_CONFIG(); static status_t initialize(retention_sram_t *retram, boot_svc_retram_t *state) { boot_svc_msg_t msg = {0}; + LOG_INFO("Initialize: try to advance to min_sec_ver %d", 2); boot_svc_min_bl0_sec_ver_req_init(2, &msg.min_bl0_sec_ver_req); retram->creator.boot_svc_msg = msg; state->state = kBootSvcTestStateMinSecAdvance; @@ -36,6 +37,8 @@ static status_t advance(retention_sram_t *retram, boot_svc_retram_t *state) { if (msg.min_bl0_sec_ver_res.min_bl0_sec_ver < MANIFEST_SEC_VER) { // Advance by one and check again for success + LOG_INFO("Advance: try to advance to min_sec_ver %d", + msg.min_bl0_sec_ver_res.min_bl0_sec_ver + 1); boot_svc_min_bl0_sec_ver_req_init( msg.min_bl0_sec_ver_res.min_bl0_sec_ver + 1, &msg.min_bl0_sec_ver_req); retram->creator.boot_svc_msg = msg; @@ -44,6 +47,8 @@ static status_t advance(retention_sram_t *retram, boot_svc_retram_t *state) { if (msg.min_bl0_sec_ver_res.min_bl0_sec_ver == MANIFEST_SEC_VER) { // Advance by one and check for failure + LOG_INFO("Too Far: try to advance to min_sec_ver %d", + msg.min_bl0_sec_ver_res.min_bl0_sec_ver + 1); state->state = kBootSvcTestStateMinSecTooFar; boot_svc_min_bl0_sec_ver_req_init( msg.min_bl0_sec_ver_res.min_bl0_sec_ver + 1, &msg.min_bl0_sec_ver_req); @@ -64,6 +69,8 @@ static status_t too_far(retention_sram_t *retram, boot_svc_retram_t *state) { TRY_CHECK(msg.min_bl0_sec_ver_res.min_bl0_sec_ver == MANIFEST_SEC_VER); // Try to go back + LOG_INFO("Go Back: try to advance to min_sec_ver %d", + msg.min_bl0_sec_ver_res.min_bl0_sec_ver - 1); state->state = kBootSvcTestStateMinSecGoBack; boot_svc_min_bl0_sec_ver_req_init(msg.min_bl0_sec_ver_res.min_bl0_sec_ver - 1, &msg.min_bl0_sec_ver_req);