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.

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

RFC: Initial ROM vendor hooks implementation #25642

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion WORKSPACE.bzlmod
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ open_dice_repos()

# Setup for linking in externally managed test and provisioning customizations
# for both secure/non-secure manufacturer domains.
load("//rules:hooks_setup.bzl", "hooks_setup", "provisioning_exts_setup", "secure_hooks_setup")
load("//rules:hooks_setup.bzl", "hooks_setup", "provisioning_exts_setup", "secure_hooks_setup", "rom_hooks_setup")
hooks_setup(
name = "hooks_setup",
dummy = "sw/device/tests/closed_source",
Expand All @@ -112,16 +112,23 @@ provisioning_exts_setup(
name = "provisioning_exts_setup",
dummy = "sw/device/silicon_creator/manuf/extensions",
)
rom_hooks_setup(
name = "rom_hooks_setup",
dummy = "sw/device/silicon_creator/rom/hooks",
)

# Declare the external repositories:
# - One for both manufacturer secure and non-secure domains.
# - One for provisioning source code extensions.
# - One for ROM vendor hooks.
load("@hooks_setup//:repos.bzl", "hooks_repo")
load("@secure_hooks_setup//:repos.bzl", "secure_hooks_repo")
load("@provisioning_exts_setup//:repos.bzl", "provisioning_exts_repo")
load("@rom_hooks_setup//:repos.bzl", "rom_hooks_repo")
hooks_repo(name = "manufacturer_test_hooks")
secure_hooks_repo(name = "secure_manufacturer_test_hooks")
provisioning_exts_repo(name = "provisioning_exts")
rom_hooks_repo(name = "rom_hooks")

# The nonhermetic_repo imports environment variables needed to run vivado.
load("//rules:nonhermetic.bzl", "nonhermetic_repo")
Expand Down
24 changes: 24 additions & 0 deletions rules/hooks_setup.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,14 @@ def provisioning_exts_repo(name):
)
"""

_ROM_HOOKS_TEMPLATE = """
def rom_hooks_repo(name):
native.local_repository(
name = name,
path = "{hooks_dir}",
)
"""

_BUILD = """
exports_files(glob(["**"]))
"""
Expand Down Expand Up @@ -77,3 +85,19 @@ provisioning_exts_setup = repository_rule(
},
environ = ["PROV_EXTS_DIR"],
)

def _rom_hooks_setup_impl(rctx):
rom_hooks_dir = rctx.os.environ.get("ROM_HOOKS_DIR", rctx.attr.dummy)
rctx.file("repos.bzl", _ROM_HOOKS_TEMPLATE.format(hooks_dir = rom_hooks_dir))
rctx.file("BUILD.bazel", _BUILD)

rom_hooks_setup = repository_rule(
implementation = _rom_hooks_setup_impl,
attrs = {
"dummy": attr.string(
mandatory = True,
doc = "Location of the dummy ROM extensions directory.",
),
},
environ = ["ROM_HOOKS_DIR"],
)
123 changes: 101 additions & 22 deletions sw/device/silicon_creator/lib/cfi.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,97 @@ enum {
kCfiIncrement = 0x5a,
};

/**
* Initializes the CFI counter at `index` with its respective initialization
* constant plus `kCfiIncrement`.
*
* This function calls `barrier32()` to synchronize the counter update.
*
* @param table The counters array variable.
* @param index The counter index.
* @param init_value The counter's initial value.
*/
static inline void cfi_func_counter_init(uint32_t table[], size_t index,
uint32_t init_value) {
table[index] = (init_value + kCfiIncrement);
barrier32(table[index]);
}

/**
* Converts a CFI step value into an expected counter value.
*
* @param init_value The counter's initial value.
* @param step The increment step.
*/
static inline uint32_t cfi_step_to_count(uint32_t init_value, uint32_t step) {
return init_value + (step * kCfiIncrement);
}

/**
* Increments the CFI counter at `index` if the current count is equivalent to
* the provided `step`. It throws an irrecoverable exception otherwise.
*
* This function calls `barrier32()` to synchronize the counter update.
*
* @param table The counters array variable.
* @param index The counter index.
* @param step The equivalent step for the current counter value.
* @param init_value The counter's initial value.
*/
static inline void cfi_func_counter_increment(uint32_t table[], size_t index,
uint32_t step,
uint32_t init_value) {
HARDENED_CHECK_EQ(table[index], cfi_step_to_count(init_value, step));
table[index] += kCfiIncrement;
barrier32(table[index]);
}

/**
* Prepare counters for function call.
*
* The `src` and `target` counters are associated with the caller and the
* callee functions, respectively. The caller uses this macro to initialize
* the `target` counter between increments of the `src` counter.
*
* The `src` counter can be verified at a later time to get a high confidence
* measurement that the target counter was initialized properly by the caller
* before entering the callee function.
*
* This function calls the `cfi_func_counter_increment()` and
* `cfi_func_counter_init()` functions, which use `barrier32()` to synchronize
* all counter updates. An irrecoverable exception is thrown if an unexpected
* `src` count value is found.
*
* @param table The counters array variable.
* @param src Index counter associated with the caller function.
* @param src_step Initial expected step of the `src` counter.
* @param target Index counter associated with the calee function.
* @param src_init_value The caller's counter's initial value.
* @param target_init_value The calle's counter's initial value.
*/
static inline void cfi_func_counter_prepcall(uint32_t table[], size_t src,
uint32_t src_step, size_t target,
uint32_t src_init_value,
uint32_t target_init_value) {
cfi_func_counter_increment(table, src, src_step, src_init_value);
cfi_func_counter_init(table, target, target_init_value);
cfi_func_counter_increment(table, src, src_step + 1, src_init_value);
}

/**
* Compares the equivalent counter value of the counter at `index` against the
* provided `step` value. Throws an irrecoverable exception on mismatch.
*
* @param table The counters array variable.
* @param index The counter index.
* @param step The equivalent step for the counter value.
* @param init_value The counter's initial value.
*/
static inline void cfi_func_counter_check(uint32_t table[], size_t index,
uint32_t step, uint32_t init_value) {
HARDENED_CHECK_EQ(table[index], cfi_step_to_count(init_value, step));
}

/**
* Initializes the CFI counter at `index` with its respective initialization
* constant plus `kCfiIncrement`.
Expand All @@ -114,19 +205,16 @@ enum {
* @param table The counters array variable.
* @param index The counter index.
*/
#define CFI_FUNC_COUNTER_INIT(table, index) \
do { \
table[index] = (index##Val0 + kCfiIncrement); \
barrier32(table[index]); \
} while (0)
#define CFI_FUNC_COUNTER_INIT(table, index) \
cfi_func_counter_init(table, index, index##Val0)

/**
* Converts a CFI step value into an expected counter value.
*
* @param index The counter index.
* @param step The increment step.
*/
#define CFI_STEP_TO_COUNT(index, step) ((index##Val0) + (step)*kCfiIncrement)
#define CFI_STEP_TO_COUNT(index, step) cfi_step_to_count(index##Val0, step)

/**
* Increments the CFI counter at `index` if the current count is equivalent to
Expand All @@ -138,12 +226,8 @@ enum {
* @param index The counter index.
* @param step The equivalent step for the current counter value.
*/
#define CFI_FUNC_COUNTER_INCREMENT(table, index, step) \
do { \
HARDENED_CHECK_EQ(table[index], CFI_STEP_TO_COUNT(index, step)); \
table[index] += kCfiIncrement; \
barrier32(table[index]); \
} while (0)
#define CFI_FUNC_COUNTER_INCREMENT(table, index, step) \
cfi_func_counter_increment(table, index, step, index##Val0)

/**
* Prepare counters for function call.
Expand All @@ -165,12 +249,9 @@ enum {
* @param src_step Initial expected step of the `src` counter.
* @param target Index counter associated with the calee function.
*/
#define CFI_FUNC_COUNTER_PREPCALL(table, src, src_step, target) \
do { \
CFI_FUNC_COUNTER_INCREMENT(table, src, src_step); \
CFI_FUNC_COUNTER_INIT(table, target); \
CFI_FUNC_COUNTER_INCREMENT(table, src, (src_step + 1)); \
} while (0)
#define CFI_FUNC_COUNTER_PREPCALL(table, src, src_step, target) \
cfi_func_counter_prepcall(table, src, src_step, target, src##Val0, \
target##Val0)

/**
* Compares the equivalent counter value of the counter at `index` against the
Expand All @@ -180,9 +261,7 @@ enum {
* @param index The counter index.
* @param step The equivalent step for the counter value.
*/
#define CFI_FUNC_COUNTER_CHECK(table, index, step) \
do { \
HARDENED_CHECK_EQ(table[index], CFI_STEP_TO_COUNT(index, step)); \
} while (0)
#define CFI_FUNC_COUNTER_CHECK(table, index, step) \
cfi_func_counter_check(table, index, step, index##Val0)

#endif // OPENTITAN_SW_DEVICE_SILICON_CREATOR_LIB_CFI_H_
18 changes: 18 additions & 0 deletions sw/device/silicon_creator/rom/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ cc_library(
"//sw/device/lib/base:csr",
"//sw/device/lib/base:macros",
"//sw/device/silicon_creator/lib:error",
"//sw/device/silicon_creator/lib:shutdown",
],
)

Expand Down Expand Up @@ -135,6 +136,7 @@ cc_library(
":boot_policy_ptrs",
":bootstrap",
":rom_epmp",
":rom_state",
":sigverify_keys_ecdsa_p256",
":sigverify_keys_spx",
":sigverify_otp_keys",
Expand Down Expand Up @@ -177,6 +179,7 @@ cc_library(
"//sw/device/silicon_creator/lib/drivers:uart",
"//sw/device/silicon_creator/lib/drivers:watchdog",
"//sw/device/silicon_creator/lib/sigverify",
"@rom_hooks",
],
)

Expand Down Expand Up @@ -255,6 +258,21 @@ cc_library(
],
)

cc_library(
name = "rom_state",
srcs = ["rom_state.c"],
hdrs = ["rom_state.h"],
target_compatible_with = [OPENTITAN_CPU],
deps = [
"//sw/device/lib/base:hardened",
"//sw/device/lib/base:macros",
"//sw/device/silicon_creator/lib:cfi",
"//sw/device/silicon_creator/lib:error",
"//sw/device/silicon_creator/lib:shutdown",
],
alwayslink = True,
)

opentitan_test(
name = "rom_epmp_test",
srcs = [
Expand Down
98 changes: 98 additions & 0 deletions sw/device/silicon_creator/rom/doc/rom_customization.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
---
Document: RFC
Title: ROM Vendor Customizations
Status: Draft
Authors: Samuel Ortiz <[email protected]>
Reviewers:
---

# RFC: ROM Vendor Customizations

## Problem Statement

Currently, OpenTitan mask ROM implementations lack customization options for silicon creators and vendors. While discrete OpenTitan ROMs can be directly built upon the [upstream implementation](https://github.com/lowRISC/opentitan/tree/master/sw/device/silicon_creator/rom), the same isn't true for integrated OpenTitan top ROMs.

Integrated OpenTitan ROMs configure, initialize, and interact with external subsystems specific to the platform. These interactions often vary between vendors and platform variants. A single, fixed ROM implementation prevents downstream silicon creators and vendors from directly utilizing the upstream code, forcing them to fork the project and maintain custom versions internally.

## Requirements
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment from @moidx in #25633 (comment):

Are we planning to define additional requirements for out of band verification? For example, can we ask silicon integrators to document the expected ROM version and associated hash for any given silicon release?

  • The information can be used to perform out of band checks against the device attestation (e.g. it may help in providing additional supply chain hardening).
  • The information can be classified as public or restricted depending on the silicon integration requirements.
  • The information can be incorporated into OCP's S.A.F.E framework for integrations requiring third party audits; or by Common Criteria certification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are we planning to define additional requirements for out of band verification? For example, can we ask silicon integrators to document the expected ROM version and associated hash for any given silicon release?

* The information can be used to perform out of band checks against the device attestation (e.g. it may help in providing additional supply chain hardening).

I am expecting a device attestation report to include its boot ROM hash, and that could be used by a verifier against a set of reference values that thus would need to include the expected ROM hash and version. Those reference values are typically built and signed by the silicon creators, so this is even more than just documenting.
Also, this should already be a requirement today, independent of this RFC proposal here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment from @moidx in #25633 (comment):

Hi @sameo, I brought this up because in cases where there are no customization hooks, the verifier can perform an out of band check by building the ROM from source. I understand that the out of band measurement is a requirement from the verifier's perspective, but thought it would be good to have this included in the documentation. Mentioning that this is covered by an attestation report is sufficient from my perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey @moidx. I see, this makes sense.

I added an additional attestation requirement to address that one.


To enable direct vendor contribution and consumption of OpenTitan upstream ROM implementations, the project architecture and source code for ROMs must meet the following requirements:

1. Vendor customization: OpenTitan ROM code should allow silicon creators and vendors to customize the upstream ROM execution flow with vendor-specific initialization and configuration sequences.
2. Controlled customization points: OpenTitan ROMs should be customizable at specific, predefined execution points. Vendors can only modify ROM behavior at these designated points.
3. Support for closed-source customization: The OpenTitan ROM build infrastructure should support linking ROM images with externally stored, vendor-specific customization code, allowing vendors to keep their customizations private.
4. Attestation: Silicon creators and vendors should provide endorsed reference values for their customized OpenTitan ROMs, including the ROM version and its associated hash. Out-of-band verifiers can then compare these values with the ROM's generated attestation reports.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @moidx

5. Default functionality: By default, OpenTitan ROMs should function without any vendor-specific customizations.

## Proposed Approach

At a high level, the proposed solution involves implementing an OpenTitan ROM as a state machine. Each state in this state machine is semantically defined by the upstream ROM code and represents a customizable execution step.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment from @modix in #25633 (comment):

State machine considerations:

We are still working on evaluating the effectiveness of hardware countermeasures with respect to fault attacks. Until then, we recommend to maintain the level of hardening present in the upstream version of the ROM.

In practice, it is very difficult to harden code with too many branches. Ideally, we should aim as much as possible to maintain a linear execution sequence. I propose we define a requirement to cover this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still working on evaluating the effectiveness of hardware countermeasures with respect to fault attacks. Until then, we recommend to maintain the level of hardening present in the upstream version of the ROM.

One interesting side effect of defining the ROM as a state machine is that you can systematically include CFI within the state machine walking routine. I have a pending patch to show this in practice, but the idea is that each ROM state would be protected by its implicit CFI counter so that the ROM state machine flow is controlled.

In practice, it is very difficult to harden code with too many branches. Ideally, we should aim as much as possible to maintain a linear execution sequence. I propose we define a requirement to cover this.

That makes sense, yes. To be clear here, the current ROM is not linear and has many branches, mostly for error handling but also for the bootstrap and immutable ROM extension flows for example. If anything, I feel that explicitly defining the ROM as a state machine would allow for more easily tracking those branches than the current implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In practice, it is very difficult to harden code with too many branches. Ideally, we should aim as much as possible to maintain a linear execution sequence. I propose we define a requirement to cover this.

That makes sense, yes. To be clear here, the current ROM is not linear and has many branches, mostly for error handling but also for the bootstrap and immutable ROM extension flows for example. If anything, I feel that explicitly defining the ROM as a state machine would allow for more easily tracking those branches than the current implementation.

As @Razer6 pointed out privately, with an actual state machine we might be able to more easily enforce that ROM states functions are actually linear (except for error paths) and branches are the state machine transitions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are still working on evaluating the effectiveness of hardware countermeasures with respect to fault attacks. Until then, we recommend to maintain the level of hardening present in the upstream version of the ROM.

One interesting side effect of defining the ROM as a state machine is that you can systematically include CFI within the state machine walking routine. I have a pending patch to show this in practice, but the idea is that each ROM state would be protected by its implicit CFI counter so that the ROM state machine flow is controlled.

I added a couple of commits to this PR to illustrate how CFI counters could be inserted in the ROM state machine walker.


As the ROM progresses through its state machine, it executes the current state and then transitions to the next one. A state execution consists of the following three sequential steps:

1. Pre-run Hook: Executed before the primary ROM state function.
2. ROM State Run: The core execution of the ROM state.
3. Post-run Hook: Executed after the primary ROM state function.

By structuring the ROM as a state machine with these hooks, vendors can customize the ROM's behavior at specific points without modifying the upstream ROM code.

```
Initial
┌ROM State───┐
│ ┌────────┐ │
│ │Pre-run │ │
│ ├────────┤ │
│ │ Run │ │
│ ├────────┤ │
│ │Post-run│ │
│ └────────┘ │
└──────┬─────┘
ROM ┌─────────┴─────────┐ ROM
┌State─▽─────┐ ┌──────▽──State
│ ┌────────┐ │ │ ┌────────┐ │
│ │Pre-run │ │ │ │Pre-run │ │
│ ├────────┤ │ │ ├────────┤ │
│ │ Run │ │ │ │ Run │ │
│ ├────────┤ │ │ ├────────┤ │
│ │Post-run│ │ │ │Post-run│ │
│ └────────┘ │ │ └────────┘ │
└────────────┘ └──────┬─────┘
│ ROM
┌──────▽──State
│ ┌────────┐ │
│ │Pre-run │ │
│ ├────────┤ │
│ │ Run │ │
│ ├────────┤ │
│ │Post-run│ │
│ └────────┘ │
└────────────┘
```

With this proposal, vendors can customize an OpenTitan ROM by providing custom implementations for the pre-run and post-run hooks of any ROM state. By default, these hooks are empty.

The main ROM state implementation, the run callback, is executed in step 2. All ROM state run callbacks are defined by the upstream ROM implementation and are not customizable.

Crucially, only the run callback determines the next state in the ROM state machine. Pre-run and post-run hooks cannot override these transitions, preventing vendors from directly modifying the ROM's execution flow.

In essence, this proposal enables vendors to customize the execution of individual ROM states, but it strictly controls the overall ROM execution flow.

### ROM states

Each OpenTitan ROM defines a set of execution states and associates each state with a specific ROM state run callback. A ROM state run callback takes an opaque pointer argument (which is shared with the state hooks) and returns the next state in the sequence.

To enable effective vendor customization, ROM states should be clearly defined and map to specific functional steps in the ROM's execution flow. For instance, `kRomStateBootstrap` is a well-defined state, while `kRomStateStep3` is too generic.

The more precisely a ROM state is defined, the easier it is for vendors to understand and customize its behavior.

### ROM state hooks

Pre-run and post-run ROM state hooks are executed by the ROM state machine before and after each state run callback, respectively. They share state with the state run callbacks through an opaque pointer argument.

Unlike the state run callback, ROM state hooks cannot determine the next state in the ROM state machine. Vendors can customize specific ROM state execution steps through these hooks by providing custom implementations in an external repository.

Similar to the existing manufacturing and test hook customization framework, custom ROM state hooks can be included in a ROM image by specifying the path to the vendor hook implementations in an environment variable passed to the ROM build system:

``` bash
ROM_HOOKS_DIR=/PATH/TO/LOCAL/ROM/HOOKS ./bazelisk.sh build //sw/device/silicon_creator/rom:mask_rom
Copy link
Contributor Author

@sameo sameo Dec 19, 2024

Choose a reason for hiding this comment

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

Comment from @a-will in #25633 (comment):

Just an implementation nit: It would be nice if we could avoid more environment variable dependencies. Perhaps bazel's --override_module could help here instead, where we define a rom_hooks module (which contains the upstream implementations) that could be overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment from @moidx in #25633 (comment):

We should look at all existing integration hooks (1. closed source DV MANUFACTURER_HOOKS_DIR, 2. personalization PROV_EXTS_DIR, 3. ROM ROM_HOOKS_DIR, 4. etc), to determine if it makes sense to provide more generic hook infrastructure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. As @a-will proposed, we could look at using Bazel's module override and make all external hooks expose themselves as an external module to OpenTitan.

```
12 changes: 12 additions & 0 deletions sw/device/silicon_creator/rom/hooks/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# Copyright lowRISC contributors (OpenTitan project).
# Licensed under the Apache License, Version 2.0, see LICENSE for details.
# SPDX-License-Identifier: Apache-2.0

package(default_visibility = ["//visibility:public"])

cc_library(
name = "rom_hooks",
srcs = ["dummy_rom_hooks.c"],
deps = ["@//sw/device/silicon_creator/rom:rom_state"],
alwayslink = True,
)
Loading
Loading