Skip to content

Conversation

@nhynes
Copy link
Contributor

@nhynes nhynes commented Jun 15, 2022

For me, this is to allow rustup target add x86_64-fortanix-unknown-sgx before building, as I cannot have a rust-toolchain.toml auto-install it because actions-rs does not support it.

@nhynes nhynes requested review from kostko, pro-wh and ptrus as code owners June 15, 2022 03:42
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #993 (b4f3349) into main (0f74237) will decrease coverage by 41.70%.
The diff coverage is n/a.

❗ Current head b4f3349 differs from pull request most recent head 27eefd0. Consider uploading reports for the commit 27eefd0 to get more accurate results

@@             Coverage Diff             @@
##             main     #993       +/-   ##
===========================================
- Coverage   67.94%   26.23%   -41.71%     
===========================================
  Files         127       24      -103     
  Lines       10304     1635     -8669     
===========================================
- Hits         7001      429     -6572     
+ Misses       3278     1181     -2097     
  Partials       25       25               
Impacted Files Coverage Δ
client-sdk/go/types/transaction.go 53.93% <0.00%> (ø)
runtime-sdk/modules/evm/src/raw_tx.rs
contract-sdk/src/testing.rs
runtime-sdk/modules/evm/src/backend.rs
contract-sdk/types/src/address.rs
runtime-sdk/modules/contracts/src/abi/mod.rs
contract-sdk/storage/src/cell.rs
runtime-sdk/src/modules/rewards/mod.rs
runtime-sdk/src/types/token.rs
runtime-sdk/src/testing/mock.rs
... and 94 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f74237...27eefd0. Read the comment docs.

@nhynes nhynes force-pushed the nhynes/config-hash-rush branch 2 times, most recently from 1e3913a to b4f3349 Compare June 15, 2022 06:24
TARGET="x86_64-fortanix-unknown-sgx"
export CARGO_TARGET_DIR="$CARGO_TARGET_ROOT/sgx"
export CFLAGS_x86_64_fortanix_unknown_sgx="-isystem/usr/include/x86_64-linux-gnu -mlvi-hardening -mllvm -x86-experimental-lvi-inline-asm-hardening"
export CFLAGS_x86_64_fortanix_unknown_sgx="-isystem/usr/include/x86_64-linux-gnu -mlvi-hardening -mllvm -x86-experimental-lvi-inline-asm-hardening -fstack-protector-all"
Copy link
Member

Choose a reason for hiding this comment

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

Why the -fstack-protector-all, this will not work because the Fortanix ABI currently sets FS and GS bases to the same value and the stack protector assumes the canary value is at FS:0x28 but with the Fortanix ABI that contains a copy of RSP. This causes stuff to crash due to canary mismatch.

Copy link
Contributor Author

@nhynes nhynes Jun 15, 2022

Choose a reason for hiding this comment

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

I just want to get something to insert the __stack_check_guard. I believe that it's inserted into the code generated by ring (it would be ring), and isn't defined by LLVM during the top-level build likely because the global is the old way of doing things.

I put in oasisprotocol/sapphire-paratime@866b1fd#diff-8383ec3e7666578793741ef1253328c1dc4bd0e3012176816493eb57e6d81773R10-R15, but I'm not sure if that works now. Have you seen this error before?

  = note: rust-lld: error: undefined symbol: __stack_chk_guard
          >>> referenced by curve25519.c:476 (crypto/curve25519/curve25519.c:476)
          >>>               curve25519.o:(GFp_x25519_ge_frombytes_vartime) in archive /tmp/rustc0BgQWN/libring-34fe96515ca24ea8.rlib
          >>> referenced by curve25519.c:0 (crypto/curve25519/curve25519.c:0)
          >>>               curve25519.o:(GFp_x25519_ge_frombytes_vartime) in archive /tmp/rustc0BgQWN/libring-34fe96515ca24ea8.rlib
          >>> referenced by curve25519.c:782 (crypto/curve25519/curve25519.c:782)
          >>>               curve25519.o:(GFp_x25519_ge_scalarmult_base) in archive /tmp/rustc0BgQWN/libring-34fe96515ca24ea8.rlib
          >>> referenced 14 more times

Maybe what I need is -mstack-protector-guard=global, but then that would require-fstack-protector, which is not permitted? Hm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Regular ring doesn't work with SGX and the upstream author explicitly refuses to merge support for it despite other people implementing everything, for (IMO) stupid reasons. This is why we also got rid of ring everywhere. Why do you need ring anyway?

Copy link
Contributor Author

@nhynes nhynes Jun 17, 2022

Choose a reason for hiding this comment

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

I'm using a ring fork that I've confirmed works. It's not used by the runtime, but it's used by a workspace dependency. I didn't want to use a workspace, but basically all of the GH tooling requires it.

@nhynes nhynes force-pushed the nhynes/config-hash-rush branch from b9d56dd to 0b2d4ff Compare June 15, 2022 06:34
@nhynes nhynes requested a review from kostko June 15, 2022 08:25
@kostko
Copy link
Member

kostko commented Jun 15, 2022

For me, this is to allow rustup target add x86_64-fortanix-unknown-sgx before building, as I cannot have a rust-toolchain.toml auto-install it because actions-rs/toolchain#209.

But the build happens in a Docker container so whatever actions do shouldn't matter? The Docker image specifically uses the same Rust toolchain version as the SDK. We are planning on bumping that version btw.

@nhynes nhynes force-pushed the nhynes/config-hash-rush branch 2 times, most recently from 39f3cc3 to 0b2d4ff Compare June 15, 2022 13:32
@nhynes
Copy link
Contributor Author

nhynes commented Jun 17, 2022

But the build happens in a Docker container so whatever actions do shouldn't matter?

The Rust action is in the sapphire-paratime repo and uses the rust-toolchain file found in the same. That rust-toolchain is also used within the docker container, but it doesn't install the sgx target because it's not a rust-toolchain.toml. Thus, I need to install the latter manually. I'm not using the same rust version as the SDK because I like new features.

Generally, it'd be nice to be able to install packages 'n stuff into the container for paratimes that want to benefit from it.

@nhynes nhynes force-pushed the nhynes/config-hash-rush branch from 0b2d4ff to 27eefd0 Compare June 17, 2022 13:14
@nhynes nhynes merged commit 650c887 into main Jun 17, 2022
@nhynes nhynes deleted the nhynes/config-hash-rush branch June 17, 2022 16:03
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