Skip to content

Conversation

@ayj
Copy link

@ayj ayj commented Dec 1, 2025

This adds code to test the following scenario:

  1. boot owner FW to sign something with the CDI_1 key,
  2. boot a different owner FW to ensure the key changes and produces a different signature
  3. boot the same owner FW in (1) to ensure the same signature in 1 can be reproduced.

This adds code to test the following scenario:
1. boot owner FW to sign something with the CDI_1 key,
2. boot a different owner FW to ensure the key changes and produces a
   different signature
3. boot the same owner FW in (1) to ensure the same signature in 1 can
   be reproduced.

Signed-off-by: Tim Trippel <[email protected]>
Signed-off-by: Jason Young <[email protected]>
@ayj ayj requested review from a team as code owners December 1, 2025 23:32
@ayj ayj requested review from cfrantz and pamaury and removed request for a team December 1, 2025 23:32
"//sw/device/silicon_creator/lib/drivers:rstmgr",
],
)
for i in range(3)
Copy link
Contributor

@pamaury pamaury Dec 2, 2025

Choose a reason for hiding this comment

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

It seems that the binary for i=0 is the same as the binary for i=2 so I would suggest to only create two binaries. If you really want a third create an alias:

alias(name = "cdi1_key_sign_for_assemble_2", actual="cdi1_key_sign_for_assemble_0")

"//hw/top_earlgrey:fpga_cw340_rom_with_fake_keys",
],
)
for i in range(3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here?

test_cmd = """
--bootstrap={firmware}
--second-bootstrap={second}
--third-bootstrap={third}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I think you could just set --third-bootstrap={firmware}

"//hw/top_earlgrey:fpga_cw310_rom_ext",
"//hw/top_earlgrey:fpga_cw340_rom_ext",
],
linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_a",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a particular reason to specify the linker script? If none is specified, a default one is used which comes from the exec_env and I think it would be fine for this test.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, just delete this line

Suggested change
linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_a",

@pamaury
Copy link
Contributor

pamaury commented Dec 2, 2025

I might be missing something but it seems to me that the test doesn't quite match the specification: each firmware signs a piece of data with the CDI_1 and the CDI_0 and (essentially) checks that the CDI_0 signature doesn't pass with the CDI_1 key. But this doesn't verify that the CDI_1 from the second firmware is different from the first firmware?

@timothytrippel timothytrippel self-requested a review December 8, 2025 00:05
"//hw/top_earlgrey:fpga_cw310_rom_ext",
"//hw/top_earlgrey:fpga_cw340_rom_ext",
],
linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_a",
Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, just delete this line

Suggested change
linker_script = "//sw/device/lib/testing/test_framework:ottf_ld_silicon_owner_slot_a",

*kDiceKeyCdi0.keymgr_diversifier, &pk_cdi0));

// Compute the CDI_1 public key and side-load the CDI_1 private key into OTBN
// to preapre for otbn_boot_attestation_endorse.
Copy link
Contributor

Choose a reason for hiding this comment

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

The CDI_1 public and private keys should be sitting in OTBN DMEM when the owner firmware (i.e. this test case) boots as it is generated just before booting the owner FW: https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/rom_ext/rom_ext.c;l=423?q=rom_ext.c&ss=opentitan%2Fopentitan

If you need the pubkey for verification purposes later, you should be able to read this out of DMEM directly using the silicon_creator keymgr driver: https://cs.opensource.google/opentitan/opentitan/+/master:sw/device/silicon_creator/lib/otbn_boot_services.c;l=120?q=otbn_boot_attestation_keygen&ss=opentitan%2Fopentitan

You should be able to just sign something with what is already there by calling the otbn_boot_attestation_endorse() function without doing any keygen before.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also could you just hash and sign a hardcoded string message?

Comment on lines +18 to +22
// Compute the CDI_0 public key.
ecdsa_p256_public_key_t pk_cdi0;
RETURN_IF_ERROR(otbn_boot_attestation_keygen(
kDiceKeyCdi0.keygen_seed_idx, kDiceKeyCdi0.type,
*kDiceKeyCdi0.keymgr_diversifier, &pk_cdi0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you attempting to generate the CDI_0 key?

  1. you won't be able to b/c this test runs at the Owner FW stage, where the keymgr as already been cranked to the OwnerKey (CDI_1) key stage by the time the FW executes (see the diagram here: https://opentitan.org/book/doc/security/specs/identities_and_root_keys/#overview).
  2. you should not need to. All you need is the CDI_1 key pair correct?

otbn_boot_sigverify(&pk_cdi1, &sig, &digest, recovered_r_cdi1));
CHECK_ARRAYS_EQ(recovered_r_cdi1, sig.r, ARRAYSIZE(sig.r));

// Digest should fail to verify with CDI_0 public key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you attempting to verify with the CDI_0 pubkey? you won't be able to regen this key at the Owner FW stage as mentioned above (see https://opentitan.org/book/doc/security/specs/identities_and_root_keys/#overview).

@timothytrippel
Copy link
Contributor

Also this test should be cherry-picked to the earlgrey_1.0.0 branch once this merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants