Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
add codegen option for using LLVM stack smash protection #84197
Conversation
|
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
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
r? @nikic |
|
|
|
|
src/test/assembly/stack-protector/stack-protector-heuristics-effect.rs
Outdated
Show resolved
Hide resolved
src/test/assembly/stack-protector/stack-protector-heuristics-effect.rs
Outdated
Show resolved
Hide resolved
src/test/ui/stack-protector/warn-stack-protector-unsupported.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 { | |||
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
|
MCP isn't necessary for adding an unstable option, no. During the stabilization a final comment period vote will occur instead. |
|
Default stack protector option is no stack protector, but there no such option as |
|
|
Right, there is no stack protection unless requested. This is also the case with gcc and clang, I believe, so |
I mean, that if that option mimic clang's one (that have |
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 |
|
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). |
This comment has been minimized.
This comment has been minimized.
|
|
|
I think I see your point @klensy and @nagisa: although the usefulness of an explicit I added this capability in the most recent commit. (The patch is larger than strictly necessary as the |
|
|
bors
added
the
S-waiting-on-review
|
@bors r- bors doesn't seem to have remembered that the tests failed. |
bors
added
S-waiting-on-author
|
Investigating the test failure, it appears "basic" stack protection is slightly different on OS X as functions with local arrays are protected regardless of element size (llvm/lib/CodeGen/StackProtector.cpp:130). I see the following options:
I'm not quite sure which option is preferable. I would say option 3, but I'm not sure how the parameterization would be best accomplished. I'm also having problems running the test with In either case I also think the description of what the different stack-protector options do should be modified to be either less specific (if we ignore this behaviour) or more specific (if we have explicit tests for the target variation). |
|
Added a commit pursuing option 3. I think this would work, but I've not been able to test it with |
This comment has been minimized.
This comment has been minimized.
|
|
|
Ping from triage: |
|
@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
added
S-waiting-on-review
|
|
|
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 <[email protected]>
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 a49b74f.
|
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 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 Most recent push reverts the "option 3"-commit, and instead adds |
|
@bors r+ rollup=never Next try! |
|
|
bors
added
S-waiting-on-bors
|
|
|
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 |
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 theseattributes. 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 intest/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 alternativeheuristics 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.