add codegen option for using LLVM stack smash protection

[bbjornse]

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]

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.

[petrochenkov]

r? @nikic

[bors]

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

[bors]

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

[nikic]

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?

[nikic]

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...

[bbjornse]

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]

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

[nikic]

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

[klensy]

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

[bors]

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

[bbjornse]

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]

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]

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]

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).

[bors]

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

[bbjornse]

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]

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

[bors]

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

[JohnCSimon]

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

[bbjornse]

@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.

[bors]

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

[nikic]

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.

[bbjornse]

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]

@bors r+ rollup=never Next try!

[bors]

📌 Commit bb9dee9 has been approved by nikic

[bors]

⌛ Testing commit bb9dee9 with merge 7c4be43...

[bors]

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

[rust-timer]

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]

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

[nikic]

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]

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]

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

[mrcnski]

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

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