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

Enable LVI hardening for x86_64-fortanix-unknown-sgx #72655

Merged
merged 1 commit into from Jun 8, 2020

Conversation

jethrogb
Copy link
Contributor

@jethrogb jethrogb commented May 27, 2020

This implements mitigations for the Load Value Injection vulnerability (CVE-2020-0551) for the x86_64-fortanix-unknown-sgx target by enabling new LLVM passes. More information about LVI and mitigations may be found at https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection.

This PR unconditionally enables the mitigations for x86_64-fortanix-unknown-sgx since there is no available hardware that doesn't require the mitigations. This may be reconsidered in the future.

@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

⚠️ Warning ⚠️

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2020
@jethrogb
Copy link
Contributor Author

cc @raoulstrackx

Comment on lines +46 to +50
AR_x86_64_fortanix_unknown_sgx=ar \
CC_x86_64_fortanix_unknown_sgx=x86_64-fortanix-unknown-sgx-clang-11 \
CFLAGS_x86_64_fortanix_unknown_sgx="-mlvi-hardening -mllvm -x86-experimental-lvi-inline-asm-hardening" \
CXX_x86_64_fortanix_unknown_sgx=x86_64-fortanix-unknown-sgx-clang++-11 \
CXXFLAGS_x86_64_fortanix_unknown_sgx="-mlvi-hardening -mllvm -x86-experimental-lvi-inline-asm-hardening" \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole block is moved from below so the variables are available for the target-specific build scripts as well. These are the only changed (added) lines

@@ -37,7 +37,9 @@ use crate::{Build, GitRepo};
// try to infer the archiver path from the C compiler path.
// In the future this logic should be replaced by calling into the `cc` crate.
fn cc2ar(cc: &Path, target: &str) -> Option<PathBuf> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the default that this falls back to (without checking for existence) is good. But I don't want to change it because it might break things.

# We pass the commit id of the port of LLVM's libunwind to the build script.
# Any update to the commit id here, should cause the container image to be re-built from this point on.
RUN /tmp/build-x86_64-fortanix-unknown-sgx-toolchain.sh "5125c169b30837208a842f85f7ae44a83533bd0e"
RUN /tmp/build-x86_64-fortanix-unknown-sgx-toolchain.sh "800f95131fe6acd20b96b6f4723ca3c820f3d379"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jethrogb
Copy link
Contributor Author

It's not clear to me that the tools CI failure is related to this PR?

@jethrogb
Copy link
Contributor Author

@matthiaskrgr confirms on Discord:

it is unrelated to the llvm pr

@jethrogb
Copy link
Contributor Author

jethrogb commented Jun 2, 2020

This is fixing a known public security vulnerability, can this please be reviewed?

@kennytm
Copy link
Member

kennytm commented Jun 2, 2020

r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned kennytm Jun 2, 2020
@petrochenkov
Copy link
Contributor

I'm going to rubber stamp the SGX changes since they are target-specific and you are the maintainer, and all other changes look reasonable.
@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2020

📌 Commit d3c41f4d4595feabf0a1d89b37c993e91fd014d3 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 2, 2020
@bors
Copy link
Contributor

bors commented Jun 4, 2020

⌛ Testing commit d3c41f4d4595feabf0a1d89b37c993e91fd014d3 with merge 47ae8ce37c58482460bca3b853e99d69ccf36d22...

@bors
Copy link
Contributor

bors commented Jun 4, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 4, 2020
@jethrogb
Copy link
Contributor Author

jethrogb commented Jun 4, 2020

[TIMING] ToolBuild { compiler: Compiler { stage: 0, host: "x86_64-apple-darwin" }, target: "x86_64-apple-darwin", tool: "rustbook", path: "src/tools/rustbook", mode: ToolBootstrap, is_optional_tool: false, source_type: InTree, extra_features: [] } -- 399.149
Rustbook (x86_64-apple-darwin) - unstable-book


command did not execute successfully: "/Users/runner/runners/2.169.1/work/1/s/build/x86_64-apple-darwin/stage0-tools-bin/rustbook" "build" "/Users/runner/runners/2.169.1/work/1/s/build/x86_64-apple-darwin/md-doc/unstable-book" "-d" "/Users/runner/runners/2.169.1/work/1/s/build/x86_64-apple-darwin/doc/unstable-book"
expected success, got: signal: 9

No idea what's wrong here but seems unrelated to this PR?

@petrochenkov
Copy link
Contributor

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2020
@bors
Copy link
Contributor

bors commented Jun 7, 2020

☔ The latest upstream changes (presumably #73072) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 7, 2020
@jethrogb
Copy link
Contributor Author

jethrogb commented Jun 7, 2020

Rebased. This security fix has been in the build queue for 5 days. Could you please assign priority?

@kennytm
Copy link
Member

kennytm commented Jun 7, 2020

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Jun 7, 2020

📌 Commit ea48f2e has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 7, 2020
@pietroalbini
Copy link
Member

@bors p=1

@bors
Copy link
Contributor

bors commented Jun 8, 2020

⌛ Testing commit ea48f2e with merge 99bb1948c80f16efdc2ccc0e6da70a2181ddac40...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Contributor

bors commented Jun 8, 2020

⌛ Testing commit ea48f2e with merge fd4b177...

@bors
Copy link
Contributor

bors commented Jun 8, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing fd4b177 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 8, 2020
@bors bors merged commit fd4b177 into rust-lang:master Jun 8, 2020
@workingjubilee workingjubilee added the O-SGX Target: SGX label Jul 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. O-SGX Target: SGX S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants