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

[manuf,rom_ext] write certs to flash in perso LTV object format #24992

Merged

Conversation

timothytrippel
Copy link
Contributor

@timothytrippel timothytrippel commented Nov 2, 2024

Currently, during ft_perso, the host writes the raw endorsed X.509 certificates (in ASN.1 DER format) to flash_info pages. When it needs to access them later on, the certificate length is calculated by parsing the ASN.1 header (1 or 2 bytes). This design works for X509 but not CWT-CBOR since there's no such information in some CBOR types (map, array specifically). This updates the perso flow to write the certificates to the info page wrapped in the perso LTV object header. Additionally, this refactors the perso_tlv_data code to use rom_error_t where applicable so it can be shared with the ROM_EXT, which also must parse and potentially update CDI_0/1 DICE certificates.

Fixes #24942.
Test: //sw/device/silicon_creator/manuf/base:ft_provision_cw340

This is a refactor of #24951 that improves code-reuse and documentation.

@timothytrippel
Copy link
Contributor Author

timothytrippel commented Nov 2, 2024

Still need to add the ROM_EXT update for parsing the new cert format. I will update the PR shortly before merging.

timothytrippel and others added 2 commits November 1, 2024 22:07
Currently, during ft_perso, the host writes the raw endorsed X.509
certificates (in ASN.1 DER format) to flash_info pages.
When it needs to access them later on, the certificate length is
calculated by parsing the ASN.1 header (1 or 2 bytes).
This design works for X509 but not CWT-CBOR since there's no such
information in some CBOR types (map, array specifically).

Fixes lowRISC#24942.

Co-authored-by: Tommy Chiu <[email protected]>
Signed-off-by: Tim Trippel <[email protected]>
This switches the `perso_tlv_get_cert_obj()` return type to a
`rom_error_t` as this function will be used in the ROM_EXT to parse DICE
certificates in a following commit.

Signed-off-by: Tim Trippel <[email protected]>
@timothytrippel timothytrippel force-pushed the update-cert-flash-format branch 2 times, most recently from daa4561 to 276a6de Compare November 2, 2024 07:45
@timothytrippel timothytrippel marked this pull request as draft November 2, 2024 07:48
@timothytrippel timothytrippel marked this pull request as ready for review November 4, 2024 03:23
@timothytrippel
Copy link
Contributor Author

I have added the code that update the ROM_EXT to use the same perso LTV format when reading/writing DICE certs from/to flash.

This PR is ready for review now.

if (obj_len > (sizeof(pb->body) - pb->next_free))
return OUT_OF_RANGE();
if (obj_size > *buf_size)
return kErrorPersoTlvOutputBufTooSmall;

// Setup the perso LTV object header.
if (needs_endorsement) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Using this option to set object type is not practiable for CWT.
Maybe we'll need one more parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah lets add in follow up PR that adds code for CWT support

@vbendeb
Copy link

vbendeb commented Nov 4, 2024

This seems to be a much larger change than modifying the storage format by adding the TLV header, a lot of refactoring, variable names changes, editing comments drown the actual modification, which makes it harder to review.

How has this been tested?

This refactors the `perso_tlv_data` cert object building functions so
they can be reused by the ROM_EXT. A prior commit updated the perso code
to store all certificates in flash info pages in the perso LTV object
format. Since the ROM_EXT needs to be able to regenerate CDI_0/1 DICE
certificates, the ROM_EXT must also be able to regenerate this same
perso LTV object format.

Signed-off-by: Tim Trippel <[email protected]>
@timothytrippel
Copy link
Contributor Author

timothytrippel commented Nov 4, 2024

This seems to be a much larger change than modifying the storage format by adding the TLV header, a lot of refactoring, variable names changes, editing comments drown the actual modification, which makes it harder to review.

How has this been tested?

This has been tested on all provisioning flows locally and in CI. This is needed in order to support the new cert format in perso and ROM_EXT code, and to enable code sharing between both. Also, I broke the changes up into several smaller commits that should make each commit easier to review. Please lmk if you have any questions.

@timothytrippel
Copy link
Contributor Author

FWIW @vbendeb , CI always tests the perso flow, so if a change passes CI it should be guaranteed to work.

@moidx moidx removed the request for review from a team November 4, 2024 19:10
@timothytrippel timothytrippel changed the title [manuf] write certs to flash in perso LTV object format [manuf,rom_ext] write certs to flash in perso LTV object format Nov 4, 2024
sw/device/silicon_creator/rom_ext/rom_ext.c Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/rom_ext.c Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/rom_ext.c Outdated Show resolved Hide resolved
sw/device/silicon_creator/rom_ext/rom_ext.c Outdated Show resolved Hide resolved
The format by which DICE certs have been to provisioned into flash
recently changed such that the certs were written in the perso LTV
structure. This updates the ROM_EXT to use the same structure when
reading and writing DICE certs from/to flash.

Signed-off-by: Tim Trippel <[email protected]>
@timothytrippel
Copy link
Contributor Author

Test failures are not related to this PR.

@timothytrippel timothytrippel merged commit 4eafa4f into lowRISC:master Nov 5, 2024
39 of 41 checks passed
@timothytrippel timothytrippel deleted the update-cert-flash-format branch November 5, 2024 17:57
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.

[manuf] Propose to store DICE certificates in the flash_info with the "Cert Header"
4 participants