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

Execute FIPS Self Test in Runtime #684

Closed
wants to merge 11 commits into from

Conversation

rusty1968
Copy link
Contributor

@rusty1968 rusty1968 commented Aug 24, 2023

This commit enables Runtime to run FIPS Self Tests as follows : KATs, verification of firmware image in ICCM, and ROM integrity test.

  • Light refactor to decouple KAT crate from Rom dependency.
  • Light refactor to decouple Image verification crate from ROM dependency.
  • Ability for RT to defer execution of self test (basic job control).

Runtime copies image from ICCM to mailbox SRAM , so it needs to fake a send a transaction and hold the mailbox lock while the image is being validated,. It defers the actual self test execution until the mailbox lock can be acquired. SoC must "poll" for self test completion.

  • SoC must send a FIPS self test command one time to schedule the execution.
  • Caliptra holds the mailbox lock while the self test is in progress.
  • Failure of FIPS self test is handled as fatal error.
  • Upon successful self-test completion the mailbox lock is released and SoC will receive a positive acknowledgement when FIPS Self test command is re-issued.

@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch from 4f3d0b2 to b8553bf Compare August 24, 2023 18:53
@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch 2 times, most recently from 3ad0673 to 1e2c0ce Compare August 27, 2023 04:35
@rusty1968 rusty1968 changed the title Execute ROM integrity tests from Runtime Execute FIPS Self Test in Runtime Aug 29, 2023
builder/src/lib.rs Show resolved Hide resolved
kat/src/lib.rs Show resolved Hide resolved
runtime/src/fips.rs Outdated Show resolved Hide resolved
Comment on lines +237 to +279
if let SelfTestStatus::InProgress(execute) = drivers.self_test_status {
if drivers.mbox.lock() == false {
match execute(drivers) {
Ok(_) => drivers.self_test_status = SelfTestStatus::Done,
Err(e) => caliptra_drivers::report_fw_error_non_fatal(e.into()),
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a specific reason SelfTest can't be blocking?

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 need to “reply with data” to conform with new FIPS API (response contains header + checksum), but the mailbox SRAM would already be mutated with the image.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The alternative approach is to make the image-verification digest generic, using the sha384 peripheral instead of sha384acc with the mailbox.

If we want to abandon calling back into ROM forever, perhaps just using the sha384 peripheral is better...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep this run-self-test mailbox API as is. If a future version of FIPS ever needs us to test the sha384acc peripheral with something other than empty input data we'll need a mechanism to modify the mailbox.

runtime/src/lib.rs Show resolved Hide resolved
@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch 3 times, most recently from 8bd794f to 6734392 Compare August 29, 2023 22:28
@rusty1968 rusty1968 marked this pull request as ready for review August 29, 2023 22:43
korran added a commit to korran/caliptra-sw that referenced this pull request Aug 30, 2023
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
korran added a commit that referenced this pull request Aug 30, 2023
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
FerralCoder
FerralCoder previously approved these changes Aug 31, 2023
korran added a commit to korran/caliptra-sw that referenced this pull request Sep 5, 2023
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
error/src/lib.rs Outdated Show resolved Hide resolved
.mailbox_execute(u32::from(CommandId::SELF_TEST), payload.as_bytes())
.unwrap_err();

hw.step_until_boot_status(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is realistic to expect callers to do this. Also, I'm not sure the boot status register is really intended to be used once we've fully booted.

Perhaps the mailbox response from the SELF_TEST command should explain to the user that the test is currently in progress.

Maybe it would be simpler if this were two separate commands CommandId::SELF_TEST_START and CommandID:SELF_TEST_GET_RESULT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -256,7 +291,18 @@ fn handle_command(drivers: &mut Drivers) -> CaliptraResult<MboxStatusE> {
#[cfg(feature = "test_only_commands")]
CommandId::TEST_ONLY_HMAC384_VERIFY => HmacVerifyCmd::execute(drivers, cmd_bytes),
CommandId::VERSION => FipsVersionCmd::execute(drivers),
CommandId::SELF_TEST => FipsSelfTestCmd::execute(drivers),
#[cfg(feature = "fips_self_test")]
CommandId::SELF_TEST => match drivers.self_test_status {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you should overload the SELF_TEST command for the "start" and "get result" phases. The response in both cases is an empty success result, which isn't very informative. See my comments in smoke_test.rs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

#[cfg(feature = "fips_self_test")]
CommandId::SELF_TEST => match drivers.self_test_status {
SelfTestStatus::Idle => {
drivers.self_test_status = SelfTestStatus::InProgress(fips_self_test_cmd::execute);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this need to take a function? Or could the handler code in goto_idle just call fips_self_test_cmd::execute directly? I feel like the current code is a bit hard to follow.

runtime/src/lib.rs Outdated Show resolved Hide resolved
rusty1968 pushed a commit that referenced this pull request Sep 7, 2023
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch from 8c49288 to dbd83b8 Compare September 7, 2023 17:39
rusty1968 pushed a commit that referenced this pull request Sep 7, 2023
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch from dbd83b8 to 1a6b7ff Compare September 7, 2023 17:53
rusty1968 and others added 10 commits September 7, 2023 13:02
When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.
@rusty1968 rusty1968 force-pushed the antrocha/fips-invoke-rom-integrity branch from fe40e89 to bcf5b40 Compare September 7, 2023 21:36
runtime/src/fips.rs Show resolved Hide resolved
@rusty1968
Copy link
Contributor Author

@mhatrevi , ajisaxena : please give feedback on changes in crates owned by msft.

korran added a commit that referenced this pull request Sep 8, 2023
)

* Decouple dependency of Kat execution from Rom

* Decouple verification environment from ROM dependency

* Move fips-self-test test to test crate

* Use conditional compilation to build caliptra-runtime as ROM image

* Postpone self test execution

* Fix 1600 byte ROM size regression in #684

When callers from separate crates call a large generic function like
verify_lms_signature_cfi(), rustc 1.70 may build multiple versions
(depending on optimizer heuristics), even when all the generic
parameters are identical. This is bad, as it can bloat the binary and
the second copy violates the FIPS requirements that the same machine
code be used for the KAT as the actual implementation. To defend against
it, we provide a non-generic function that production firmware should
call instead.

* Rename goto_idle to enter_idle

* Fix typo

* Renamed SELF_TEST

* START/GET_PROGRESS commands

* kats executes before image verification

* Remove persistent_data from venv

* Implement FIPS commands in ROM

VERSION
SELF_TEST
SHUTDOWN

* Fix unassigned error codes

* Address feedback

- Remove mut from fips create_slice
- Use copy_bytes_to_mbox in write_response
- Restore idevid tests mistakenly removed
- Add bounds checking on fmc and rt sizes

* Rom itegrity test call from Runtime.

* Fixes for gate

---------

Co-authored-by: Anthony Rocha <anthony.rocha@amd.com>
Co-authored-by: Kor Nielsen <kor@google.com>
@FerralCoder
Copy link
Contributor

FerralCoder commented Sep 11, 2023

changes were included with PR #693

@rusty1968 rusty1968 deleted the antrocha/fips-invoke-rom-integrity branch October 20, 2024 23:48
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.

5 participants