Self Signed FMC Alias Csr #1863

rusty1968 · 2024-12-19T15:45:58Z

FMC modfied to generate a self signed FMC Alias CSR test upon cold boot.
Persistent driver modified to add persistent memory for the FMC Alias CSR
Runtime modified to expose an API to retrieve it.
Test case created to verify the self signed FMC Alias CSR.
Test case created to verify the RT Alias Certificate with the pub key of the FMC Alias CSR.

clundin25 · 2024-12-19T19:13:43Z

api/src/mailbox.rs

@@ -1010,6 +1016,41 @@ impl Default for GetIdevCsrResp {
    }
 }

+// GET_IDEVID_CSR


nit: Minor copy paste error

Corrected. Thank you.

clundin25 · 2024-12-19T19:18:20Z

drivers/src/persistent.rs

+    #[cfg(feature = "fmc")]
+    reserved11: [u8; memory_layout::FMC_ALIAS_CSR_SIZE as usize - size_of::<FmcAliasCsr>()],


When the fmc is not enabled, a slice of memory_layout::FMC_ALIAS_CSR_SIZE should still be reserved so PersistentData is the same size / shape between image types.

Corrected. Thanks.

clundin25 · 2024-12-19T19:19:03Z

error/src/lib.rs

@@ -450,6 +450,9 @@ impl CaliptraError {
    pub const RUNTIME_AUTH_MANIFEST_IMAGE_METADATA_LIST_DUPLICATE_FIRMWARE_ID: CaliptraError =
        CaliptraError::new_const(0x000E0053);

+    pub const RUNTIME_GET_FMC_CSR_UNPROVISIONED: CaliptraError =
+        CaliptraError::new_const(0x000E0053);


0x000E0053 => 0x000E0054 so RUNTIME_GET_FMC_CSR_UNPROVISIONED has a unique error code.

Corrected, Thanks.

clundin25 · 2024-12-19T19:21:08Z

fmc/src/flow/crypto.rs

+    /// # Arguments
+    ///
+    /// * `env` - FMC Environment
+    /// * `priv_key` - Key slot to retrieve the private key


Looks like pub_key is missing from the doc comment.

It was. Thanks.

clundin25 · 2024-12-19T19:22:42Z

fmc/src/flow/fmc_alias_csr.rs

+/// * `hand_off` - HandOff
+///
+/// # Returns
+///
+/// * `DiceInput` - DICE Layer Input


Looks like the doc string doesn't match the function signature.

It did not. Thanks.

clundin25 · 2024-12-19T19:24:49Z

runtime/src/get_fmc_alias_csr.rs

+        let csr_persistent_mem = &drivers.persistent_data.get().fmc_alias_csr;
+
+        match csr_persistent_mem.get_csr_len() {
+            FmcAliasCsr::UNPROVISIONED_CSR => Err(CaliptraError::RUNTIME_GET_FMC_CSR_UNPROVISIONED),


Does the 0 case matter? E.g. for FMC images that did not support this feature?

It does. Thanks.

clundin25 · 2024-12-19T19:24:55Z

runtime/src/get_fmc_alias_csr.rs

+
+pub struct GetFmcAliasCsrCmd;
+impl GetFmcAliasCsrCmd {
+    //   #[cfg_attr(not(feature = "no-cfi"), cfi_impl_fn)]


nit: Commented code

clundin25 · 2024-12-19T19:25:27Z

runtime/src/get_fmc_alias_csr.rs

+                // A valid `IDevIDCsr` cannot be larger than `MAX_CSR_SIZE`, which is the max
+                // size of the buffer in `GetIdevCsrResp`


nit: Copy and paste mismatch

jhand2 · 2024-12-20T01:01:23Z

common/src/lib.rs

@@ -38,9 +38,9 @@ pub use fuse::{FuseLogEntry, FuseLogEntryId};
 pub use pcr::{PcrLogEntry, PcrLogEntryId, RT_FW_CURRENT_PCR, RT_FW_JOURNEY_PCR};

 pub const FMC_ORG: u32 = 0x40000000;
-pub const FMC_SIZE: u32 = 20 * 1024;


According to the size-history CI job, we're still only using ~18KiB in FMC. Do we need to change the size here?

I had to increase it. If I keep it the same it won't fit:
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 312 bytes
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 961 bytes
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 964 bytes
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 976 bytes
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 988 bytes
rust-lld: error: section '.rodata' will not fit in region 'ICCM': overflowed by 988 bytes

jhand2 · 2024-12-20T01:03:33Z

drivers/src/memory_layout.rs

@@ -74,7 +75,8 @@ pub const DPE_SIZE: u32 = 5 * 1024;
 pub const PCR_RESET_COUNTER_SIZE: u32 = 1024;
 pub const AUTH_MAN_IMAGE_METADATA_MAX_SIZE: u32 = 7 * 1024;
 pub const IDEVID_CSR_SIZE: u32 = 1024;
-pub const DATA_SIZE: u32 = 27 * 1024;


nit: now that #1722 is merged, should be able to just add to the PersistentData driver and leave memory layout as-is

Agree. Thanks for pointing this out.

jhand2 · 2024-12-20T01:04:26Z

drivers/src/persistent.rs

@@ -164,6 +231,12 @@ pub struct PersistentData {

    pub idevid_csr: IdevIdCsr,
    reserved10: [u8; memory_layout::IDEVID_CSR_SIZE as usize - size_of::<IdevIdCsr>()],
+
+    #[cfg(feature = "fmc")]
+    pub fmc_alias_csr: FmcAliasCsr,


Doesn't this also need to be accessible in RT?

Correct. The feature is now renamed to fmc-alias-csr.

Why do we need a new feature for this? Can you simply use the runtime feature?

jhand2 · 2024-12-20T01:05:21Z

runtime/src/main.rs

- \____\__,_|_|_| .__/ \__|_|  \__,_| |_| \_\|_|
-               |_|
-"#;
+const BANNER: &str = r#"Caliptra RT"#;


optional: might be good to put size optimizations in a separate PR

- FMC modfied to generate a self signed FMC Alias CSR test upon cold boot. - Persistent driver modified to add persistent memory for the FMC Alias CSR - Runtime modified to expose an API to retrieve it. - Test case created to verify the self signed FMC Alias CSR. - Test case created to verify the RT Alias Certificate with the pub key of the FMC Alias CSR.

mhatrevi · 2025-01-09T21:29:46Z

common/src/lib.rs

 pub const RUNTIME_ORG: u32 = FMC_ORG + FMC_SIZE;
-pub const RUNTIME_SIZE: u32 = 97 * 1024;
+pub const RUNTIME_SIZE: u32 = 96 * 1024;


Why is this decreasing?

mhatrevi · 2025-01-09T21:50:42Z

drivers/src/persistent.rs

            persistent_data_offset += IDEVID_CSR_SIZE;
            assert_eq!(
-                addr_of!((*P).reserved_memory) as u32,
+                addr_of!((*P).fmc_alias_csr) as u32,
                memory_layout::PERSISTENT_DATA_ORG + persistent_data_offset
            );


Please re-add the check for reserved_memory.

mhatrevi · 2025-01-09T21:53:38Z

drivers/Cargo.toml

@@ -37,6 +37,7 @@ verilator = ["caliptra-hw-model/verilator"]
 no-cfi = []
 "hw-1.0" = ["caliptra-builder/hw-1.0", "caliptra-registers/hw-1.0"]
 fips-test-hooks = []
+fmc-alias-csr = []


Why is there a need for a separate feature for this?

mhatrevi · 2025-01-09T21:55:47Z

fmc/src/flow/fmc_alias_csr.rs

+    Ok(output)
+}
+
+fn write_csr_to_peristent_storage(env: &mut FmcEnv, csr: &FmcAliasCsr) -> CaliptraResult<()> {


This function can never fail so no point in keeping the return type as CaliptraResult.

Agree. Same applies to https://github.com/chipsalliance/caliptra-sw/blob/main/rom/dev/src/flow/cold_reset/idev_id.rs#L294 ?

mhatrevi · 2025-01-09T21:58:09Z

fmc/src/flow/rt_alias.rs

@@ -100,18 +99,15 @@ impl RtAliasLayer {
    #[cfg_attr(not(feature = "no-cfi"), cfi_impl_fn)]
    #[inline(never)]
    pub fn run(env: &mut FmcEnv) -> CaliptraResult<()> {


Any particular reason for removing these?

Saving space.

mhatrevi · 2025-01-09T22:03:48Z

x509/src/fmc_alias_csr.rs

+
+Abstract:
+
+    Initial Device ID Certificate Signing Request related code.


Leftover copied comment.

mhatrevi · 2025-01-09T22:03:53Z

x509/src/fmc_alias_csr.rs

+
+File Name:
+
+    idevid_csr.rs


Leftover copied comment.

mhatrevi

Please see the feedback.

rusty1968 self-assigned this Dec 19, 2024

rusty1968 added the Caliptra v1.2 label Dec 19, 2024

rusty1968 marked this pull request as ready for review December 19, 2024 18:14

rusty1968 requested review from jhand2, fdamato, bluegate010, mhatrevi, swenson, vsonims, FerralCoder, ajisaxena, jlmahowa-amd, korran and JohnTraverAmd as code owners December 19, 2024 18:14

clundin25 reviewed Dec 19, 2024

View reviewed changes

jhand2 reviewed Dec 20, 2024

View reviewed changes

rusty1968 force-pushed the fmc-csr-feature branch 4 times, most recently from ea58405 to d021950 Compare January 2, 2025 02:48

rusty1968 force-pushed the fmc-csr-feature branch 6 times, most recently from 2edab39 to 95aab5e Compare January 2, 2025 12:20

rusty1968 force-pushed the fmc-csr-feature branch from 95aab5e to bfc72aa Compare January 6, 2025 20:01

mhatrevi reviewed Jan 9, 2025

View reviewed changes

mhatrevi requested changes Jan 9, 2025

View reviewed changes

Code review feedback.

373fbac

rusty1968 force-pushed the fmc-csr-feature branch from bfc72aa to 373fbac Compare January 10, 2025 20:48

rusty1968 added 2 commits January 10, 2025 14:00

Remove fmc-alias-csr feature.

c932714

NIT changes.

da1a4a0

		#[cfg(feature = "fmc")]
		reserved11: [u8; memory_layout::FMC_ALIAS_CSR_SIZE as usize - size_of::<FmcAliasCsr>()],

		// A valid `IDevIDCsr` cannot be larger than `MAX_CSR_SIZE`, which is the max
		// size of the buffer in `GetIdevCsrResp`


		Abstract:

		Initial Device ID Certificate Signing Request related code.

Self Signed FMC Alias Csr #1863

Are you sure you want to change the base?

Self Signed FMC Alias Csr #1863

Conversation

rusty1968 commented Dec 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhatrevi left a comment

Choose a reason for hiding this comment