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

Proposal for Adapt Stack Protector for Rust #841

Open
1 of 3 tasks
SparrowLii opened this issue Feb 22, 2025 · 4 comments
Open
1 of 3 tasks

Proposal for Adapt Stack Protector for Rust #841

SparrowLii opened this issue Feb 22, 2025 · 4 comments
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@SparrowLii
Copy link
Member

SparrowLii commented Feb 22, 2025

Proposal for Adapt Stack Protector for Rust

Stack smash protection is a requirement for many products in actual production environments.

Although Rust is known for its memory safety, Rust's unsafe code may still cause stack smash risks. In the current industry, many products use Rust/C/C++ interop, which leads to the frequent use of unsafe code. This is the main reason why products have a great demand for Rust's stack smash protection.

There are three modes of stack protection: basic, strong and all. (Tracking issue here). The current status is that the all mode will significantly increase the binary size ( >7% in average) (which maybe affect performance too, needing more test), which is hard to accept for products. So they prefer to use basic/strong mode. But as this issue discussed, this two modes are for C++ and cannot be adapted to Rust.

Therefore, Rust needs to implement its own stack protection mode. Our goal is to define similar checking rules for Rust, referring to the implementation of basic/strong in gcc/clang, and enable the compiler to identify functions that need to be protected.

The following are the initially proposed function check rules for rusty mode that require stack protection in Rust(Reference for clang here):

  • calls to stack memory allocation (Although there is no direct function in the Rust standard library that provides this functionality)
  • obtaining reference/pointer of local variables

Arrays and references/pointers in Rust are of different types. If you want to use an array to manipulate stack space to cause a buffer overflow, you must first obtain a reference/pointer to it. Therefore, there is no need to specify stack protection rules for arrays in Rust.

By checking each function in the mir layer (because it is convenient to traverse rvalues), we can effectively identify functions that need to perform stack protection and add the corresponding flag in codegen.

Problems that require further discussion:

  • How to identify inline scenarios and make corresponding strategy adjustments
  • Figure out how the Rust compiler passes large structures (whether it will be optimized to pass by reference)
  • How to identify stack memory allocation functions more effectively

prototype PR: rust-lang/rust#137418

Mentors or Reviewers

@rcvalle

Thanks for your help!

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@SparrowLii SparrowLii added major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team labels Feb 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 22, 2025

Important

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rfcbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rfcbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Feb 22, 2025
@Noratrieb Noratrieb changed the title (My major change proposal) Proposal for Adapt Stack Protector for Rust Feb 22, 2025
@rcvalle
Copy link
Member

rcvalle commented Feb 25, 2025

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Feb 25, 2025
@wesleywiser
Copy link
Member

wesleywiser commented Feb 26, 2025

Recording some objections that should be resolved prior to accepting this MCP:

@rfcbot concern impl-at-mir-level

Zulip: doing this on MIR [...] will end up up protecting a lot more functions than necessary, because it happens pre-(LLVM-)inlining, so there will be a lot of addresses taken that later get inlined out.

@rfcbot concern inhibit-opts

Zulip: By inserting the stack protectors prior to optimization, we're most likely going to inhibit optimizations

@rfcbot concern lose-debuginfo-data

Zulip: Some targets (eg, msvc) record in debuginfo whether stack protectors were enabled or disabled on a per-function basis and this data is used in automated compliance tooling to ensure standard deployment practices are being followed. By inserting stack protectors earlier than LLVM is aware, we lose out on this data being captured correctly.

EDIT: reformatted comment for machine parsing

@SparrowLii
Copy link
Member Author

SparrowLii commented Mar 3, 2025

@wesleywiser Based on the previous discussion, I think these problems have solutions.

  • The heuristics need to be reduced. Instead of adding stack protection based on whether a variable reference is obtained, the following behaviors are detected:
    (1)Converts a local variable reference to a pointer type
    (2)Passing reference types(ot types contains reference) to other functions: recursively check whether the function has the behavior of converting the reference to a pointer

  • This does not affect llvm's optimizations, because we only add a LLVM attribute flag to the function, which is the same as the flag added in current stack-protection=all mode.

  • If the current stack-protector=all mode does not have this problem, then this MCP does not have either. If it have, then we can add another flag to inform the debuginfo-data tool.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Mar 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

5 participants