Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rom_ext] Lock the correct ROM_EXT region #25892

Open
wants to merge 1 commit into
base: earlgrey_es_sival
Choose a base branch
from
Open

[rom_ext] Lock the correct ROM_EXT region #25892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions sw/device/silicon_creator/lib/error.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ enum module_ {
X(kErrorOwnershipInvalidDin, ERROR_(13, kModuleOwnership, kInvalidArgument)), \
X(kErrorOwnershipUnlockDenied, ERROR_(14, kModuleOwnership, kPermissionDenied)), \
X(kErrorOwnershipFlashConfigRomExt, ERROR_(15, kModuleOwnership, kInvalidArgument)), \
X(kErrorOwnershipFlashConfigBounds, ERROR_(16, kModuleOwnership, kInvalidArgument)), \
/* Group all of the tag version error codes together */ \
X(kErrorOwnershipOWNRVersion, ERROR_(0x70, kModuleOwnership, kInvalidArgument)), \
X(kErrorOwnershipAPPKVersion, ERROR_(0x71, kModuleOwnership, kInvalidArgument)), \
Expand Down
1 change: 1 addition & 0 deletions sw/device/silicon_creator/lib/ownership/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ cc_library(
":ownership_key",
"//sw/device/lib/base:hardened_memory",
"//sw/device/silicon_creator/lib:boot_data",
"//sw/device/silicon_creator/lib:boot_log",
"//sw/device/silicon_creator/lib:dbg_print",
"//sw/device/silicon_creator/lib/drivers:flash_ctrl",
"//sw/device/silicon_creator/lib/drivers:lifecycle",
Expand Down
98 changes: 78 additions & 20 deletions sw/device/silicon_creator/lib/ownership/owner_block.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ owner_page_status_t owner_page_valid[2];
enum {
kFlashBankSize = FLASH_CTRL_PARAM_REG_PAGES_PER_BANK,
kFlashPageSize = FLASH_CTRL_PARAM_BYTES_PER_PAGE,
kFlashTotalSize = 2 * kFlashBankSize,

kRomExtAStart = 0 / kFlashPageSize,
kRomExtAEnd = CHIP_ROM_EXT_SIZE_MAX / kFlashPageSize,
kRomExtBStart = kFlashBankSize + kRomExtAStart,
kRomExtBEnd = kFlashBankSize + kRomExtAEnd,
};

hardened_bool_t owner_block_newversion_mode(void) {
Expand Down Expand Up @@ -150,27 +156,51 @@ rom_error_t owner_block_parse(const owner_block_t *block,
return kErrorOk;
}

// These functions aren't defined in owner_block.h because they aren't meant
// to be public APIs. They aren't static so we can access the symbols in the
// unit test program.

// Checks if the half-open range [start..end) overlaps with the ROM_EXT region.
// The RomExt_Start/End constants are also expressed as half-open ranges.
hardened_bool_t rom_ext_flash_overlap(uint32_t start, uint32_t end) {
return (start < kRomExtAEnd && end > kRomExtAStart) ||
(start < kRomExtBEnd && end > kRomExtBStart)
? kHardenedBoolTrue
: kHardenedBoolFalse;
}

// Checks if the half-open range [start..end) is exclusively within the ROM_EXT
// region. The RomExt_Start/End constants are also expressed as half-open
// ranges.
hardened_bool_t rom_ext_flash_exclusive(uint32_t start, uint32_t end) {
return (kRomExtAStart <= start && start < kRomExtAEnd &&
kRomExtAStart < end && end <= kRomExtAEnd) ||
(kRomExtBStart <= start && start < kRomExtBEnd &&
kRomExtBStart < end && end <= kRomExtBEnd)
? 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);
if (len >= 8) {
return kErrorOwnershipFlashConfigLenth;
}

const uint32_t kRomExtAStart = 0 / kFlashPageSize;
const uint32_t kRomExtAEnd = CHIP_ROM_EXT_SIZE_MAX / kFlashPageSize;
const uint32_t kRomExtBStart = kFlashBankSize + kRomExtAStart;
const uint32_t kRomExtBEnd = kFlashBankSize + kRomExtAEnd;

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 ((kRomExtAStart >= start && kRomExtAStart < end) ||
(kRomExtAEnd > start && kRomExtAEnd <= end) ||
(kRomExtBStart >= start && kRomExtBStart < end) ||
(kRomExtBEnd > start && kRomExtBEnd <= end)) {
if (end > kFlashTotalSize) {
return kErrorOwnershipFlashConfigBounds;
}
// When checking the flash configuration, a region is a ROM_EXT region if
// it overlaps the ROM_EXT bounds. This detects if the defined ROM_EXT
// region has the same ecc/scrambling parameters as the default flash
// configuration.
if (rom_ext_flash_overlap(start, end) == kHardenedBoolTrue) {
uint32_t val = config->properties ^ crypt;
flash_ctrl_cfg_t cfg = {
.scrambling = bitfield_field32_read(val, FLASH_CONFIG_SCRAMBLE),
Expand All @@ -195,7 +225,9 @@ 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 lockdown) {
uint32_t config_side,
uint32_t creator_lockdown,
uint32_t owner_lockdown) {
if ((hardened_bool_t)flash == kHardenedBoolFalse)
return kErrorOk;
// TODO: Hardening: lockdown should be one of kBootSlotA, kBootSlotB or
Expand Down Expand Up @@ -229,19 +261,45 @@ rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash,
.erase = bitfield_field32_read(val, FLASH_CONFIG_ERASE),
};

if (lockdown == config_side) {
if (bitfield_field32_read(val, FLASH_CONFIG_PROTECT_WHEN_PRIMARY) !=
kMultiBitBool4False) {
uint32_t pwp =
bitfield_field32_read(val, FLASH_CONFIG_PROTECT_WHEN_PRIMARY);
hardened_bool_t lock =
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the detailed comment! The comment and the logic don't quite match either other. I agree with the comment. The logic to match the comment would be

kRomExtAStart <= region_start < kRomExtAEnd &&
kRomExtAStart < region_end <= kRomExtAEnd

(and maybe we need an explicit region_start<=region_end check too?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 How embarrassing. A total logic fail.

I've fixed this and added unit tests to owner_block_unittest.cc to test the overlap/exclusivity checking functions.

We don't need to test region_start <= region_end because the bounds are computed from a start and size (which are uint16_t) and the region_* values are uint32_t.

I added a sanity check to the end value: if the end is beyond the total size of the flash, the configuration is invalid.

// 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.
// If the config_side is the same as the creator lockdown side, and
// protect_when_primary is requested, deny write/erase to the region.
if (config_side == creator_lockdown && pwp != kMultiBitBool4False) {
perm.write = kMultiBitBool4False;
perm.erase = kMultiBitBool4False;
}
}

hardened_bool_t lock = kHardenedBoolFalse;
if (lockdown != kHardenedBoolFalse) {
if (bitfield_field32_read(val, FLASH_CONFIG_LOCK) !=
kMultiBitBool4False) {
lock = kHardenedBoolTrue;
// If we aren't in a lockdown state, then do not lock the region
// configuration via the flash_ctrl regwen bits.
if (creator_lockdown == kHardenedBoolFalse) {
lock = kHardenedBoolFalse;
}
} 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_ctrl_data_region_protect(i, config->start, config->size, perm, cfg,
Expand Down
13 changes: 9 additions & 4 deletions sw/device/silicon_creator/lib/ownership/owner_block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,18 @@ rom_error_t owner_block_flash_check(const owner_flash_config_t *flash);
*
* @param flash A pointer to a flash configuration struct.
* @param config_side Which side of the flash to configure.
* @param lockdown Apply any special lockdown configuration to the specified
* side of the flash. May use kHardenedBoolFalse to skip
* lockdown.
* @param creator_lockdown Apply any special lockdown configuration to
* silicon_creator regions on the specified side of the
* flash. May use kHardenedBoolFalse to skip lockdown.
* @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.
* @return error code.
*/
rom_error_t owner_block_flash_apply(const owner_flash_config_t *flash,
uint32_t config_side, uint32_t lockdown);
uint32_t config_side,
uint32_t creator_lockdown,
uint32_t owner_lockdown);

/**
* Apply the flash info configuration parameters from the owner block.
Expand Down
78 changes: 67 additions & 11 deletions sw/device/silicon_creator/lib/ownership/owner_block_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,9 @@ const owner_flash_info_config_t info_config = {
};

TEST_F(OwnerBlockTest, FlashConfigApplyBad) {
rom_error_t error = owner_block_flash_apply(&bad_flash_config, kBootSlotA, 0);
rom_error_t error = owner_block_flash_apply(&bad_flash_config, kBootSlotA,
/*creator_lockdown=*/0,
/*owner_lockdown=*/0);
EXPECT_EQ(error, kErrorOwnershipFlashConfigLenth);
}

Expand Down Expand Up @@ -269,15 +271,16 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideA) {
kMultiBitBool4True),
kHardenedBoolFalse));

rom_error_t error =
owner_block_flash_apply(&simple_flash_config, kBootSlotA, 0);
rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotA,
/*creator_lockdown=*/0,
/*owner_lockdown=*/0);
EXPECT_EQ(error, kErrorOk);
}

// Tests that the flash parameters get applied for side A and the
// ProtectWhenPrimary disables erase and program on the ROM_EXT and FIRMWARE
// ProtectWhenActive disables erase and program on the ROM_EXT and FIRMWARE
// regions.
TEST_F(OwnerBlockTest, FlashConfigApplySideAPrimary) {
TEST_F(OwnerBlockTest, FlashConfigApplySideA_Active) {
EXPECT_CALL(
flash_ctrl_,
DataRegionProtect(0, 0, 32,
Expand All @@ -303,13 +306,16 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideAPrimary) {
kMultiBitBool4True),
kHardenedBoolFalse));

rom_error_t error =
owner_block_flash_apply(&simple_flash_config, kBootSlotA, kBootSlotA);
rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotA,
/*creator_lockdown=*/kBootSlotA,
/*owner_lockdown=*/kBootSlotA);
EXPECT_EQ(error, kErrorOk);
}

// Tests that the flash parameters get applied for side B.
TEST_F(OwnerBlockTest, FlashConfigApplySideB) {
// 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, FlashConfigApplySideB_NotActive) {
EXPECT_CALL(
flash_ctrl_,
DataRegionProtect(3, 256 + 0, 32,
Expand All @@ -335,8 +341,9 @@ TEST_F(OwnerBlockTest, FlashConfigApplySideB) {
kMultiBitBool4True),
kHardenedBoolFalse));

rom_error_t error =
owner_block_flash_apply(&simple_flash_config, kBootSlotB, 0);
rom_error_t error = owner_block_flash_apply(&simple_flash_config, kBootSlotB,
/*creator_lockdown=*/kBootSlotA,
/*owner_lockdown=*/kBootSlotA);
EXPECT_EQ(error, kErrorOk);
}

Expand Down Expand Up @@ -723,4 +730,53 @@ INSTANTIATE_TEST_SUITE_P(AllCases, RomExtFlashConfigTest,
&invalid_flash_2, &invalid_flash_3,
&invalid_flash_4));

struct FlashRegion {
uint32_t start;
uint32_t end;
bool overlap;
bool exclusive;
};

class RomExtFlashBoundsTest : public OwnerBlockTest,
public testing::WithParamInterface<FlashRegion> {
};

extern "C" {
// These functions aren't defined in owner_block.h because they aren't meant
// to be public APIs. They aren't static so we can access the symbols here
// for testing.
hardened_bool_t rom_ext_flash_overlap(uint32_t, uint32_t);
hardened_bool_t rom_ext_flash_exclusive(uint32_t, uint32_t);
}

// Test the flash bounds checking functions.
TEST_P(RomExtFlashBoundsTest, FlashBoundsTest) {
FlashRegion p = GetParam();
hardened_bool_t overlap = rom_ext_flash_overlap(p.start, p.end);
hardened_bool_t exclusive = rom_ext_flash_exclusive(p.start, p.end);
EXPECT_EQ(overlap, p.overlap ? kHardenedBoolTrue : kHardenedBoolFalse);
EXPECT_EQ(exclusive, p.exclusive ? kHardenedBoolTrue : kHardenedBoolFalse);
}

// clang-format off
INSTANTIATE_TEST_SUITE_P(AllCases, RomExtFlashBoundsTest,
testing::Values(
// The ROM_EXT A region is blocks [0x00 .. 0x20).
// The ROM_EXT B region is blocks [0x100 .. 0x120).
// The ranges expressed are half-open: [start .. end).
FlashRegion{0x000, 0x020, /*overlap=*/true, /*exclusive=*/true}, // Full ROM_EXT A region.
FlashRegion{0x100, 0x120, /*overlap=*/true, /*exclusive=*/true}, // Full ROM_EXT B region.
FlashRegion{0x005, 0x010, /*overlap=*/true, /*exclusive=*/true}, // Partial ROM_EXT A region.
FlashRegion{0x105, 0x110, /*overlap=*/true, /*exclusive=*/true}, // Partial ROM_EXT B region.
FlashRegion{0x000, 0x040, /*overlap=*/true, /*exclusive=*/false}, // Overlapped ROM_EXT A region.
FlashRegion{0x100, 0x140, /*overlap=*/true, /*exclusive=*/false}, // Overlapped ROM_EXT B region.
FlashRegion{0x020, 0x040, /*overlap=*/false, /*exclusive=*/false}, // Not ROM_EXT A region.
FlashRegion{0x120, 0x140, /*overlap=*/false, /*exclusive=*/false}, // Not ROM_EXT B region.
FlashRegion{0x005, 0x105, /*overlap=*/true, /*exclusive=*/false}, // Mid RX_A to mid RX_B.
FlashRegion{0x020, 0x100, /*overlap=*/false, /*exclusive=*/false}, // Full not ROM_EXT side A.
FlashRegion{0x020, 0x101, /*overlap=*/true, /*exclusive=*/false}, // Side A intrudes in to RX_B.
FlashRegion{0x000, 0x200, /*overlap=*/true, /*exclusive=*/false} // Full flash region.
));
// clang-format on

} // namespace
28 changes: 18 additions & 10 deletions sw/device/silicon_creator/lib/ownership/ownership.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include "sw/device/lib/base/macros.h"
#include "sw/device/lib/base/memory.h"
#include "sw/device/silicon_creator/lib/boot_data.h"
#include "sw/device/silicon_creator/lib/boot_log.h"
#include "sw/device/silicon_creator/lib/dbg_print.h"
#include "sw/device/silicon_creator/lib/drivers/flash_ctrl.h"
#include "sw/device/silicon_creator/lib/drivers/hmac.h"
Expand Down Expand Up @@ -125,10 +126,12 @@ static rom_error_t locked_owner_init(boot_data_t *bootdata,
HARDENED_RETURN_IF_ERROR(owner_block_parse(&owner_page[0], config, keyring));
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, kBootSlotA,
/*lockdown=*/kHardenedBoolFalse));
/*creator_lockdown=*/kHardenedBoolFalse,
/*owner_lockdown=*/kHardenedBoolFalse));
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, kBootSlotB,
/*lockdown=*/kHardenedBoolFalse));
/*creator_lockdown=*/kHardenedBoolFalse,
/*owner_lockdown=*/kHardenedBoolFalse));
HARDENED_RETURN_IF_ERROR(owner_block_info_apply(config->info));
return kErrorOk;
}
Expand All @@ -153,7 +156,8 @@ static rom_error_t unlocked_init(boot_data_t *bootdata, owner_config_t *config,
owner_block_parse(&owner_page[0], config, keyring));
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, bootdata->primary_bl0_slot,
/*lockdown=*/kHardenedBoolFalse));
/*creator_lockdown=*/kHardenedBoolFalse,
/*owner_lockdown=*/kHardenedBoolFalse));
}

if (owner_block_page1_valid_for_transfer(bootdata) == kHardenedBoolTrue) {
Expand All @@ -171,7 +175,8 @@ static rom_error_t unlocked_init(boot_data_t *bootdata, owner_config_t *config,
}
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, secondary,
/*lockdown=*/kHardenedBoolFalse));
/*creator_lockdown=*/kHardenedBoolFalse,
/*owner_lockdown=*/kHardenedBoolFalse));
HARDENED_RETURN_IF_ERROR(owner_block_info_apply(config->info));
return kErrorOk;
}
Expand Down Expand Up @@ -271,14 +276,17 @@ rom_error_t ownership_init(boot_data_t *bootdata, owner_config_t *config,
return error;
}

rom_error_t ownership_flash_lockdown(boot_data_t *bootdata,
uint32_t active_slot,
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,
/*lockdown=*/active_slot));
HARDENED_RETURN_IF_ERROR(owner_block_flash_apply(config->flash, kBootSlotB,
/*lockdown=*/active_slot));
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, kBootSlotA,
/*creator_lockdown=*/bootlog->rom_ext_slot,
/*owner_lockdown=*/bootlog->bl0_slot));
HARDENED_RETURN_IF_ERROR(
owner_block_flash_apply(config->flash, kBootSlotB,
/*creator_lockdown=*/bootlog->rom_ext_slot,
/*owner_lockdown=*/bootlog->bl0_slot));
} else {
HARDENED_CHECK_NE(bootdata->ownership_state, kOwnershipStateLockedOwner);
}
Expand Down
4 changes: 2 additions & 2 deletions sw/device/silicon_creator/lib/ownership/ownership.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include "sw/device/lib/base/hardened.h"
#include "sw/device/silicon_creator/lib/boot_data.h"
#include "sw/device/silicon_creator/lib/boot_log.h"
#include "sw/device/silicon_creator/lib/error.h"
#include "sw/device/silicon_creator/lib/ownership/datatypes.h"
#include "sw/device/silicon_creator/lib/ownership/owner_block.h"
Expand All @@ -25,8 +26,7 @@ rom_error_t ownership_init(boot_data_t *bootdata, owner_config_t *config,
* @param config The current owner configuration.
* @return error state.
*/
rom_error_t ownership_flash_lockdown(boot_data_t *bootdata,
uint32_t active_slot,
rom_error_t ownership_flash_lockdown(boot_data_t *bootdata, boot_log_t *bootlog,
const owner_config_t *config);

/**
Expand Down
Loading
Loading