[rom_ext] Add Bound Check to Next Minimum Security Version

sw/device/silicon_creator/rom_ext/e2e/boot_svc/BUILD

@@ -221,16 +221,23 @@ 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 = [
         "//hw/top_earlgrey:fpga_hyper310_rom_ext",
         "//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",

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);

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;
 

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;