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

Add rusty stack protector #137418

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions compiler/rustc_codegen_llvm/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -254,12 +254,19 @@ fn probestack_attr<'ll>(cx: &CodegenCx<'ll, '_>) -> Option<&'ll Attribute> {
Some(llvm::CreateAttrStringValue(cx.llcx, "probe-stack", attr_value))
}

fn stackprotector_attr<'ll>(cx: &CodegenCx<'ll, '_>) -> Option<&'ll Attribute> {
fn stackprotector_attr<'ll>(cx: &CodegenCx<'ll, '_>, def_id: DefId) -> 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,
StackProtector::Rusty => {
if cx.tcx.stack_protector.borrow().contains(&def_id) {
AttributeKind::StackProtectReq
} else {
return None;
}
}
};

Some(sspattr.create_attr(cx.llcx))
Expand Down Expand Up @@ -384,7 +391,9 @@ pub(crate) fn llfn_attrs_from_instance<'ll, 'tcx>(
to_add.extend(instrument_function_attr(cx));
to_add.extend(nojumptables_attr(cx));
to_add.extend(probestack_attr(cx));
to_add.extend(stackprotector_attr(cx));

// stack protector
to_add.extend(stackprotector_attr(cx, instance.def_id()));

if codegen_fn_attrs.flags.contains(CodegenFnAttrFlags::NO_BUILTINS) {
to_add.push(llvm::CreateAttrString(cx.llcx, "no-builtins"));
Expand Down
5 changes: 4 additions & 1 deletion compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rustc_abi::{ExternAbi, FieldIdx, Layout, LayoutData, TargetDataLayout, Varia
use rustc_ast as ast;
use rustc_data_structures::defer;
use rustc_data_structures::fingerprint::Fingerprint;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_data_structures::intern::Interned;
use rustc_data_structures::profiling::SelfProfilerRef;
use rustc_data_structures::sharded::{IntoPointer, ShardedHashMap};
Expand Down Expand Up @@ -1392,6 +1392,8 @@ pub struct GlobalCtxt<'tcx> {
/// Stores memory for globals (statics/consts).
pub(crate) alloc_map: interpret::AllocMap<'tcx>,

pub stack_protector: Lock<FxHashSet<DefId>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct for incremental (it's good enough to collect some data but would have to be fixed before merging). One implementation strategy I would try is to use the same mechanism as the (I forgot how exactly it's called) inferred function attrs thing pcwalton implemented a while ago.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think so. I'm trying to think of a more convenient way than adding a new query

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I will learn the strategy you mentioned.


current_gcx: CurrentGcx,
}

Expand Down Expand Up @@ -1608,6 +1610,7 @@ impl<'tcx> TyCtxt<'tcx> {
canonical_param_env_cache: Default::default(),
data_layout,
alloc_map: interpret::AllocMap::new(),
stack_protector: Default::default(),
current_gcx,
});

Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ mod pass_manager;
use std::sync::LazyLock;

use pass_manager::{self as pm, Lint, MirLint, MirPass, WithMinOptLevel};
use rustc_target::spec::StackProtector;

mod check_pointers;
mod cost_checker;
Expand Down Expand Up @@ -197,6 +198,7 @@ declare_passes! {
mod single_use_consts : SingleUseConsts;
mod sroa : ScalarReplacementOfAggregates;
mod strip_debuginfo : StripDebugInfo;
mod stack_protector: StackProtectorFinder;
mod unreachable_enum_branching : UnreachableEnumBranching;
mod unreachable_prop : UnreachablePropagation;
mod validate : Validator;
Expand Down Expand Up @@ -451,6 +453,17 @@ fn mir_promoted(
lint_tail_expr_drop_order::run_lint(tcx, def, &body);

let promoted = promote_pass.promoted_fragments.into_inner();

if tcx.sess.stack_protector() == StackProtector::Rusty {
pm::run_passes(
tcx,
&mut body,
&[&stack_protector::StackProtectorFinder],
None,
pm::Optimizations::Allowed,
)
}

(tcx.alloc_steal_mir(body), tcx.alloc_steal_promoted(promoted))
}

Expand Down
46 changes: 46 additions & 0 deletions compiler/rustc_mir_transform/src/stack_protector.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
//! Validates the MIR to ensure that invariants are upheld.

use std::ops::Deref;

use rustc_middle::mir::*;
use rustc_middle::ty;
use rustc_middle::ty::TyCtxt;

pub(super) struct StackProtectorFinder;

impl<'tcx> crate::MirPass<'tcx> for StackProtectorFinder {
fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
use Rvalue::*;
let def_id = body.source.def_id();

for block in body.basic_blocks.iter() {
for stmt in block.statements.iter() {
if let StatementKind::Assign(assign) = &stmt.kind {
let (_, rvalue) = assign.deref();
match rvalue {
// Get a reference/pointer to a variable
Ref(..) | ThreadLocalRef(_) | RawPtr(..) => {
tcx.stack_protector.borrow_mut().insert(def_id);
return;
}
_ => continue,
}
}
}

if let Some(terminator) = block.terminator.as_ref() {
if let TerminatorKind::Call { destination: place, .. } = &terminator.kind {
// Returns a mutable raw pointer, possibly a memory allocation function
if let ty::RawPtr(_, Mutability::Mut) = place.ty(body, tcx).ty.kind() {
tcx.stack_protector.borrow_mut().insert(def_id);
return;
}
}
}
}
}

fn is_required(&self) -> bool {
true
}
}
8 changes: 8 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1605,6 +1605,12 @@ pub enum StackProtector {
/// the address of a local variable.
Strong,

/// Stack protection for Rust code, the following are function check rules
/// that require stack protection in Rust:
/// - calls to stack memory allocation
/// - obtaining reference/pointer of local variables
Rusty,

/// Generate stack canaries in all functions.
All,
}
Expand All @@ -1615,6 +1621,7 @@ impl StackProtector {
StackProtector::None => "none",
StackProtector::Basic => "basic",
StackProtector::Strong => "strong",
StackProtector::Rusty => "rusty",
StackProtector::All => "all",
}
}
Expand All @@ -1628,6 +1635,7 @@ impl FromStr for StackProtector {
"none" => StackProtector::None,
"basic" => StackProtector::Basic,
"strong" => StackProtector::Strong,
"rusty" => StackProtector::Rusty,
"all" => StackProtector::All,
_ => return Err(()),
})
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
//@ revisions: all strong basic none missing
//@ revisions: all strong basic none missing rusty
//@ assembly-output: emit-asm
//@ only-windows
//@ only-msvc
//@ ignore-32bit 64-bit table based SEH has slightly different behaviors than classic SEH
//@ [all] compile-flags: -Z stack-protector=all
//@ [rusty] compile-flags: -Z stack-protector=rusty
//@ [strong] compile-flags: -Z stack-protector=strong
//@ [basic] compile-flags: -Z stack-protector=basic
//@ [none] compile-flags: -Z stack-protector=none
Expand All @@ -21,6 +22,8 @@ pub fn emptyfn() {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty-NOT: __security_check_cookie
}

// CHECK-LABEL: array_char
Expand All @@ -39,6 +42,8 @@ pub fn array_char(f: fn(*const char)) {
// basic: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: array_u8_1
Expand All @@ -55,6 +60,8 @@ pub fn array_u8_1(f: fn(*const u8)) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: array_u8_small:
Expand All @@ -72,6 +79,8 @@ pub fn array_u8_small(f: fn(*const u8)) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: array_u8_large:
Expand All @@ -88,6 +97,8 @@ pub fn array_u8_large(f: fn(*const u8)) {
// basic: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

#[derive(Copy, Clone)]
Expand All @@ -107,6 +118,8 @@ pub fn array_bytesizednewtype_9(f: fn(*const ByteSizedNewtype)) {
// basic: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: local_var_addr_used_indirectly
Expand Down Expand Up @@ -134,6 +147,8 @@ pub fn local_var_addr_used_indirectly(f: fn(bool)) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: local_string_addr_taken
Expand Down Expand Up @@ -172,6 +187,8 @@ pub fn local_string_addr_taken(f: fn(&String)) {
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty-NOT: __security_check_cookie

// CHECK-DAG: .seh_endproc
}

Expand Down Expand Up @@ -202,6 +219,9 @@ pub fn local_var_addr_taken_used_locally_only(factory: fn() -> i32, sink: fn(i32
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// FIXME: rusty stack smash protection needs to support inline scenario detection
// rusty: __security_check_cookie
}

pub struct Gigastruct {
Expand Down Expand Up @@ -239,6 +259,9 @@ pub fn local_large_var_moved(f: fn(Gigastruct)) {
// basic: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// FIXME: How does the rust compiler handle moves of large structures?
// rusty-NOT: __security_check_cookie
}

// CHECK-LABEL: local_large_var_cloned
Expand Down Expand Up @@ -268,6 +291,9 @@ pub fn local_large_var_cloned(f: fn(Gigastruct)) {
// basic: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// FIXME: How does the rust compiler handle moves of large structures?
// rusty-NOT: __security_check_cookie
}

extern "C" {
Expand Down Expand Up @@ -308,6 +334,11 @@ pub fn alloca_small_compile_time_constant_arg(f: fn(*mut ())) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// FIXME: Rusty thinks a function that returns a mutable raw pointer may
// be a stack memory allocation function, so it performs stack smash protection.
// Is it possible to optimize the heuristics?
// rusty: __security_check_cookie
}

// CHECK-LABEL: alloca_large_compile_time_constant_arg
Expand All @@ -320,6 +351,8 @@ pub fn alloca_large_compile_time_constant_arg(f: fn(*mut ())) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// CHECK-LABEL: alloca_dynamic_arg
Expand All @@ -332,6 +365,8 @@ pub fn alloca_dynamic_arg(f: fn(*mut ()), n: usize) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty: __security_check_cookie
}

// The question then is: in what ways can Rust code generate array-`alloca`
Expand Down Expand Up @@ -364,6 +399,8 @@ pub fn unsized_fn_param(s: [u8], l: bool, f: fn([u8])) {
// basic-NOT: __security_check_cookie
// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty-NOT: __security_check_cookie
}

// CHECK-LABEL: unsized_local
Expand All @@ -388,4 +425,6 @@ pub fn unsized_local(s: &[u8], l: bool, f: fn(&mut [u8])) {

// none-NOT: __security_check_cookie
// missing-NOT: __security_check_cookie

// rusty-NOT: __security_check_cookie
}
Loading
Loading