Test remote attestation in UEFI app #2703
Conversation
|
Running I'm not sure what's going on, ring seems to be compiling fine, but then there's an error when building the app. Ideas @conradgrobler / @andrisaar? |
|
Those undefined symbols ( |
|
Based on it seems these trace back to rings crypto build scripts, eg What I'm a bit surprised by is this being caught only in the bundling step when adding it the UEFI example app. The remote attestation crate (which includes ring) on its own appears to build fine for the UEFI target. |
jul-sh
force-pushed
the
uefi-test-remote-attestation
branch
from
0a9532d
to
f7841d5
Compare
|
I assume it builds because it thinks the symbols would be resolved later during the linking process. As the UEFI app was not yet using the code, the linker did not include it and therefore did not try to resolve all the symbols. Now that it is trying to do so, it cannot resolve it and linking breaks. |
|
It looks like this is a potential problem with nasm: openssl/openssl#12712 I am not sure how the precompiled bits are built to bypass this problem. |
|
Looks like this bit of code is setting the assembler: Lines 225 to 231 in f7841d5 Perhaps UEFI should be split out as a sperate one that does not use nasm. |
|
Worth pointing out that the UEFI patch briansmith/ring#1406 we applied does not include a UEFI test runner for the ring crate itself. The proposed CI integration only checks the the library builds, so wouldn't catch this. |
|
I guess we could also provide a similar stub implementation: https://github.com/tianocore/edk2/blob/7c0ad2c33810ead45b7919f8f8d0e282dae52e71/CryptoPkg/Library/OpensslLib/X64/ApiHooks.c |
For reference, the email thread associated with the issue https://listman.redhat.com/archives/edk2-devel-archive/2020-August/msg00981.html. Looks like the person who opened said issue is associated with edk2, and like they ended up going with the stub |
third_party/ring/stubs.c
Outdated
| Inserted MinGW, stubbed as it unavailable in UEFI. | ||
| Ref: https://metricpanda.com/rival-fortress-update-45-dealing-with-__chkstk-__chkstk_ms-when-cross-compiling-for-windows/ | ||
| **/ | ||
| void * |
There was a problem hiding this comment.
Thinking about this a bit more, I don't think this is the correct signature. I think it should be a naked function (not sure how to do that in c, but in rust it is marked with a [naked] attribute to stop the compiler from emitting a function prologue or epilogue) with no arguments and no return values.
Perhaps something like the following might work:
__attribute__((naked)) void ___chkstk_ms() {
}There was a problem hiding this comment.
thanks, will look into it
There was a problem hiding this comment.
Current status is that with those two stubs (as currently present), it does build. However it panics as soon as the underlying crypto code is called:
[ERROR]: /home/docker/.cargo/registry/src/github.com-1ecc6299db9ec823/uefi-services-0.12.1/src/lib.rs@142: Panic in src/main.rs at (149, 14):
[ERROR]: /home/docker/.cargo/registry/src/github.com-1ecc6299db9ec823/uefi-services-0.12.1/src/lib.rs@149: called `Result::unwrap()` on an `Err` value: Couldn't create signer
ERROR:
ERROR: Caused by:
[ERROR]: /home/docker/.cargo/registry/src/github.com-1ecc6299db9ec823/uefi-services-0.12.1/src/lib.rs@149: Couldn't generate PKCS#8 key pair: Unspecified
A simpler test (just filling a buf with SystemRandom to start with) results in the same panic.
There was a problem hiding this comment.
I think the current stub should be fine, very similar to the stub that go used for a while:
https://android.googlesource.com/platform/external/compiler-rt/+/ccaafe6%5E%21/#F1
ref discussion in golang/go#6305
jul-sh
force-pushed
the
uefi-test-remote-attestation
branch
from
3f21c3f
to
11c7191
Compare
jul-sh
force-pushed
the
uefi-test-remote-attestation
branch
from
11c7191
to
d8111c8
Compare
third_party/ring/stubs.c
Outdated
| Inserted Mby inGW, stubbed as it is unavailable in UEFI. Given that this | ||
| routine is used for very large variable it appears unlikely to ever be | ||
| invoked in deployment. |
There was a problem hiding this comment.
Famous last words; would appreciate extra eyes on this in review. :)
There was a problem hiding this comment.
From what I could see this is only used by the poly1305 code because it puts a lot of data on the stack. My understanding is that it is only inserted as a peformance optimisation on Windows, and should not break anything if a blank stub version is invoked. At worst it might cause slightly reduced performance, so I thinkg this is good for now.
experimental/uefi/app/runner
Outdated
| #!/usr/bin/env bash | ||
|
|
||
| # Thin shell script invoked as a cargo runner to run the compiled efi firmware | ||
| # in QEMU. Detects if kvm is supported, and sets qemu flags based on that. | ||
| # Instead of this single runner script it would be preferable to use a different | ||
| # runner based on whether the kvm feature is set. However, cargo does not | ||
| # currently allow this. Ref: https://github.com/rust-lang/cargo/issues/8170 | ||
|
|
||
| readonly TARGET=$1 | ||
|
|
||
| qemu_flags=( | ||
| '-nodefaults' | ||
| '-nographic' | ||
| '-bios' '/usr/share/OVMF/OVMF_CODE.fd' | ||
| '-serial' 'file:target/console.log' | ||
| '-serial' 'stdio' | ||
| '-machine' 'q35' | ||
| '-device' 'isa-debug-exit,iobase=0xf4,iosize=0x04' | ||
| ) | ||
|
|
||
| # Use kvm if supported, as it is required for certain features (that are gated | ||
| # behind the cargo `kvm` feature flag. Note that systems that support kvm still | ||
| # need to enable the cargo `kvm` feature for this crate in order to include | ||
| # code that requires kvm in the compiled firmware. Ideally this check would | ||
| # check the cargo `kvm` flag itself. However, cargo does not expose this | ||
| # information to the runner. | ||
| # Ref: https://doc.rust-lang.org/cargo/reference/environment-variables.html | ||
| if [[ -e "/dev/kvm" ]]; then | ||
| qemu_flags+=( | ||
| '-enable-kvm' | ||
| '-cpu' 'Broadwell-IBRS' | ||
| ) | ||
| fi | ||
|
|
||
| qemu-system-x86_64 "${qemu_flags[@]}" -kernel "${TARGET}" | ||
|
|
There was a problem hiding this comment.
Not a fan of this solution, but there does not currently seem to be a better way to do this. As long as we want this crate to run in GitHub CI we'll need to support hosts that do not support kvm.
Perhaps once we add a CI runner that supports virtualization we can remove this, only leaving the kvm reliant runner.
…nables all flags for tests
| # Thin shell script invoked as a cargo runner to run the compiled efi firmware | ||
| # in QEMU. Detects if kvm is supported, and sets qemu flags based on that. | ||
| # Instead of this single runner script it would be preferable to use a different | ||
| # runner based on whether the kvm feature is set. However, cargo does not | ||
| # currently allow this. Ref: https://github.com/rust-lang/cargo/issues/8170 |
There was a problem hiding this comment.
Not a fan of this solution, but there does not currently seem to be a better way to do this. As long as we want this crate to run in GitHub CI we'll need to support hosts that do not support kvm.
Perhaps once we add a CI runner that supports virtualization we can remove this, only leaving the kvm reliant runner.
third_party/ring/stubs.c
Outdated
| Inserted Mby inGW, stubbed as it is unavailable in UEFI. Given that this | ||
| routine is used for very large variable it appears unlikely to ever be | ||
| invoked in deployment. |
There was a problem hiding this comment.
From what I could see this is only used by the poly1305 code because it puts a lot of data on the stack. My understanding is that it is only inserted as a peformance optimisation on Windows, and should not break anything if a blank stub version is invoked. At worst it might cause slightly reduced performance, so I thinkg this is good for now.
third_party/ring/stubs.c
Outdated
|
|
||
| /** | ||
| Stub function for win64 routine used for exceedingly large variables. | ||
| Inserted Mby inGW, stubbed as it is unavailable in UEFI. Given that this |
There was a problem hiding this comment.
super nit: I think this is inserted by nasm rather than MinGW.
| // limitations under the License. | ||
| // | ||
|
|
||
| //! Integration Test of Remote Attestation in UEFI |
There was a problem hiding this comment.
nit: This line of the comment should end with a . and seeing that it looks like a heading it should probably have a blank line after it.
| //! Integration Test of Remote Attestation in UEFI | ||
| //! This tests that remote attestion works inside the UEFI app. While the test | ||
| //! code is identical to (a subset of) the tests in the remote attestation crate | ||
| //! they here utilize the qemu runner configured in the UEFI app. This means | ||
| //! that test code actually compiled to a UEFI target, which changes the | ||
| //! underlying implementation of the remote attestation crate. | ||
| //! TODO(#2654): It would be preferable to remove the test here, and instead | ||
| //! run the tests in the oak_remote_attestation crate itself for both standard | ||
| //! and UEFI targets. Due to concerns related to the workspace this is presently | ||
| //! not possible. Ref: https://github.com/project-oak/oak/issues/2654 |
There was a problem hiding this comment.
I agree that it is a good pragmatic choice for now. Once we have an end-to-end example with a connection from a client that exercises this functionality the test could be removed.
|
Reproducibility Index: Reproducibility Index diff: diff --git a/reproducibility_index b/reproducibility_index
index abfb372..f737749 100644
--- a/reproducibility_index
+++ b/reproducibility_index
@@ -1,2 +1,2 @@
-d11f60ea056c550c6e288747b2487a78a8699fcb4a4e6490634cd19997775978 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
-ff6fa85496bb0e9f830f5e3d39851789df01b5a8cbfc04dda485ca1c05a79f46 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
+93dc1dfb8849e2618d4378bac34e03d41abfac2565ac53f0cc7d77e9f2b5f795 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_base
+65af1630e9b7df522becb6f519d6a96492ba78212e1121133cd18b387635a448 ./target/x86_64-unknown-linux-musl/release/oak_functions_loader_unsafe
|
No description provided.