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

[RISCV] Added TPM DRoT support with Tychools verification #96

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

alex-douk
Copy link
Collaborator

Summary

Tyche can now share with the application (and by extent, Tychools) an attestation report dynamically generated by a platform TPM. A modifiedOpenSBI version is responsible for the DRTM operations, and passes the report to Tyche through the manifest. We require an augmented version of QEMU to be able to leverage this feature.

OpenSBI changes and QEMU changes are documented in the tyche-stage1-opensbi repo. OpenSBI changes are on branch alex-rot, and QEMU changes are on branch qemu_modified_files. The QEMU branch only contains the modified files from QEMU 8.1.

Tyche changes

We modified the following :

  • crates/attestation/src/signature.rs: We added fields to capture the TPM-related information in the EnclaveReport data structure. These fields are : RSA-3072 modulus (384 bytes), RSA-3072 signature (384 bytes), and the TPM attestation per se (129 bytes).
  • monitor/tyche/src/calls.rs : Added a ENCLAVE_ATTESTATION_SIZE call. This was added because the TPM attestation parameters (and therefore size) can be reconfigured.
  • monitor/tyche/src/riscv/riscv_tpm_attestation.rs : New file that contains the ENCLAVE_ATTESTATION Tyche call handler. As the TPM attestation can change, we thought it best to abstract away how it is parsed and passed to the application.
  • monitor/tyche/src/riscv/guest.rs : See above. We also added a handler for the ENCLAVE_ATTESTATION_SIZE that just passes the size of the augmented EnclaveReport struct.
  • monitor/tyche/src/attestation_domain.rs : Added the TPM-related fields in the enclave report creation.

The default behavior in case of absence of a TPM on the platform is that the TPM-related fields are still passed to the application, but are all initialized to 0.

New example

In order to demonstrate the above capabilities, we added an example, based on simple-enclave. We named this new example application rot_enclave, implemented in example/enclave_rot_riscv. The core application did not change. We modified the number of calls to Tyche (with the ENCLAVE_ATTESTATION hypercall) to match the size of the augmented report, and matched the data written to Tychools.

There is a sanity check using the ENCLAVE_ATTESTATION_SIZE hypercall, so the example is bound to the current configuration of OpenSBI's TPM parameters.

Tychools changes

We added in crates/tychools/src/attestation.rs a way to read from the application output file the entire augmented report. We also added a hardcoded PCR value that the report should be matched against. In order to verify the attestation signature, we had to import the ring crate. The Tychools changes are also RISC-V specific (wrapped in a block checking for the riscv_enabled flag). In case of absence of a TPM on the platform, the verification fails as the hardcoded PCR value and the attestation PCR value mismatch.

Note that currently, on its own, simple-enclave is broken and the message is not verified, and as such, our example follows the same pattern.

Note that the hardcoded PCR value has to be changed everytime a change to Tyche is made for the PCR digest check to pass. Signature verification will still go through.

@alex-douk alex-douk requested review from CharlyCst and neeluk7 April 17, 2024 12:26
@neeluk7
Copy link
Collaborator

neeluk7 commented Jun 3, 2024

Hello Alexandre (@alex-douk)
thanks a lot for the work on completing dynamic root of trust end-to-end. ^ ^

I left some comments on specific files, can you please take a look? :)

Further, two things:
Can you please rebase on main again, there are only 2 new commits, so it should be smooth and then the PR can be ready for merging.
Can you also check that you execute the "just format" and "just check" commands successfully?

(I am not sure what the QEMU test here is - the one that's failing, maybe @CharlyCst can provide more insights.)

Once these are all resolved, I believe it's ready for merging. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this one hasn't been modified since being fetched from simple enclave. ^ ^
Maybe it could be edited to replace the simple enclave explanation with rot enclave explanation and same for the sample output too?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add the sample output for when TPM fields are set to zero?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We generally don't include the binaries and they are rebuilt locally by everyone so we may not end up facing any environment configuration/portability issues ^ ^
Could we please remove these? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Line 28: can we restore the path as "qemu/" so that we have the general build command?
Line 154: Similarly, the bear command, can we restore the original one and you can add a comment of what problems you observed with the bear setup and put the existing line 154 in the comment as which command to use instead when bear doesn't work as expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the TPM device option to QEMU, can we define a tpm-dev (similar to dev-riscv}?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I also see some lines removed during the testing. Is that intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just put a todo here before line 128 to specify the impl. using registers to retrieve the report and that ideally we would do it with 1 call via some shared memory? (Or if this description already exists in the monitor or somewhere else, can you please add a reference to that?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the enclave_attestation_size call is only used by the enclave_rot_example, it doesn't have the support to be called using the tyche driver (so basically it's not exposed as part of the API through the driver).
Can we add a note here to specify its limited purpose and implementation to avoid confusion?

CharlyCst and others added 4 commits June 5, 2024 11:30
Our RC frame were never de-allocated, causing memory leaks.
With this change I run the creation micro-benchmark with 20000 enclaves
without running out of memory.
@CharlyCst
Copy link
Collaborator

The test that fails is due to our server not responding, it needs to be fixed when someone gets some free time.
We can't run x86 QEMU on Github CI, so we need to use one of our own machines.

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