From 0c2d6ba4b8a82ba11acc8d3dd4809f95399735b0 Mon Sep 17 00:00:00 2001 From: Chris Frantz Date: Wed, 29 Jan 2025 14:26:59 -0800 Subject: [PATCH] [rom_ext, ownership] Fix the flash region configuration I did not write correct test cases that would check a flash configuration similar to the owner config already deployed in some skus. This oversight could cause ownership initialization to fail on devies with such a configuration. 1. Permit a maximum of 3 regions per side in the flash configuration. The regions must be fully within the bounds of SlotA or SlotB and may not overlap the ROM_EXT region. 2. Apply the region configuration in order. Previously, there was a correspondence between the index of the region in the owner config and the MP_REGION register that it would land in, but this makes ownership transfers prone to configuration clashes. 3. Flash configuration is done in two passes to configure each side independently (the reason for this is to allow next_owner's flash config to apply to the non-primary side during ownership transfer). The flash_apply function now takex an index parameter to manage the desination MP_REGION register between passes. 4. Create a unittest case with a flash configuration similar to the already-deployed configuration. Include the ROM_EXT, application, filesystem and reserved segments. 5. Update the existing test cases to accomodate the new configuration scheme (e.g. applying in order rather than by index). Signed-off-by: Chris Frantz --- sw/device/silicon_creator/lib/error.h | 1 + .../silicon_creator/lib/ownership/datatypes.h | 5 + .../lib/ownership/owner_block.c | 117 +++-- .../lib/ownership/owner_block.h | 5 +- .../lib/ownership/owner_block_unittest.cc | 497 ++++++++++++++++-- .../silicon_creator/lib/ownership/ownership.c | 39 +- .../tests/ownership/flash_permission_test.rs | 58 +- 7 files changed, 612 insertions(+), 110 deletions(-) diff --git a/sw/device/silicon_creator/lib/error.h b/sw/device/silicon_creator/lib/error.h index a4e9e993c4676f..927f59acaecdd6 100644 --- a/sw/device/silicon_creator/lib/error.h +++ b/sw/device/silicon_creator/lib/error.h @@ -223,6 +223,7 @@ enum module_ { X(kErrorOwnershipFlashConfigRomExt, ERROR_(15, kModuleOwnership, kInvalidArgument)), \ X(kErrorOwnershipFlashConfigBounds, ERROR_(16, kModuleOwnership, kInvalidArgument)), \ X(kErrorOwnershipInvalidAlgorithm, ERROR_(17, kModuleOwnership, kInvalidArgument)), \ + X(kErrorOwnershipFlashConfigSlots, ERROR_(18, kModuleOwnership, kInvalidArgument)), \ /* Group all of the tag version error codes together */ \ X(kErrorOwnershipOWNRVersion, ERROR_(0x70, kModuleOwnership, kInvalidArgument)), \ X(kErrorOwnershipAPPKVersion, ERROR_(0x71, kModuleOwnership, kInvalidArgument)), \ diff --git a/sw/device/silicon_creator/lib/ownership/datatypes.h b/sw/device/silicon_creator/lib/ownership/datatypes.h index bb9a6d973a9a06..e38e5bfb9d4d4d 100644 --- a/sw/device/silicon_creator/lib/ownership/datatypes.h +++ b/sw/device/silicon_creator/lib/ownership/datatypes.h @@ -295,6 +295,11 @@ enum { #define FLASH_CONFIG_HIGH_ENDURANCE ((bitfield_field32_t) { .mask = 0xF, .index = 8 }) // clang-format on +/** + * The maximum number of owner_flash_region_t allower per slot. + */ +#define FLASH_CONFIG_REGIONS_PER_SLOT 3 + /** * The owner flash region describes a region of flash and its configuration * properties (ie: ECC, Scrambling, High Endurance, etc). diff --git a/sw/device/silicon_creator/lib/ownership/owner_block.c b/sw/device/silicon_creator/lib/ownership/owner_block.c index d46645d8476a75..2ad35ec2af9ef1 100644 --- a/sw/device/silicon_creator/lib/ownership/owner_block.c +++ b/sw/device/silicon_creator/lib/ownership/owner_block.c @@ -27,6 +27,11 @@ enum { kFlashPageSize = FLASH_CTRL_PARAM_BYTES_PER_PAGE, kFlashTotalSize = 2 * kFlashBankSize, + kFlashSlotAStart = 0, + kFlashSlotAEnd = kFlashSlotAStart + kFlashBankSize, + kFlashSlotBStart = kFlashBankSize, + kFlashSlotBEnd = kFlashSlotBStart + kFlashBankSize, + kRomExtSizeInPages = CHIP_ROM_EXT_SIZE_MAX / kFlashPageSize, kRomExtAStart = 0 / kFlashPageSize, kRomExtAEnd = kRomExtAStart + kRomExtSizeInPages, @@ -198,6 +203,15 @@ hardened_bool_t rom_ext_flash_exclusive(uint32_t start, uint32_t end) { : kHardenedBoolFalse; } +static hardened_bool_t in_flash_slot(uint32_t bank_start, uint32_t start, + uint32_t end) { + uint32_t bank_end = bank_start + kFlashBankSize; + return (bank_start <= start && start < bank_end && bank_start < end && + end <= bank_end) + ? kHardenedBoolTrue + : kHardenedBoolFalse; +} + rom_error_t owner_block_flash_check(const owner_flash_config_t *flash) { size_t len = (flash->header.length - sizeof(owner_flash_config_t)) / sizeof(owner_flash_region_t); @@ -205,19 +219,34 @@ rom_error_t owner_block_flash_check(const owner_flash_config_t *flash) { return kErrorOwnershipFlashConfigLength; } + uint32_t num_slot_a = 0; + uint32_t num_slot_b = 0; const owner_flash_region_t *config = flash->config; uint32_t crypt = 0; for (size_t i = 0; i < len; ++i, ++config, crypt += 0x11111111) { uint32_t start = config->start; uint32_t end = start + config->size; - if (end > kFlashTotalSize) { - return kErrorOwnershipFlashConfigBounds; - } // When checking the flash configuration, a region is a ROM_EXT region if // it overlaps the ROM_EXT bounds. It is an error to accept a new config // with a flash region that overlaps the ROM_EXT. if (rom_ext_flash_overlap(start, end) == kHardenedBoolTrue) { return kErrorOwnershipFlashConfigRomExt; + } else if (in_flash_slot(kFlashSlotAStart, start, end) == + kHardenedBoolTrue) { + num_slot_a += 1; + if (num_slot_a > FLASH_CONFIG_REGIONS_PER_SLOT) { + return kErrorOwnershipFlashConfigSlots; + } + } else if (in_flash_slot(kFlashSlotBStart, start, end) == + kHardenedBoolTrue) { + num_slot_b += 1; + if (num_slot_b > FLASH_CONFIG_REGIONS_PER_SLOT) { + return kErrorOwnershipFlashConfigSlots; + } + } else { + // Flash regions are not allowed to span between slots or extend beyond + // the end of flash. + return kErrorOwnershipFlashConfigBounds; } } return kErrorOk; @@ -225,7 +254,8 @@ rom_error_t owner_block_flash_check(const owner_flash_config_t *flash) { rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash, uint32_t config_side, - uint32_t owner_lockdown) { + uint32_t owner_lockdown, + uint32_t *mp_index) { if ((hardened_bool_t)flash == kHardenedBoolFalse) return kErrorOk; @@ -239,13 +269,31 @@ rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash, : 0; size_t len = (flash->header.length - sizeof(owner_flash_config_t)) / sizeof(owner_flash_region_t); - if (len > kProtectSlots - kRomExtRegions) { + if (len > kProtectSlots) { + // We really want `kProtectSlots - kRomExtRegions`, but we have to + // accomodate configurations that may have specified ROM_EXT protection + // regions. We deal with that fact in the loop below. return kErrorOwnershipFlashConfigLength; } const owner_flash_region_t *config = flash->config; uint32_t crypt = 0; for (size_t i = 0; i < len; ++i, ++config, crypt += 0x11111111) { + // When applying the flash configuration, a region is a ROM_EXT region if + // it is exclusively within the ROM_EXT bounds. This sets the creator + // the lockdown policy for a region that is exclusively the creator + // region. + uint32_t region_start = config->start; + uint32_t region_end = region_start + config->size; + if (rom_ext_flash_exclusive(region_start, region_end) == + kHardenedBoolTrue) { + // Flash region is a ROM_EXT region. Do nothing. + // Some early configurations explicitly set parameters for the ROM_EXT + // region. We ignore these regions in favor of the ROM_EXT setting + // its own protection parameters. + continue; + } + if (config->start >= start && config->start + config->size <= end) { uint32_t val = config->properties ^ crypt; flash_ctrl_cfg_t cfg = { @@ -266,40 +314,33 @@ rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash, bitfield_field32_read(val, FLASH_CONFIG_LOCK) != kMultiBitBool4False ? kHardenedBoolTrue : kHardenedBoolFalse; - uint32_t region_start = config->start; - uint32_t region_end = region_start + config->size; - - // When applying the flash configuration, a region is a ROM_EXT region if - // it is exclusively within the ROM_EXT bounds. This sets the creator - // the lockdown policy for a region that is exclusively the creator - // region. - if (rom_ext_flash_exclusive(region_start, region_end) == - kHardenedBoolTrue) { - // Flash region is a ROM_EXT region. Do nothing. - // Some early configurations explicitly set parameters for the ROM_EXT - // region. We ignore these regions in favor of the ROM_EXT setting - // its own protection parameters. - continue; - } else { - // Flash region is an owner region. - // If the config_side is the same as the owner lockdown side, and - // protect_when_primary is requested, deny write/erase to the region. - if (config_side == owner_lockdown && pwp != kMultiBitBool4False) { - perm.write = kMultiBitBool4False; - perm.erase = kMultiBitBool4False; - } - // If we aren't in a lockdown state, then do not lock the region - // configuration via the flash_ctrl regwen bits. - if (owner_lockdown == kHardenedBoolFalse) { - lock = kHardenedBoolFalse; - } + + // Flash region is an owner region. + // If the config_side is the same as the owner lockdown side, and + // protect_when_primary is requested, deny write/erase to the region. + if (config_side == owner_lockdown && pwp != kMultiBitBool4False) { + perm.write = kMultiBitBool4False; + perm.erase = kMultiBitBool4False; + } + + // If we aren't in a lockdown state, then do not lock the region + // configuration via the flash_ctrl regwen bits. + if (owner_lockdown == kHardenedBoolFalse) { + lock = kHardenedBoolFalse; + } + + if (*mp_index < 2 * FLASH_CONFIG_REGIONS_PER_SLOT) { + // We can only apply the region protection of mp_index is + // within its acceptable bounds. + flash_ctrl_data_region_protect(kRomExtRegions + *mp_index, + config->start, config->size, perm, cfg, + lock); + SEC_MMIO_WRITE_INCREMENT(kFlashCtrlSecMmioDataRegionProtect + lock == + kHardenedBoolTrue + ? kFlashCtrlSecMmioDataRegionProtectLock + : 0); } - flash_ctrl_data_region_protect(kRomExtRegions + i, config->start, - config->size, perm, cfg, lock); - SEC_MMIO_WRITE_INCREMENT(kFlashCtrlSecMmioDataRegionProtect + lock == - kHardenedBoolTrue - ? kFlashCtrlSecMmioDataRegionProtectLock - : 0); + *mp_index += 1; } } return kErrorOk; diff --git a/sw/device/silicon_creator/lib/ownership/owner_block.h b/sw/device/silicon_creator/lib/ownership/owner_block.h index fc17d371922241..35c4acd8da5365 100644 --- a/sw/device/silicon_creator/lib/ownership/owner_block.h +++ b/sw/device/silicon_creator/lib/ownership/owner_block.h @@ -118,11 +118,14 @@ rom_error_t owner_block_flash_check(const owner_flash_config_t *flash); * @param owner_lockdown Apply any special lockdown configuration to * silicon_owner regions on the specified side of the * flash. May use kHardenedBoolFalse to skip lockdown. + * @param mp_index The destination configuration index. The value should be + * initialized to zero before the first call to this function. * @return error code. */ rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash, uint32_t config_side, - uint32_t owner_lockdown); + uint32_t owner_lockdown, + uint32_t *mp_index); /** * Apply the flash info configuration parameters from the owner block. diff --git a/sw/device/silicon_creator/lib/ownership/owner_block_unittest.cc b/sw/device/silicon_creator/lib/ownership/owner_block_unittest.cc index 2faf23519ff852..ef0b1f0b27a789 100644 --- a/sw/device/silicon_creator/lib/ownership/owner_block_unittest.cc +++ b/sw/device/silicon_creator/lib/ownership/owner_block_unittest.cc @@ -5,6 +5,7 @@ #include "sw/device/silicon_creator/lib/ownership/owner_block.h" #include +#include #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -88,7 +89,6 @@ const owner_flash_config_t simple_flash_config = { /*scramble=*/true, /*ecc=*/true, /*he=*/false), - }, { // SideA Filesystem. @@ -106,7 +106,6 @@ const owner_flash_config_t simple_flash_config = { /*scramble=*/false, /*ecc=*/false, /*he=*/true), - }, { // SideB FIRMWARE. @@ -145,15 +144,6 @@ const owner_flash_config_t simple_flash_config = { }, }; -const owner_flash_config_t bad_flash_config = { - .header = - { - .tag = kTlvTagFlashConfig, - .length = - sizeof(owner_flash_config_t) + 8 * sizeof(owner_flash_region_t), - }, -}; - const owner_flash_config_t flash_config_contains_rom_ext = { .header = { @@ -249,27 +239,22 @@ const owner_flash_info_config_t info_config = { }, }; -TEST_F(OwnerBlockTest, FlashConfigApplyBad) { - rom_error_t error = owner_block_flash_apply(&bad_flash_config, kBootSlotA, - /*owner_lockdown=*/0); - EXPECT_EQ(error, kErrorOwnershipFlashConfigLength); -} - TEST_F(OwnerBlockTest, FlashConfigApplyBadRomExt) { // There are no expectations because `owner_block_flash_apply` will ignore // regions that match the ROM_EXT. This is intentional so pre-existing owner // configs that cover the ROM_EXT won't result in an error and boot-loop the // chip. + uint32_t mp_index = 0; rom_error_t error = owner_block_flash_apply(&flash_config_contains_rom_ext, kBootSlotA, - /*owner_lockdown=*/0); + /*owner_lockdown=*/0, &mp_index); EXPECT_EQ(error, kErrorOk); } // Tests that the flash parameters get applied for side A. TEST_F(OwnerBlockTest, FlashConfigApplySideA) { // The ROM_EXT always uses regions 0-1 to protect itself, The items in - // the flash config always get programmed into `index + 2`. + // the flash config always get programmed in order starting at index 2. EXPECT_CALL( flash_ctrl_, DataRegionProtect( @@ -287,8 +272,9 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideA) { kMultiBitBool4True), kHardenedBoolFalse)); + uint32_t mp_index = 0; rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotA, - /*owner_lockdown=*/0); + /*owner_lockdown=*/0, &mp_index); EXPECT_EQ(error, kErrorOk); } @@ -297,7 +283,7 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideA) { // regions. TEST_F(OwnerBlockTest, FlashConfigApplySideA_Active) { // The ROM_EXT always uses regions 0-1 to protect itself, The items in - // the flash config always get programmed into `index + 2`. + // the flash config always get programmed in order starting at index 2. EXPECT_CALL( flash_ctrl_, DataRegionProtect( @@ -315,8 +301,10 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideA_Active) { kMultiBitBool4True), kHardenedBoolFalse)); - rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotA, - /*owner_lockdown=*/kBootSlotA); + uint32_t mp_index = 0; + rom_error_t error = + owner_block_flash_apply(&simple_flash_config, kBootSlotA, + /*owner_lockdown=*/kBootSlotA, &mp_index); EXPECT_EQ(error, kErrorOk); } @@ -325,26 +313,258 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideA_Active) { // permissions for slot B. TEST_F(OwnerBlockTest, FlashConfigApplySideB_NotActive) { // The ROM_EXT always uses regions 0-1 to protect itself, The items in - // the flash config always get programmed into `index + 2`. + // the flash config always get programmed in order starting at index 2. EXPECT_CALL( flash_ctrl_, DataRegionProtect( - 4, 256 + 32, 192, + 2, 256 + 32, 192, FlashPerms(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4True), FlashCfg(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4False), kHardenedBoolFalse)); EXPECT_CALL( flash_ctrl_, - DataRegionProtect(5, 256 + 224, 32, + DataRegionProtect(3, 256 + 224, 32, FlashPerms(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4True), FlashCfg(kMultiBitBool4False, kMultiBitBool4False, kMultiBitBool4True), kHardenedBoolFalse)); - rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotB, - /*owner_lockdown=*/kBootSlotA); + uint32_t mp_index = 0; + rom_error_t error = + owner_block_flash_apply(&simple_flash_config, kBootSlotB, + /*owner_lockdown=*/kBootSlotA, &mp_index); + EXPECT_EQ(error, kErrorOk); +} + +const owner_flash_config_t legacy_flash_config = { + .header = + { + .tag = kTlvTagFlashConfig, + .length = + sizeof(owner_flash_config_t) + 8 * sizeof(owner_flash_region_t), + }, + .config = + { + { + // SideA ROM_EXT. This configuration should be skipped + // by flash_ apply. + .start = 0, + .size = 32, + .access = FLASH_ACCESS( + /*index=*/0, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/0, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + + }, + { + // SideA FIRMWARE. + .start = 32, + .size = 192, + .access = FLASH_ACCESS( + /*index=*/1, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/1, + /*scramble=*/true, + /*ecc=*/true, + /*he=*/false), + }, + { + // SideA Filesystem. + .start = 224, + .size = 31, + .access = FLASH_ACCESS( + /*index=*/2, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/false, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/2, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/true), + }, + { + // SideA ROM_EXT. This configuration should be skipped + // by flash_ apply. + .start = 256 + 0, + .size = 32, + .access = FLASH_ACCESS( + /*index=*/3, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/3, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideB FIRMWARE. + .start = 256 + 32, + .size = 192, + .access = FLASH_ACCESS( + /*index=*/4, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/4, + /*scramble=*/true, + /*ecc=*/true, + /*he=*/false), + }, + { + // SideB Filesystem. + .start = 256 + 224, + .size = 31, + .access = FLASH_ACCESS( + /*index=*/5, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/false, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/5, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/true), + }, + { + // SideA Reserved last page. + .start = 255, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/6, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/false, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/6, + /*scramble=*/true, + /*ecc=*/true, + /*he=*/false), + }, + { + // SideB Reserved last page. + .start = 511, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/7, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/false, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/7, + /*scramble=*/true, + /*ecc=*/true, + /*he=*/false), + }, + + }, +}; + +// Tests that the flash parameters get applied for side B when B is not the +// active slot. Check that ProtectWhenActive does not change the write/erase +// permissions for slot B. +TEST_F(OwnerBlockTest, FlashConfigApplyLegacy) { + // The ROM_EXT always uses regions 0-1 to protect itself, The items in + // the flash config always get programmed in order starting at index 2. + + // We should not see the ROM_EXT regions from the `legacy_flash_config`. + // We should see the rest of the regions in order. + + // The SideA Firmware region has "protect when active" set, so the erase + // and program permissions should be false. + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect( + 2, 32, 192, + FlashPerms(kMultiBitBool4True, kMultiBitBool4False, + kMultiBitBool4False), + FlashCfg(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4False), + kHardenedBoolFalse)); + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect(3, 224, 31, + FlashPerms(kMultiBitBool4True, kMultiBitBool4True, + kMultiBitBool4True), + FlashCfg(kMultiBitBool4False, kMultiBitBool4False, + kMultiBitBool4True), + kHardenedBoolFalse)); + // The SideA reserved last page is out-of-order with the rest of the SideA + // entries. This is ok, and it should land in the next mp_region register. + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect( + 4, 255, 1, + FlashPerms(kMultiBitBool4True, kMultiBitBool4True, + kMultiBitBool4True), + FlashCfg(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4False), + kHardenedBoolFalse)); + + // The SideB Firmware region has "protect when active" set, but it isn't + // the active side, so the erase and program permissions should be true. + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect( + 5, 256 + 32, 192, + FlashPerms(kMultiBitBool4True, kMultiBitBool4True, + kMultiBitBool4True), + FlashCfg(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4False), + kHardenedBoolFalse)); + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect(6, 256 + 224, 31, + FlashPerms(kMultiBitBool4True, kMultiBitBool4True, + kMultiBitBool4True), + FlashCfg(kMultiBitBool4False, kMultiBitBool4False, + kMultiBitBool4True), + kHardenedBoolFalse)); + // The SideB reserved last page is out-of-order with the rest of the SideB + // entries. This is ok, and it should land in the next mp_region register. + EXPECT_CALL( + flash_ctrl_, + DataRegionProtect( + 7, 511, 1, + FlashPerms(kMultiBitBool4True, kMultiBitBool4True, + kMultiBitBool4True), + FlashCfg(kMultiBitBool4True, kMultiBitBool4True, kMultiBitBool4False), + kHardenedBoolFalse)); + + uint32_t mp_index = 0; + rom_error_t error = + owner_block_flash_apply(&legacy_flash_config, kBootSlotA, + /*owner_lockdown=*/kBootSlotA, &mp_index); + EXPECT_EQ(error, kErrorOk); + error = owner_block_flash_apply(&legacy_flash_config, kBootSlotB, + /*owner_lockdown=*/kBootSlotA, &mp_index); EXPECT_EQ(error, kErrorOk); } @@ -676,22 +896,231 @@ const owner_flash_config_t invalid_flash_3 = { }; +// Flash configuration has too many entries. +// We don't have to include the entries because the length is checked first +// and none of the non-existent entries will be accessed. +const owner_flash_config_t invalid_flash_4 = { + .header = + { + .tag = kTlvTagFlashConfig, + .length = + sizeof(owner_flash_config_t) + 8 * sizeof(owner_flash_region_t), + }, +}; + +// Flash configuration extends beyond end of flash +const owner_flash_config_t invalid_flash_5 = { + .header = + { + .tag = kTlvTagFlashConfig, + .length = + sizeof(owner_flash_config_t) + 1 * sizeof(owner_flash_region_t), + }, + .config = + { + { + // SideB APP + .start = 256 + 32, + .size = 256, + .access = FLASH_ACCESS( + /*index=*/0, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/0, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + }, +}; + +// Flash configuration has too many entries for Slot A. +const owner_flash_config_t invalid_flash_6 = { + .header = + { + .tag = kTlvTagFlashConfig, + .length = + sizeof(owner_flash_config_t) + 4 * sizeof(owner_flash_region_t), + }, + .config = {{ + // SideA APP + .start = 32, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/0, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/0, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideA APP + .start = 33, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/1, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/1, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideA APP + .start = 34, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/2, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/2, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideA APP + .start = 35, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/3, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/3, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }}, +}; + +// Flash configuration has too many entries for Slot B. +const owner_flash_config_t invalid_flash_7 = { + .header = + { + .tag = kTlvTagFlashConfig, + .length = + sizeof(owner_flash_config_t) + 4 * sizeof(owner_flash_region_t), + }, + .config = {{ + // SideB APP + .start = 256 + 32, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/0, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/0, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideB APP + .start = 256 + 33, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/1, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/1, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideB APP + .start = 256 + 34, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/2, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/2, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }, + { + // SideB APP + .start = 256 + 35, + .size = 1, + .access = FLASH_ACCESS( + /*index=*/3, + /*read=*/true, + /*program=*/true, + /*erase=*/true, + /*pwp=*/true, + /*lock=*/false), + .properties = FLASH_PROP( + /*index=*/3, + /*scramble=*/false, + /*ecc=*/false, + /*he=*/false), + }}, +}; + class RomExtFlashConfigTest : public OwnerBlockTest, - public testing::WithParamInterface {}; + public testing::WithParamInterface< + std::tuple> {}; // Test bad ROM_EXT region configs with respect to the default config. TEST_P(RomExtFlashConfigTest, BadFlashConfig) { EXPECT_CALL(flash_ctrl_, DataDefaultCfgGet) .WillRepeatedly(Return(default_config)); - const owner_flash_config_t *param = GetParam(); + const owner_flash_config_t *param; + rom_error_t expected; + std::tie(param, expected) = GetParam(); rom_error_t error = owner_block_flash_check(param); - EXPECT_EQ(error, kErrorOwnershipFlashConfigRomExt); + EXPECT_EQ(error, expected); } -INSTANTIATE_TEST_SUITE_P(AllCases, RomExtFlashConfigTest, - testing::Values(&invalid_flash_0, &invalid_flash_1, - &invalid_flash_2, &invalid_flash_3)); +INSTANTIATE_TEST_SUITE_P( + AllCases, RomExtFlashConfigTest, + testing::Values( + std::make_tuple(&invalid_flash_0, kErrorOwnershipFlashConfigRomExt), + std::make_tuple(&invalid_flash_1, kErrorOwnershipFlashConfigRomExt), + std::make_tuple(&invalid_flash_2, kErrorOwnershipFlashConfigRomExt), + std::make_tuple(&invalid_flash_3, kErrorOwnershipFlashConfigRomExt), + std::make_tuple(&invalid_flash_4, kErrorOwnershipFlashConfigLength), + std::make_tuple(&invalid_flash_5, kErrorOwnershipFlashConfigBounds), + std::make_tuple(&invalid_flash_6, kErrorOwnershipFlashConfigSlots), + std::make_tuple(&invalid_flash_7, kErrorOwnershipFlashConfigSlots))); struct FlashRegion { uint32_t start; diff --git a/sw/device/silicon_creator/lib/ownership/ownership.c b/sw/device/silicon_creator/lib/ownership/ownership.c index 40a50a883aeb6c..55909a37ac8681 100644 --- a/sw/device/silicon_creator/lib/ownership/ownership.c +++ b/sw/device/silicon_creator/lib/ownership/ownership.c @@ -125,12 +125,13 @@ static rom_error_t locked_owner_init(boot_data_t *bootdata, } HARDENED_RETURN_IF_ERROR(owner_block_parse( &owner_page[0], /*check_only=*/kHardenedBoolFalse, config, keyring)); - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, kBootSlotA, - /*owner_lockdown=*/kHardenedBoolFalse)); - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, kBootSlotB, - /*owner_lockdown=*/kHardenedBoolFalse)); + uint32_t mp_index = 0; + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, kBootSlotA, + /*owner_lockdown=*/kHardenedBoolFalse, &mp_index)); + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, kBootSlotB, + /*owner_lockdown=*/kHardenedBoolFalse, &mp_index)); HARDENED_RETURN_IF_ERROR(owner_block_info_apply(config->info)); return kErrorOk; } @@ -149,13 +150,14 @@ static rom_error_t unlocked_init(boot_data_t *bootdata, owner_config_t *config, return kErrorOwnershipBadInfoPage; } + uint32_t mp_index = 0; if (owner_page_valid[0] == kOwnerPageStatusSealed) { // Configure the primary half of the flash as Owner Page 0 requests. HARDENED_RETURN_IF_ERROR(owner_block_parse( &owner_page[0], /*check_only=*/kHardenedBoolFalse, config, keyring)); - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, bootdata->primary_bl0_slot, - /*owner_lockdown=*/kHardenedBoolFalse)); + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, bootdata->primary_bl0_slot, + /*owner_lockdown=*/kHardenedBoolFalse, &mp_index)); } if (owner_block_page1_valid_for_transfer(bootdata) == kHardenedBoolTrue) { @@ -170,9 +172,9 @@ static rom_error_t unlocked_init(boot_data_t *bootdata, owner_config_t *config, dbg_printf("error: owner page 1 invalid.\r\n"); } } - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, secondary, - /*owner_lockdown=*/kHardenedBoolFalse)); + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, secondary, + /*owner_lockdown=*/kHardenedBoolFalse, &mp_index)); HARDENED_RETURN_IF_ERROR(owner_block_info_apply(config->info)); return kErrorOk; } @@ -275,12 +277,13 @@ rom_error_t ownership_init(boot_data_t *bootdata, owner_config_t *config, rom_error_t ownership_flash_lockdown(boot_data_t *bootdata, boot_log_t *bootlog, const owner_config_t *config) { if (bootdata->ownership_state == kOwnershipStateLockedOwner) { - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, kBootSlotA, - /*owner_lockdown=*/bootlog->bl0_slot)); - HARDENED_RETURN_IF_ERROR( - owner_block_flash_apply(config->flash, kBootSlotB, - /*owner_lockdown=*/bootlog->bl0_slot)); + uint32_t mp_index = 0; + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, kBootSlotA, + /*owner_lockdown=*/bootlog->bl0_slot, &mp_index)); + HARDENED_RETURN_IF_ERROR(owner_block_flash_apply( + config->flash, kBootSlotB, + /*owner_lockdown=*/bootlog->bl0_slot, &mp_index)); } else { HARDENED_CHECK_NE(bootdata->ownership_state, kOwnershipStateLockedOwner); } diff --git a/sw/host/tests/ownership/flash_permission_test.rs b/sw/host/tests/ownership/flash_permission_test.rs index ee3c64ae170877..b22854a938916c 100644 --- a/sw/host/tests/ownership/flash_permission_test.rs +++ b/sw/host/tests/ownership/flash_permission_test.rs @@ -230,22 +230,28 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<() region[1], FlashRegion("data", 1, 256, 32, romext_region[1], "LK") ); + + // Current owner (fake_owner in flash SideA) doesn't have a configuration, + // so no MP_REGION registers are programmed. + + // Next owner (dummy_owner in Flash SideB) is the next owner configuration. assert_eq!( region[2], - FlashRegion("data", 2, 0, 0, "xx-xx-xx-xx-xx-xx", "UN") + FlashRegion("data", 2, 288, 192, "RD-WR-ER-SC-EC-xx", "UN") ); assert_eq!( region[3], - FlashRegion("data", 3, 0, 0, "xx-xx-xx-xx-xx-xx", "UN") + FlashRegion("data", 3, 480, 32, "RD-WR-ER-xx-xx-HE", "UN") ); - // Flash SideB is the next owner configuration. + + // The remaining flash regions are unused. assert_eq!( region[4], - FlashRegion("data", 4, 288, 192, "RD-WR-ER-SC-EC-xx", "UN") + FlashRegion("data", 4, 0, 0, "xx-xx-xx-xx-xx-xx", "UN") ); assert_eq!( region[5], - FlashRegion("data", 5, 480, 32, "RD-WR-ER-xx-xx-HE", "UN") + FlashRegion("data", 5, 0, 0, "xx-xx-xx-xx-xx-xx", "UN") ); assert_eq!( region[6], @@ -297,14 +303,6 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<() } let region = FlashRegion::find_all(&capture[1])?; - // The rescue_slot should be the active side and has protect_when_active = true. - let app_region = match opts.rescue_slot { - BootSlot::SlotA => ["RD-xx-xx-SC-EC-xx", "RD-WR-ER-SC-EC-xx"], - BootSlot::SlotB => ["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 // owner's flash configuration. @@ -313,6 +311,26 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<() } else { "UN" }; + + // The rescue_slot should be the active side and has protect_when_active = true. + let app_region = match opts.rescue_slot { + BootSlot::SlotA => [ + // Slot A protected, Slot B writable. + FlashRegion("data", 2, 32, 192, "RD-xx-xx-SC-EC-xx", locked), + FlashRegion("data", 3, 224, 32, "RD-WR-ER-xx-xx-HE", locked), + FlashRegion("data", 4, 288, 192,"RD-WR-ER-SC-EC-xx", locked), + FlashRegion("data", 5, 480, 32, "RD-WR-ER-xx-xx-HE", locked), + ], + BootSlot::SlotB => [ + // Slot A writable, Slot B protected. + FlashRegion("data", 2, 32, 192, "RD-WR-ER-SC-EC-xx", locked), + FlashRegion("data", 3, 224, 32, "RD-WR-ER-xx-xx-HE", locked), + FlashRegion("data", 4, 288, 192,"RD-xx-xx-SC-EC-xx", locked), + FlashRegion("data", 5, 480, 32, "RD-WR-ER-xx-xx-HE", locked), + ], + _ => return Err(anyhow!("Unknown boot slot {}", data.bl0_slot)), + }; + // The ROM_EXT always protects itself in regions 0 and 1. assert_eq!( region[0], @@ -325,23 +343,25 @@ fn flash_permission_test(opts: &Opts, transport: &TransportWrapper) -> Result<() // Flash Slot A: assert_eq!( region[2], - FlashRegion("data", 2, 32, 192, app_region[0], locked) + app_region[0], ); assert_eq!( region[3], - FlashRegion("data", 3, 224, 32, "RD-WR-ER-xx-xx-HE", locked) + app_region[1], + ); // Flash Slot B: assert_eq!( region[4], - FlashRegion("data", 4, 288, 192, app_region[1], locked) + app_region[2], + ); assert_eq!( region[5], - FlashRegion("data", 5, 480, 32, "RD-WR-ER-xx-xx-HE", locked) + app_region[3], + ); - // Regions 6 and 7 aren't specified in the owner config and therefore - // should be left unlocked. + // The last two regions are unused, and therefore, should be left unlocked. assert_eq!( region[6], FlashRegion("data", 6, 0, 0, "xx-xx-xx-xx-xx-xx", "UN")