Tracking Issue for stabilizing stack smashing protection (i.e., -Z stack-protector)

[rcvalle]

This is a tracking issue for stabilizing stack smashing protection (i.e., -Z stack-protector). The was added in #84197 without a tracking issue. There is also no feature gate for it.

As part of the work on reviewing previously-added support for exploit mitigations to the Rust compiler (see https://hackmd.io/@rcvalle/H1epy5Xqn), @luismerino and I, and also @wesleywiser reviewed the stack smashing protection option (i.e., -Z stack-protector) implementation and it looked good for stabilization, so we'd like to propose stabilizing it.

Steps

TBD.

Unresolved Questions

Is there any concern or anything that the community would like to see implemented before its stabilization?

• The analysis is based on LLVM IR types which are meaningless.

Implementation history

• add codegen option for using LLVM stack smash protection #84197
• Document stack smashing protection #93052
• LLVM 17 regression: tests/ui/abi/stack-protector.rs no longer actually attempts stack-smashing #109671
• Document stack-protector option #111722
• Add -Zstack-protector test for Windows targets #116037

[nikic]

Concern: The heuristics for -Z stack-protector=strong and -Z stack-protector=basic do not work in any sensible manner for LLVM IR produced by rustc. If we want to stabilize these options, they need to be re-implemented in a completely different way that is based on annotations provided by the front-end, rather than (policy-violating) IR type analysis in the backend.

I believe the only option that has well-defined semantics for rust is -Z stack-protector=all.

[rcvalle]

Hey @nikic! Thanks for bringing this up! Do you have any examples of this? Are you referring to any of the cases listed at https://github.com/rust-lang/rust/blob/master/tests/assembly/stack-protector/stack-protector-heuristics-effect.rs (e.g., array_char, local_string_addr_taken, or the alloca cases)?

[nikic]

@rcvalle If you look at the history of the file, you will see that the behavior changes occasionally. This is an artifact of exposing compiler implementation details and should not happen.

You can find the implementation of these heuristics here: https://github.com/llvm/llvm-project/blob/52db7e27458f774fa0c6c6a864ce197fa071a230/llvm/lib/CodeGen/StackProtector.cpp#L125

The type argument is the alloca type, which should generally only be inspected for size and alignment. There are at least two ways in which this can go wrong:

• Structs may include [n x iM] as padding to satisfy alignment requirements. This may end up triggering array heuristics where no actual arrays are involved.
• Arrays in Rust are often wrapped in structs like { [n x %T] }, which affects heuristics.

There are long-term plans to always emit [n x i8] types to insulate us from (precisely this kind of) LLVM stupidity, and that should not have an effect on heuristics.

[daira]

What's the current progress? Re: @nikic 's comment, can/should -Z stack-protector=all be stabilized on its own for now?

[rcvalle]

What's the current progress? Re: @nikic 's comment, can/should -Z stack-protector=all be stabilized on its own for now?

Sorry for the late reply. I'm planning to resume working on this soon. In the meantime, @davidtwco is working on stabilizing -Z stack-protector=all so we might get it stabilized before then.

[marmeladema]

I just saw in the stabilization PR that people are asking for use cases and I wonder if the issue I am facing is a good one.

I am running rust code as a shared library in a (big) C process with lots of other deps etc but it happens that I get crashes in a "pure" rust thread from time to time and I haven't been able to find a root cause for now.
And in some cases, the stack is completely trashed and I was thinking that maybe enabling stack-protection would crash the process sooner, maybe at a point which would help me understand the problem better.

At least that's something I would have wanted to try to see if it helps at all and that's how I ended up here :)
Is this a valid use case?

cc @nikic @davidtwco

[lxy19980601]

Concern: The heuristics for -Z stack-protector=strong and -Z stack-protector=basic do not work in any sensible manner for LLVM IR produced by rustc. If we want to stabilize these options, they need to be re-implemented in a completely different way that is based on annotations provided by the front-end, rather than (policy-violating) IR type analysis in the backend.

I believe the only option that has well-defined semantics for rust is -Z stack-protector=all.

Is there any cases showing -Z stack-protector=strong and -Z stack-protector=basic do not work` ? And are there any plans to re-implement these two options?

[nikic]

@marmeladema So ... did you actually try it, using a nightly compiler or RUSTC_BOOTSTRAP=1? I'd say that that is definitely not an intended use case for the feature (your use case seems more like a case for -Z sanitizer=address), but it would still be good to know it would actually help find the issue or not.

[the8472]

Doesn't this require -Zbuild-std anyway to get full coverage?

[arielb1]

What's the actual problem with -Zstack-protector=strong?

The intent is to put a stack protector on every function that does something with the address of a local variable that LLVM can't prove safe even assuming UB, and from a cursory look it does achieve that in Rust as well.

What does not work particularly well in Rust is -Zstack-protector=basic aka -fstack-protector. But that mode is intended to solely guard against overflows coming from unsafe use of C strings or memcpy, and that's not the major risk in Rust.

I definitely see a path forward where we keep -Z stack-protector=basic unstable forever since I don't see the use case.

[arielb1]

rust-lang/cc-rs#1550 landed