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

add codegen option for using LLVM stack smash protection #84197

Merged
merged 1 commit into from
Nov 23, 2021

Conversation

bbjornse
Copy link
Contributor

LLVM has built-in heuristics for adding stack canaries to functions. These
heuristics can be selected with LLVM function attributes. This PR adds a codegen
option -C stack-protector={basic,strong,all} which controls the use of these
attributes. This gives rustc the same stack smash protection support as clang
offers through options -fstack-protector, -fstack-protector-strong, and
-fstack-protector-all. The protection this can offer is demonstrated in
test/ui/abi/stack-protector.rs. This fills a gap in the current list of rustc
exploit mitigations (https://doc.rust-lang.org/rustc/exploit-mitigations.html),
originally discussed in #15179.

Stack smash protection adds runtime overhead and is therefore still off by
default, but now users have the option to trade performance for security as they
see fit. An example use case is adding Rust code in an existing C/C++ code base
compiled with stack smash protection. Without the ability to add stack smash
protection to the Rust code, the code base artifacts could be exploitable in
ways not possible if the code base remained pure C/C++.

Stack smash protection support is present in LLVM for almost all the current
tier 1/tier 2 targets: see
test/assembly/stack-protector/stack-protector-target-support.rs. The one
exception is nvptx64-nvidia-cuda. This PR follows clang's example, and adds a
warning message printed if stack smash protection is used with this target (see
test/ui/stack-protector/warn-stack-protector-unsupported.rs). Support for tier 3
targets has not been checked.

Since the heuristics are applied at the LLVM level, the heuristics are expected
to add stack smash protection to a fraction of functions comparable to C/C++.
Some experiments demonstrating how Rust code is affected by the different
heuristics can be found in
test/assembly/stack-protector/stack-protector-heuristics-effect.rs. There is
potential for better heuristics using Rust-specific safety information. For
example it might be reasonable to skip stack smash protection in functions which
transitively only use safe Rust code, or which uses only a subset of functions
the user declares safe (such as anything under std.*). Such alternative
heuristics could be added at a later point.

LLVM also offers a "safestack" sanitizer as an alternative way to guard against
stack smashing (see #26612). This could possibly also be included as a
stack-protection heuristic. An alternative is to add it as a sanitizer (#39699).
This is what clang does: safestack is exposed with option
-fsanitize=safe-stack.

The options are only supported by the LLVM backend, but as with other codegen
options it is visible in the main codegen option help menu. The heuristic names
"basic", "strong", and "all" are hopefully sufficiently generic to be usable in
other backends as well.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 14, 2021
@rust-log-analyzer

This comment has been minimized.

@petrochenkov
Copy link
Contributor

r? @nikic

@rust-highfive rust-highfive assigned nikic and unassigned petrochenkov Apr 14, 2021
@bors
Copy link
Contributor

bors commented Apr 29, 2021

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

@bors
Copy link
Contributor

bors commented May 2, 2021

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

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks pretty reasonable to me. Main feedback is that this should start as an unstable -Z option. @nagisa Is an MCP needed to add an unstable option?

compiler/rustc_codegen_llvm/src/lib.rs Outdated Show resolved Hide resolved
compiler/rustc_session/src/options.rs Outdated Show resolved Hide resolved
@@ -899,6 +899,54 @@ supported_targets! {
("aarch64_be-unknown-linux-gnu_ilp32", aarch64_be_unknown_linux_gnu_ilp32),
}

/// Controls use of stack canaries.
#[derive(Clone, Copy, Debug, PartialEq, Hash, Eq)]
pub enum StackProtector {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is the right place to put this, as it's not actually used by target definitions. I'm not sure what the right place would be though...

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 looked through the set of dependencies of rustc_session to if I could address this comment, but none of the other crates seem more appropriate. The actual user is rustc_codegen_llvm, but this crate depends on rustc_session and not the other way around. One alternative could perhaps be to set move the definition within rustc_session itself, rationale being that the option is "owned" by the option parser?

I've left the location alone for now though, rather than finding a completely new option location, as the other options defined there are at least somewhat similar. I did move it closer to the others, though, rather than being separated from them by the long supported_targets! list.

@nagisa
Copy link
Member

nagisa commented May 3, 2021

MCP isn't necessary for adding an unstable option, no. During the stabilization a final comment period vote will occur instead.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

Looks good to me. Would you mind squashing all the commits together before this gets merged?

@klensy
Copy link
Contributor

klensy commented May 30, 2021

Default stack protector option is no stack protector, but there no such option as -fno-stack-protector?

@bors
Copy link
Contributor

bors commented May 31, 2021

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

@bbjornse
Copy link
Contributor Author

Default stack protector option is no stack protector, but there no such option as -fno-stack-protector?

Right, there is no stack protection unless requested. This is also the case with gcc and clang, I believe, so -fno-stack-protector is the same as passing no option.

@klensy
Copy link
Contributor

klensy commented May 31, 2021

Right, there is no stack protection unless requested. This is also the case with gcc and clang, I believe, so -fno-stack-protector is the same as passing no option.

I mean, that if that option mimic clang's one (that have -fno-stack-protector), we should have explicit variant as well? Something like -Cstack-protector=no

@bbjornse
Copy link
Contributor Author

I mean, that if that option mimic clang's one (that have -fno-stack-protector), we should have explicit variant as well? Something like -Cstack-protector=no

I see. My ambition was to replicate the functionality clang and gcc provides, more than the set of options itself, and for this the current set of alternatives suffice.

It is possible to add an explicit none alternative as you suggest. Is this desirable? I would personally lean against adding code which accomplishes the same as having no code. Arguments I see in favour of such an option is that some users might prefer being explicit about their choice of avoiding stack protection overhead.

@nagisa
Copy link
Member

nagisa commented May 31, 2021

I think the concern is more about the consistency with the other options which tend to have a way to enable the default behaviour and the ability to override the options set by other tooling (possibly in the future).

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Jun 1, 2021

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

@bbjornse
Copy link
Contributor Author

bbjornse commented Jun 4, 2021

I think I see your point @klensy and @nagisa: although the usefulness of an explicit none option is hypothetical, there is value in ruling out hypothetical problems. The presence of such an option may also be expected by users.

I added this capability in the most recent commit. (The patch is larger than strictly necessary as the none option could have been represented with Option::None, but I think one enum variant for each option makes more sense.)

@bors
Copy link
Contributor

bors commented Jun 5, 2021

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

@bors
Copy link
Contributor

bors commented Oct 27, 2021

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

@JohnCSimon
Copy link
Member

Ping from triage:
@bbjornse please address the broken build and
adjust the label with @rustbot ready when you're ready for a review

@bbjornse
Copy link
Contributor Author

@rustbot ready

I still haven't been able to test the fix, but ready for a review of the strategy and compiletest patch in the latest commit.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 15, 2021
@bors
Copy link
Contributor

bors commented Nov 18, 2021

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

@nikic
Copy link
Contributor

nikic commented Nov 21, 2021

I feel like option 3 ends up testing LLVM implementation details too much -- the only thing that really matters to us here is that the attributes get passed through to LLVM properly and it acts based on them in some way, but I don't think it's important to test LLVM's precise behavior on all platforms. LLVM should have their own tests for that. Especially if it needs new compiletest infrastructure, I think this is more hassle than it's worth. I'd skip darwin for this test.

The stack protector heuristics used by LLVM don't really make sense for rust in any case -- e.g. this one is supposed to exclude non-char arrays in structs, but because rust wraps everything in structs it also ends up applying to non-struct arrays.

LLVM has built-in heuristics for adding stack canaries to functions. These
heuristics can be selected with LLVM function attributes. This patch adds a
rustc option `-Z stack-protector={none,basic,strong,all}` which controls the use
of these attributes. This gives rustc the same stack smash protection support as
clang offers through options `-fno-stack-protector`, `-fstack-protector`,
`-fstack-protector-strong`, and `-fstack-protector-all`. The protection this can
offer is demonstrated in test/ui/abi/stack-protector.rs. This fills a gap in the
current list of rustc exploit
mitigations (https://doc.rust-lang.org/rustc/exploit-mitigations.html),
originally discussed in rust-lang#15179.

Stack smash protection adds runtime overhead and is therefore still off by
default, but now users have the option to trade performance for security as they
see fit. An example use case is adding Rust code in an existing C/C++ code base
compiled with stack smash protection. Without the ability to add stack smash
protection to the Rust code, the code base artifacts could be exploitable in
ways not possible if the code base remained pure C/C++.

Stack smash protection support is present in LLVM for almost all the current
tier 1/tier 2 targets: see
test/assembly/stack-protector/stack-protector-target-support.rs. The one
exception is nvptx64-nvidia-cuda. This patch follows clang's example, and adds a
warning message printed if stack smash protection is used with this target (see
test/ui/stack-protector/warn-stack-protector-unsupported.rs). Support for tier 3
targets has not been checked.

Since the heuristics are applied at the LLVM level, the heuristics are expected
to add stack smash protection to a fraction of functions comparable to C/C++.
Some experiments demonstrating how Rust code is affected by the different
heuristics can be found in
test/assembly/stack-protector/stack-protector-heuristics-effect.rs. There is
potential for better heuristics using Rust-specific safety information. For
example it might be reasonable to skip stack smash protection in functions which
transitively only use safe Rust code, or which uses only a subset of functions
the user declares safe (such as anything under `std.*`). Such alternative
heuristics could be added at a later point.

LLVM also offers a "safestack" sanitizer as an alternative way to guard against
stack smashing (see rust-lang#26612). This could possibly also be included as a
stack-protection heuristic. An alternative is to add it as a sanitizer (rust-lang#39699).
This is what clang does: safestack is exposed with option
`-fsanitize=safe-stack`.

The options are only supported by the LLVM backend, but as with other codegen
options it is visible in the main codegen option help menu. The heuristic names
"basic", "strong", and "all" are hopefully sufficiently generic to be usable in
other backends as well.

Reviewed-by: Nikita Popov <nikic@php.net>

Extra commits during review:

- [address-review] make the stack-protector option unstable

- [address-review] reduce detail level of stack-protector option help text

- [address-review] correct grammar in comment

- [address-review] use compiler flag to avoid merging functions in test

- [address-review] specify min LLVM version in fortanix stack-protector test

  Only for Fortanix test, since this target specifically requests the
  `--x86-experimental-lvi-inline-asm-hardening` flag.

- [address-review] specify required LLVM components in stack-protector tests

- move stack protector option enum closer to other similar option enums

- rustc_interface/tests: sort debug option list in tracking hash test

- add an explicit `none` stack-protector option

Revert "set LLVM requirements for all stack protector support test revisions"

This reverts commit a49b74f92a4e7d701d6f6cf63d207a8aff2e0f68.
@bbjornse
Copy link
Contributor Author

I agree that this test is highly detailed, to the point where it may increase rather than reduce maintenance cost (more often updating the test to reflect a new truth than fixing a true error). The point of view justifying such a test would be that rustc bears the responsibility that its options work as advertised even if some work is carried out by a third-party codegen library. For different stack smash protection heuristics, this means ensuring that the heuristics work as documented in the help text.

That said, I have few qualms testing this only on Linux. Reading the LLVM source, it doesn't look like there are other exceptions, and it seems reasonable to assume that any exceptions added in the future would not prompt action even if this was detected by a rustc test (apart, perhaps from updating the help text). Not to mention that clang doesn't seem to describe this exception to heuristic behaviour in its help text either. I would say it's more important to enable the end-to-end test on more targets, since codegen is more likely to have target-specific breakage with more detrimental effect.

Most recent push reverts the "option 3"-commit, and instead adds ignore-macos to the failing test. I did not update the help text as the test still covers this somewhat elaborately, with one exception shoved under the rug.

@nikic
Copy link
Contributor

nikic commented Nov 22, 2021

@bors r+ rollup=never Next try!

@bors
Copy link
Contributor

bors commented Nov 22, 2021

📌 Commit bb9dee9 has been approved by nikic

@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 Nov 22, 2021
@bors
Copy link
Contributor

bors commented Nov 23, 2021

⌛ Testing commit bb9dee9 with merge 7c4be43...

@bors
Copy link
Contributor

bors commented Nov 23, 2021

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 7c4be43 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 23, 2021
@bors bors merged commit 7c4be43 into rust-lang:master Nov 23, 2021
@rustbot rustbot added this to the 1.58.0 milestone Nov 23, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (7c4be43): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@SparrowLii
Copy link
Member

SparrowLii commented Mar 9, 2023

This is still an unstable option. Are there any unresolved issues?

@nikic
Copy link
Contributor

nikic commented Mar 9, 2023

I think a problem with stabilizing this would be that LLVM's heuristics for placing stack protectors are not appropriate for rustc. -Z stack-protector=all is well-defined, but the other two aren't.

@SparrowLii
Copy link
Member

SparrowLii commented Mar 9, 2023

Thanks for the explanation! Yea, rustc already has many security mechanisms, and the llvm's stack protection heuristics may not be suitable for it.

But there are many scenarios where Rust and other languages (such as C/C++) call each other. In this case, Rust's function stack could be broken by functions from other language. In this case, stack protection is still required. I think `stack-protector=all' is enough for these situations.

Can we separate stack-protector=all from the other options and stabilize it?

@SparrowLii
Copy link
Member

Or we can add a rustc compilation option to add stack-protector attributes only to those rust functions that call external functions

@mrcnski
Copy link
Contributor

mrcnski commented May 15, 2023

Can we separate stack-protector=all from the other options and stabilize it?

This seems reasonable, if the only issue is with the heuristics.

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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.