Stabilize stack-protector

[arielb1]

View all comments

Stabilize -Zstack-protector as -Cstack-protector

I propose stabilizing -Zstack-protector as -Cstack-protector. This PR adds a new -Cstack-protector flag, leaving the unstable -Z flag as is to ease the transition period. The -Z flag will be removed in the future.

Please hold technical discussion in the zulip thread at https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20Adapt.20Stack.20Protector.20for.20Ru.E2.80.A6.20compiler-team.23841

Let's move mitigation enforcement discussion to https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Mitigation.20enforcement.20.28.60-C.20allow-partial-sanitizer.60.29/with/539144305

-Zstack-protector stabilization report

What is the RFC for this feature and what changes have occurred to the user-facing design since the RFC was finalized?

No RFC/MCP, this flag was added in #84197 and was not deemed large enough to require additional process.

The tracking issue for this feature is #114903.

What behavior are we committing to that has been controversial? Summarize the major arguments pro/con.

The -Cstack-protector=strong mode uses the same underlying heuristics as Clang's -fstack-protector-strong.
These heuristics weren't designed for Rust, and may be over-conservative in some cases - for example, if
Rust stores a field's data in an alloca using an LLVM array type, LLVM regard the alloca as meaning
that the function has a C array, and enable stack overflow canaries even if the function accesses
the alloca in a safe way. Some people thought we should wait on stabilization until there are better
heuristics, but I didn't hear about any concrete case where this unduly harms performance, and I think
that when a need comes, we can improve the heuristics in LLVM after stabilization.

The heuristics do seem to not be under-conservative, so this should not be a security risk.

The -Cstack-protector=basic mode (-fstack-protector) uses heuristics that are specifically designed
to catch old-C-style string manipulation. This is not a good fit to Rust, which does not perform much
unsafe C-style string manipulation. As far as I can tell, nobody has been asking for it,
and few people are using it even in today's C - modern distros (e.g. Debian) tend to use
-fstack-protector-strong.

Therefore, -Cstack-protector=basic has been removed. If anyone is interested in it, they
are welcome to add it back as an unstable option.

Summarize the major parts of the implementation and provide links into the code (or to PRs)

Most implementation was done in #84197. The command-line
attribute enables the relevant LLVM attribute on all functions in

rust/compiler/rustc_codegen_llvm/src/attributes.rs

Lines 267 to 276 in 68baa87

	fn stackprotector_attr<'ll>(cx: &CodegenCx<'ll, '_>) -> Option<&'ll Attribute> {
	let sspattr = match cx.sess().stack_protector() {
	StackProtector::None => return None,
	StackProtector::All => AttributeKind::StackProtectReq,
	StackProtector::Strong => AttributeKind::StackProtectStrong,
	StackProtector::Basic => AttributeKind::StackProtect,
	};
	Some(sspattr.create_attr(cx.llcx))
	}

.

Each target can indicate that it does not support stack canaries - currently,
the GPU platforms nvptx64-nvidia-cuda and amdgcn-amd-amdhsa. On these
platforms, use of -Cstack-protector causes an error.

Summarize existing test coverage of this feature

The feature has tests that make sure that the LLVM heuristic gives reasonable
results for several functions, by checking for __security_check_cookie (on Windows)
or __stack_chk_fail (on Linux). See
https://github.com/rust-lang/rust/tree/68baa87ba6f03f8b6af2a368690161f1601e4040/tests/assembly-llvm/stack-protector

Has a call-for-testing period been conducted? If so, what feedback was received?

No call-for-testing has been conducted, but the feature seems to be in use.

What outstanding bugs in the issue tracker involve this feature? Are they stabilization-blocking?

No reported bugs seem to exist.

Summarize contributors to the feature by name for recognition and assuredness that people involved in the feature agree with stabilization

• @bbjornse was the original implementor at add codegen option for using LLVM stack smash protection #84197
• @mrcnski documented it at Document stack-protector option #111722
• @wesleywiser added tests for Windows at Add -Zstack-protector test for Windows targets #116037
• @davidtwco worked on the feature at sess: stabilize -C stack-protector=all #121742
• @nikic provided support from the LLVM side (on Zulip on https://rust-lang.zulipchat.com/#narrow/channel/233931-t-compiler.2Fmajor-changes/topic/Proposal.20for.20Adapt.20Stack.20Protector.20for.20Ru.E2.80.A6.20compiler-team.23841 and elsewhere),
  thanks @nikic!

What FIXMEs are still in the code for that feature and why is it ok to leave them there?

No FIXMEs related to this feature.

What static checks are done that are needed to prevent undefined behavior?

This feature cannot cause undefined behavior.

In what way does this feature interact with the reference/specification, and are those edits prepared?

No changes to reference/spec, docs added to the codegen docs as part of the stabilization PR.

Does this feature introduce new expressions and can they produce temporaries? What are the lifetimes of those temporaries?

No.

What other unstable features may be exposed by this feature?

None.

What is tooling support like for this feature, w.r.t rustdoc, clippy, rust-analzyer, rustfmt, etc.?

No support needed for rustdoc, clippy, rust-analyzer, rustfmt or rustup.

Cargo could expose this as an option in build profiles but I would expect the decision as to what version should be used would
be made for the entire crate graph at build time rather than by individual package authors.

-C stack-protector is propagated to C compilers using cc-rs via rust-lang/cc-rs#1550

Fixes #114903

r? @wesleywiser (feel free to reassign)