b49020 commented 9 months ago

Existing OP-TEE rust environment required a custom rust toolchain target for OP-TEE based TAs. I suppose back in 2019 when this SDK was created, rust embedded ecosystem (especially no_std support) was in its very early stages of development. But as of today many rust crates have already added support for rust no_std or are being actively worked on to add rust no_std support for example rustls here(https://github.com/rustls/rustls/pull/1399).

This effort is a followup effort to the discussion here (https://github.com/apache/incubator-teaclave-trustzone-sdk/issues/113). The major motivation for this effort was to make OP-TEE rust TAs development environment to be the first class citizen. Rust no_std support seems to provide quite similar environment as we have on the C counterpart side in OP-TEE (we don't support fully fledged libc/glibc but rather our own quite simple libutils library).

Upsides for this PR:

Reusing standard rust aarch64 teir-1 toolchain target (aarch64-unknown-linux-gnu) for TAs development.
Significant rust TAs performance improvements.
Significant rust TAs binary size reduction.
Dropping custom rust toolchain/libc/compiler-builtins support.

Make rust TA builds to be quite similar to rust Linux application builds:

$ cargo build --target $(TARGET) --release --verbose --config $(LINKER_CFG)

Downsides for this PR:

We have to drop networking and serde related TA examples due to their strong reliance on rust std support. But as I mentioned above with no_std support picking up, we should be able to rewrite them.

Testing

Their is one change needed for OP-TEE build repo in order to build this PR as follows. Once there is consensus on this PR, I will submit this change as well.

build$ git diff
diff --git a/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk b/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
index e19e8b5..af2f368 100644
--- a/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
+++ b/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
@@ -12,7 +12,7 @@ endif
 EXAMPLE = $(wildcard examples/*)

 HOST_TARGET := aarch64-unknown-linux-gnu
-TA_TARGET := aarch64-unknown-optee-trustzone
+TA_TARGET := aarch64-unknown-linux-gnu

 export RUST_TARGET_PATH = $(@D)
 export RUST_COMPILER_RT_ROOT = $(RUST_TARGET_PATH)/rust/rust/src/llvm-project/compiler-rt

Once that's done we should be able to build OP-TEE buildroot setup with rust support:

$ make -j`nproc` OPTEE_RUST_ENABLE=y

For interactive run, just bring up Qemu with below command and run rust examples:

$ make run-only

Or you can test all rust examples in one go:

$ make check-only-rust
<snip>
Starting QEMU...
 done, guest is booted.
Test Rust applications:
Running acipher-rs...
Test success
Running aes-rs...
Test success
Running authentication-rs...
Test success
Running big_int-rs...
Test success
Running diffie_hellman-rs...
Test success
Running digest-rs...
Test success
Running hello_world-rs...
Test success
Running hotp-rs...
Test success
Running random-rs...
Test success
Running secure_storage-rs...
Test success
Running supp_plugin-rs...
Test success
Running time-rs...
Test success
Running signature_verification-rs...
Test success
Test Rust application finished

Performance comparisons

After this PR, the TA performance becomes equivalent to the C counterparts. This is impressive improvement as compared to 35% performance gap earlier as illustrated here (https://github.com/apache/incubator-teaclave-trustzone-sdk/issues/89).

See below comparison after this PR:

# time aes-rs 
Prepare encode operation
Load key in TA
Reset ciphering operation in TA (provides the initial vector)
Encode buffer from TA
Prepare decode operation
Load key in TA
Reset ciphering operation in TA (provides the initial vector)
Decode buffer from TA
Clear text and decoded text match
real    0m 0.10s
user    0m 0.00s
sys 0m 0.09s
# 
# time optee_example_aes 
Prepare session with the TA
Prepare encode operation
Load key in TA
Reset ciphering operation in TA (provides the initial vector)
Encode buffer from TA
Prepare decode operation
Load key in TA
Reset ciphering operation in TA (provides the initial vector)
Decode buffer from TA
Clear text and decoded text match
real    0m 0.10s
user    0m 0.00s
sys 0m 0.08s
#
#
# time random-rs 
Invoking TA to generate random UUID...
Invoking done!
Generate random UUID: 60ee720f-493b-45a2-f7413c1bfc3df154
Success
real    0m 0.08s
user    0m 0.00s
sys 0m 0.07s
# 
# time optee_example_random 
Invoking TA to generate random UUID... 
TA generated UUID value = 0x4d45495584fb6fa851a761c3583dc3c
real    0m 0.08s
user    0m 0.00s
sys 0m 0.06s

Size comparisons

As you can observe from the comparisons below, there is approx. 70K - 80K TA binary size reduction after this PR:

$ ls -lh ./per-package/optee_rust_examples_ext/target/lib/optee_armtz/
...
-r--r--r-- 2 sumit sumit 197K Dec 21 17:29 057f4b66-bdab-11eb-96cf-33d6e41cc849.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 0864c8ec-bdab-11eb-8926-c7fa47a8c92d.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 0a5a06b2-bdab-11eb-add0-77f29de31296.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 0bef16a2-bdab-11eb-94be-6f9815f37c21.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 0e6bf4fe-bdab-11eb-9bc5-3f4ecb50aee7.ta
-r--r--r-- 2 sumit sumit 196K Dec 21 17:30 10de87e2-bdab-11eb-b73c-63fec73e597c.ta
-r--r--r-- 2 sumit sumit 196K Dec 21 17:30 133af0ca-bdab-11eb-9130-43bf7873bf67.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 1585d412-bdab-11eb-ba91-3b085fd2601f.ta
-r--r--r-- 2 sumit sumit 196K Dec 21 17:30 197c710c-bdab-11eb-8f3f-17a5f698d23b.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:30 1cd6d392-bdab-11eb-9082-abc902ac5cd4.ta
-r--r--r-- 2 sumit sumit 196K Dec 21 17:31 21b1a1da-bdab-11eb-b614-275a7098826f.ta
-r--r--r-- 2 sumit sumit 262K Dec 21 17:31 255fc838-de89-42d3-9a8e-d044c50fa57c.ta
-r--r--r-- 2 sumit sumit 197K Dec 21 17:31 c7e478c2-89b3-46eb-ac19-571e66c3830d.ta

Before this PR:

$ ls -lh ./per-package/optee_rust_examples_ext/target/lib/optee_armtz/
...
-r--r--r-- 2 sumit.garg primary 272K Dec 19 10:26 057f4b66-bdab-11eb-96cf-33d6e41cc849.ta
-r--r--r-- 2 sumit.garg primary 273K Dec 19 10:26 0864c8ec-bdab-11eb-8926-c7fa47a8c92d.ta
-r--r--r-- 2 sumit.garg primary 273K Dec 19 10:26 0a5a06b2-bdab-11eb-add0-77f29de31296.ta
-r--r--r-- 2 sumit.garg primary 273K Dec 19 10:27 0bef16a2-bdab-11eb-94be-6f9815f37c21.ta
-r--r--r-- 2 sumit.garg primary 272K Dec 19 10:27 0e6bf4fe-bdab-11eb-9bc5-3f4ecb50aee7.ta
-r--r--r-- 2 sumit.garg primary 264K Dec 19 10:27 10de87e2-bdab-11eb-b73c-63fec73e597c.ta
-r--r--r-- 2 sumit.garg primary 264K Dec 19 10:28 133af0ca-bdab-11eb-9130-43bf7873bf67.ta
-r--r--r-- 2 sumit.garg primary 272K Dec 19 10:28 1585d412-bdab-11eb-ba91-3b085fd2601f.ta
...
-r--r--r-- 2 sumit.garg primary 264K Dec 19 10:29 197c710c-bdab-11eb-8f3f-17a5f698d23b.ta
-r--r--r-- 2 sumit.garg primary 268K Dec 19 10:29 1cd6d392-bdab-11eb-9082-abc902ac5cd4.ta
...
-r--r--r-- 2 sumit.garg primary 260K Dec 19 10:32 21b1a1da-bdab-11eb-b614-275a7098826f.ta
-r--r--r-- 2 sumit.garg primary 273K Dec 19 10:31 255fc838-de89-42d3-9a8e-d044c50fa57c.ta
...
-r--r--r-- 2 sumit.garg primary 338K Dec 19 10:31 c7e478c2-89b3-46eb-ac19-571e66c3830d.ta
...

b49020 commented 9 months ago

@DemesneGH @Sword-Destiny @jbech-linaro @jenswi-linaro @jforissier @etienne-lms @daniel-thompson @Ablu Fyi..

Feedback/comments are very much welcome, thanks.

DemesneGH commented 9 months ago

@b49020 Thanks so much for your contributions!

Since the std still fits some cases which relying on serde, network, and other crates that have not support no-std yet, how about merging the PR into the separate branch from master?

BTW please check out the failed CI tasks, thanks!

b49020 commented 9 months ago

Thanks @DemesneGH for your comments.

Since the std still fits some cases which relying on serde, network, and other crates that have not support no-std yet, how about merging the PR into the separate branch from master?

Sure I am open to merging this in separate branch for now but I would like to see optee-utee crate new release with no-std support. That would allow people to write their own TAs using latest rust toolchain support without the need to have this full SDK cloned. I would like to give trusted keys TA rewrite in rust a try so that we have real world TAs written in rust rather than just examples in this SDK.

BTW please check out the failed CI tasks

My bad, I forgot to update linker configuration for host applications. The build didn't show any issue for me since the host applications weren't rebuilt. BTW, the issue should be fixed now, please re-run CI now.

daniel-thompson commented 9 months ago

Rather than create a branch is it possible to introduce a no_std feature to optee-utee crate (similar to crates like serde which support no_std but it is optional). That would allow progressive adoption of no_std without having to fork.

Cargo can handle some quite complex feature combinations, including marking up examples that require std-only crates so you don't need to enable them crate wide.

DemesneGH commented 9 months ago

Rather than create a branch is it possible to introduce a no_std feature to optee-utee crate (similar to crates like serde which support no_std but it is optional). That would allow progressive adoption of no_std without having to fork.

Good point. I agree with you.

DemesneGH commented 9 months ago

@b49020 Seems there are some license issue on CI:

ERROR the following files don't have a valid license header: 
examples/acipher-rs/ta/dyn_list
examples/aes-rs/ta/dyn_list
examples/authentication-rs/ta/dyn_list
examples/big_int-rs/ta/dyn_list
examples/diffie_hellman-rs/ta/dyn_list
examples/digest-rs/ta/dyn_list
examples/hello_world-rs/ta/dyn_list
examples/hotp-rs/ta/dyn_list
examples/random-rs/ta/dyn_list
examples/secure_storage-rs/ta/dyn_list
examples/signature_verification-rs/ta/dyn_list
examples/supp_plugin-rs/ta/dyn_list
examples/time-rs/ta/dyn_list 
ERROR one or more files does not have a valid license header

b49020 commented 9 months ago

Rather than create a branch is it possible to introduce a no_std feature to optee-utee crate (similar to crates like serde which support no_std but it is optional). That would allow progressive adoption of no_std without having to fork.

That sounds fine but for std counterpart I am not sure if the current custom OP-TEE toolchain target (along with libc and compiler builtin forks) can support the latest rust nightly toolchain. Currently its based on nightly-2021-09-20. But we need recent nightly releases for C FFI bindings like core::ffi::c_size_t. @DemesneGH any thoughts on the uprev?

b49020 commented 9 months ago

Seems there are some license issue on CI:

dyn_list files are generated by <optee_os>/ta/link.mk and to be consumed directly by the linker. So I don't think they deserve any license text.

b49020 commented 9 months ago

The current CI failure is due to following missing patch to OP-TEE build repo as I have stated above:

build$ git diff
diff --git a/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk b/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
index e19e8b5..af2f368 100644
--- a/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
+++ b/br-ext/package/optee_rust_examples_ext/optee_rust_examples_ext.mk
@@ -12,7 +12,7 @@ endif
 EXAMPLE = $(wildcard examples/*)

 HOST_TARGET := aarch64-unknown-linux-gnu
-TA_TARGET := aarch64-unknown-optee-trustzone
+TA_TARGET := aarch64-unknown-linux-gnu

 export RUST_TARGET_PATH = $(@D)
 export RUST_COMPILER_RT_ROOT = $(RUST_TARGET_PATH)/rust/rust/src/llvm-project/compiler-rt

b49020 commented 9 months ago

Thanks @Ablu for your review.

It looks like there is interest to optimize for size. Since you are using nightly anyway, you may want to try:

Binary size reduction is not such a hard requirement here. However, from security point of view its better to reduce code footprint which this no_std adoption would provide us. Also, I would like to keep nightly dependencies to a minimal since we should aim to migrate to a stable toochain build instead.

DemesneGH commented 9 months ago

That sounds fine but for std counterpart I am not sure if the current custom OP-TEE toolchain target (along with libc and compiler builtin forks) can support the latest rust nightly toolchain. Currently its based on nightly-2021-09-20. But we need recent nightly releases for C FFI bindings like core::ffi::c_size_t. @DemesneGH any thoughts on the uprev?

@b49020 I see. I think after upstreaming the aarch64-unknown-optee-trustzone target (as discussed at https://github.com/apache/incubator-teaclave-trustzone-sdk/issues/113) we can support the latest toolchain and then proceed to merge the no-std as the feature. Since that needs more effort, I'd prefer the separate no-std branch for now.

b49020 commented 9 months ago

@DemesneGH How about we rather stick the std based TAs to v0.2.0 of optee-utee crate and merge no-std feature in a later version of optee-utee crate using latest rust toolchain? Although I am still uncertain about stability (reasons already provided here) of aarch64-unknown-optee-trustzone target but we shouldn't gate OP-TEE rust development for its availability upstream. BTW, we can very well uprev std TAs once we have that upstream support available.

This will atleast allow the adoption of no-std TAs with migration option for other std TAs as well. Also, as I mentioned earlier we would like to provide OP-TEE community with standardized rust environment to develop standalone TAs.

b49020 commented 9 months ago

The current CI failure is due to following missing patch to OP-TEE build repo as I have stated above:

The patch has been posted here: https://github.com/OP-TEE/build/pull/714

DemesneGH commented 9 months ago

@b49020 do you need a new release on https://crates.io/crates/optee-utee?

b49020 commented 9 months ago

@daniel-thompson @DemesneGH So I gave no_std being an optional feature a try for optee-utee and optee-utee-sys crates, it turns out that I found a way to keep the std TAs alongside no_std TAs. I did managed the older rust toolchain pinning for std TAs via following in the Makefile:

    rustup override set nightly-2021-09-20
    @xargo build --target $(TARGET) --release --verbose -Z unstable-options --config $(LINKER_CFG)
    rustup override unset

I hope this is something we can agree upon, feedback/comments are very much welcome.

b49020 commented 9 months ago

do you need a new release on https://crates.io/crates/optee-utee?

Yeah once we have sufficient functionality implemented to enable standalone rust TAs development. I suppose we still lack inter TA communication rust library APIs, right?

b49020 commented 9 months ago

@DemesneGH The CI reports No space left on device, are there any space constraints for CI? BTW, I don't think this PR adds anything extra apart from extra latest nightly toolchain installation.

DemesneGH commented 9 months ago

The CI reports No space left on device, are there any space constraints for CI?

The Github actions provide at least 14G storage (reference: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources). It would be more than 14GB sometimes (https://github.com/actions/runner-images/issues/2840#issuecomment-791177163) since the error haven't occurred before.

There are some workaround on https://github.com/actions/runner-images/issues/2840#issuecomment-790492173 but I'd prefer to remove the dependencies of aarch64-unknown-optee-trustzone target (large clones in setup.sh).

I'm trying to summarize the conclusions we reached and the steps for breaking down, please correct me if I've misunderstood:

STAGE 1:

keep the no-std a separate branch from master.
publish the optee-utee crate on crates.io. The optee-utee crate is no-std by default, corresponding to no-std branch on repo. Users can import the crate using cargo add optee-utee.
When users need the std , they should clone the SDK repo of master branch and setup the environment.

comments:

for no-std branch:
1. Recommend removing all aarch64-unknown-optee-trustzonetarget related files such as aarch64-unknown-optee-trustzone.json, arm-unknown-optee-trustzone.json, environment, and std-only examples. It makes things clear and also helps us to figure out the difference when we trying to merge the no-std branch at STAGE 2. (BTW it's also the workaround for the no space left issue above)
for publishing optee-utee crates:
1. todo: support for inter-TA APIs
2. Aiming that users use the optee-utee crate with just cargo add, we may need a build.rs inside the optee-utee crate which download toolchains, optee c source code and compile c libs.

STAGE 2 (after the STAGE 1 is finished):

for master branch:
1. merge the new released optee-utee crate (enable the std feature)
2. proceed aarch64-unknown-optee-trustzone uprev
3. consider merging the no-std into master

b49020 commented 9 months ago

@DemesneGH I mostly agree with your stage 1 but for stage 2 I think once we have published optee-utee crate on crates.io then we can just drop optee-utee from master branch and instead just use:

optee-utee = { version = "x.x.x", features = ["std"] }

This would avoid any divergence and allow the std counterpart to be regularly tested with latest optee-utee changes too. I think we should follow the same approach for optee-teec as well where no-std acts as the main development branch and releases are synced to crates.io. OTOH, if we rather have std as a separate branch and keep master as the main development branch then it would be even better.

If you agree then please feel free to create a separate branch (std or no-std).

b49020 commented 9 months ago

if we rather have std as a separate branch and keep master as the main development branch then it would be even better.

So once we have the aarch64-unknown-optee-trustzone upstreamed then it would just be a simple move to migrate std examples from std branch to master branch.

DemesneGH commented 9 months ago

if we rather have std as a separate branch and keep master as the main development branch then it would be even better

Sure, after we finished the stage 1 which means the no-std is stable I'm okay to set the no-std as main branch.

b49020 commented 9 months ago

Sure, after we finished the stage 1 which means the no-std is stable I'm okay to set the no-std as main branch.

Fair enough, let's go with no-std branch then. Let me know once it's created, I will create a PR for that.

DemesneGH commented 9 months ago

https://github.com/apache/incubator-teaclave-trustzone-sdk/tree/no-std, thanks! @b49020

b49020 commented 9 months ago

@DemesneGH Created PR for no-std here: https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/115

b49020 commented 9 months ago

@DemesneGH With no-std changes merged, the next step is to publish optee-utee crate.

todo: support for inter-TA APIs

Although I agree it is an important feature which I will target next but I still think we can have v0.3.0 published for optee-utee crate without it to allow developers start playing with standalone rust TAs. We can target this feature for next v0.4.0 release.

Aiming that users use the optee-utee crate with just cargo add, we may need a build.rs inside the optee-utee crate which download toolchains, optee c source code and compile c libs.

I don't think we should bundle a script to build OP-TEE OS since it can be configured in many ways for different platforms. We should rather depend on TA_DEV_KIT_DIR environment variable to provide path to pre-build OP-TEE utee libraries. This is quite similar to C development environment. I will make corresponding changes to optee-utee crate regarding this.

b49020 commented 9 months ago

We should rather depend on TA_DEV_KIT_DIR environment variable to provide path to pre-build OP-TEE utee libraries. This is quite similar to C development environment. I will make corresponding changes to optee-utee crate regarding this.

Fyi.. https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/116

jforissier commented 9 months ago

The CI reports No space left on device, are there any space constraints for CI?

The Github actions provide at least 14G storage (reference: https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources). It would be more than 14GB sometimes (actions/runner-images#2840 (comment)) since the error haven't occurred before.

There are some workaround on actions/runner-images#2840 (comment) but I'd prefer to remove the dependencies of aarch64-unknown-optee-trustzone target (large clones in setup.sh).

This might help: https://github.com/OP-TEE/optee_os/commit/788069fa88ed8bf376a07991196fbf80c36b7e06 (although minimizing the cloned data is certainly a good idea regardless).

b49020 commented 9 months ago

This might help: https://github.com/OP-TEE/optee_os/commit/788069fa88ed8bf376a07991196fbf80c36b7e06 (although minimizing the cloned data is certainly a good idea regardless).

With https://github.com/OP-TEE/manifest/pull/260, the rust build would be significantly lighter for OP-TEE CI. Given that I am also thinking that we should probably get rid of OPTEE_RUST_ENABLE flag and enable OP-TEE rust examples in the default build, thoughts?

jforissier commented 9 months ago

This might help: OP-TEE/optee_os@788069f (although minimizing the cloned data is certainly a good idea regardless).

With OP-TEE/manifest#260, the rust build would be significantly lighter for OP-TEE CI. Given that I am also thinking that we should probably get rid of OPTEE_RUST_ENABLE flag and enable OP-TEE rust examples in the default build, thoughts?

Perhaps just make it ?= y by default, but it is always good to have a way to disable things in case they break. This also raises the question of make check (C apps) vs make check-rust. Should make check run both? (it makes sense if make builds both). But then we may want to be able to run only the C apps perhaps (make check-c?). TBD.

b49020 commented 9 months ago

Perhaps just make it ?= y by default, but it is always good to have a way to disable things in case they break.

That sounds reasonable. I will do that as a next step to current rust examples build refactoring.

This also raises the question of make check (C apps) vs make check-rust. Should make check run both? (it makes sense if make builds both). But then we may want to be able to run only the C apps perhaps (make check-c?). TBD.

How about if we rather add rust.exp to build repo and have the check tests conditional under CHECK_TESTS similar to what we did for trusted keys tests?

jforissier commented 9 months ago

Perhaps just make it ?= y by default, but it is always good to have a way to disable things in case they break.

That sounds reasonable. I will do that as a next step to current rust examples build refactoring.

OK thanks.

This also raises the question of make check (C apps) vs make check-rust. Should make check run both? (it makes sense if make builds both). But then we may want to be able to run only the C apps perhaps (make check-c?). TBD.

How about if we rather add rust.exp to build repo and have the check tests conditional under CHECK_TESTS similar to what we did for trusted keys tests?

Yes, much better :+1:

DemesneGH commented 4 months ago

Hi @b49020 Could you review the updated README which added the no-std description? https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/128

Thanks!

apache / incubator-teaclave-trustzone-sdk

RFC: An effort to standardize OP-TEE rust based TAs development environment #114

Upsides for this PR:

Downsides for this PR:

Testing

Performance comparisons

Size comparisons