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

Add CFI to DPE #310

Merged
merged 1 commit into from
Feb 14, 2024
Merged

Add CFI to DPE #310

merged 1 commit into from
Feb 14, 2024

Conversation

sree-revoori1
Copy link
Contributor

No description provided.

Copy link
Collaborator

@jhand2 jhand2 left a comment

Choose a reason for hiding this comment

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

Let me know if I'm misunderstanding, but a general comment is it seems like the CFI codepaths may not really be tested here, since I don't think we implement cfi_panic_handler anywhere in this repo. So any issues with final linking or something like that will only be caught once this is built into caliptra-sw.

But this PR is already fairly large, so maybe we should just open an issue for how to better test this inside of caliptra-dpe.

Cargo.toml Outdated Show resolved Hide resolved
dpe/src/commands/certify_key.rs Show resolved Hide resolved
crypto/Cargo.toml Outdated Show resolved Hide resolved
ci.sh Show resolved Hide resolved
dpe/src/commands/mod.rs Show resolved Hide resolved
@sree-revoori1
Copy link
Contributor Author

sree-revoori1 commented Feb 13, 2024

Let me know if I'm misunderstanding, but a general comment is it seems like the CFI codepaths may not really be tested here, since I don't think we implement cfi_panic_handler anywhere in this repo. So any issues with final linking or something like that will only be caught once this is built into caliptra-sw.

But this PR is already fairly large, so maybe we should just open an issue for how to better test this inside of caliptra-dpe.

We are testing it since I added the dev-dependency caliptra-cfi-lib with the cfi-test feature. caliptra-cfi has a cfi_panic_handler and cfi_state for testing purposes behind the cfi-test feature. I confirmed that CFI codepaths are being hit in the CI too.

@jhand2
Copy link
Collaborator

jhand2 commented Feb 14, 2024

Let me know if I'm misunderstanding, but a general comment is it seems like the CFI codepaths may not really be tested here, since I don't think we implement cfi_panic_handler anywhere in this repo. So any issues with final linking or something like that will only be caught once this is built into caliptra-sw.
But this PR is already fairly large, so maybe we should just open an issue for how to better test this inside of caliptra-dpe.

We are testing it since I added the dev-dependency caliptra-cfi-lib with the cfi-test feature. caliptra-cfi has a cfi_panic_handler and cfi_state for testing purposes behind the cfi-test feature. I confirmed that CFI codepaths are being hit in the CI too.

Oh cool, thanks!

@jhand2 jhand2 merged commit f048f4a into chipsalliance:main Feb 14, 2024
1 check passed
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.

2 participants