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.

Sign up for GitHub

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

KCFI: Add KCFI arity indicator support #138368

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+128 −0
Open

KCFI: Add KCFI arity indicator support #138368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_llvm/messages.ftl
View file
Original file line number Diff line number Diff line change
@@ -56,6 +56,8 @@ codegen_llvm_prepare_thin_lto_module_with_llvm_err = failed to prepare thin LTO
codegen_llvm_run_passes = failed to run LLVM passes
codegen_llvm_run_passes_with_llvm_err = failed to run LLVM passes: {$llvm_err}

codegen_llvm_sanitizer_kcfi_arity_requires_llvm_20_1_0 = `-Zsanitizer-kcfi-arity` requires LLVM 20.1.0 or later.

codegen_llvm_sanitizer_memtag_requires_mte =
`-Zsanitizer=memtag` requires `-Ctarget-feature=+mte`

11 changes: 11 additions & 0 deletions compiler/rustc_codegen_llvm/src/context.rs
View file
Original file line number Diff line number Diff line change
@@ -328,6 +328,17 @@ pub(crate) unsafe fn create_module<'ll>(
}
}

// Add "kcfi-arity" module flag if KCFI arity indicator is enabled. (See
// https://github.com/llvm/llvm-project/pull/117121.)
if sess.is_sanitizer_kcfi_arity_enabled() && sess.is_sanitizer_kcfi_enabled() {
// KCFI arity requires LLVM 20.1.0 or later.
if llvm_version < (20, 1, 0) {
Comment on lines +334 to +335
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the case -- there is no tag yet in upstream LLVM that contains the feature, sadly.

tcx.dcx().emit_err(crate::errors::SanitizerKcfiArityRequiresLLVM2010);
}

llvm::add_module_flag_u32(llmod, llvm::ModuleFlagMergeBehavior::Override, "kcfi-arity", 1);
}

// Control Flow Guard is currently only supported by MSVC and LLVM on Windows.
if sess.target.is_like_msvc
|| (sess.target.options.os == "windows"
4 changes: 4 additions & 0 deletions compiler/rustc_codegen_llvm/src/errors.rs
View file
Original file line number Diff line number Diff line change
@@ -217,3 +217,7 @@ pub(crate) struct MismatchedDataLayout<'a> {
pub(crate) struct FixedX18InvalidArch<'a> {
pub arch: &'a str,
}

#[derive(Diagnostic)]
#[diag(codegen_llvm_sanitizer_kcfi_arity_requires_llvm_20_1_0)]
pub(crate) struct SanitizerKcfiArityRequiresLLVM2010;
1 change: 1 addition & 0 deletions compiler/rustc_interface/src/tests.rs
View file
Original file line number Diff line number Diff line change
@@ -852,6 +852,7 @@ fn test_unstable_options_tracking_hash() {
tracked!(sanitizer_cfi_generalize_pointers, Some(true));
tracked!(sanitizer_cfi_normalize_integers, Some(true));
tracked!(sanitizer_dataflow_abilist, vec![String::from("/rustc/abc")]);
tracked!(sanitizer_kcfi_arity, Some(true));
tracked!(sanitizer_memory_track_origins, 2);
tracked!(sanitizer_recover, SanitizerSet::ADDRESS);
tracked!(saturating_float_casts, Some(true));
2 changes: 2 additions & 0 deletions compiler/rustc_session/messages.ftl
View file
Original file line number Diff line number Diff line change
@@ -94,6 +94,8 @@ session_sanitizer_cfi_requires_lto = `-Zsanitizer=cfi` requires `-Clto` or `-Cli

session_sanitizer_cfi_requires_single_codegen_unit = `-Zsanitizer=cfi` with `-Clto` requires `-Ccodegen-units=1`

session_sanitizer_kcfi_arity_requires_kcfi = `-Zsanitizer-kcfi-arity` requires `-Zsanitizer=kcfi`

session_sanitizer_kcfi_requires_panic_abort = `-Z sanitizer=kcfi` requires `-C panic=abort`

session_sanitizer_not_supported = {$us} sanitizer is not supported for this target
4 changes: 4 additions & 0 deletions compiler/rustc_session/src/errors.rs
View file
Original file line number Diff line number Diff line change
@@ -147,6 +147,10 @@ pub(crate) struct SanitizerCfiGeneralizePointersRequiresCfi;
#[diag(session_sanitizer_cfi_normalize_integers_requires_cfi)]
pub(crate) struct SanitizerCfiNormalizeIntegersRequiresCfi;

#[derive(Diagnostic)]
#[diag(session_sanitizer_kcfi_arity_requires_kcfi)]
pub(crate) struct SanitizerKcfiArityRequiresKcfi;

#[derive(Diagnostic)]
#[diag(session_sanitizer_kcfi_requires_panic_abort)]
pub(crate) struct SanitizerKcfiRequiresPanicAbort;
2 changes: 2 additions & 0 deletions compiler/rustc_session/src/options.rs
View file
Original file line number Diff line number Diff line change
@@ -2433,6 +2433,8 @@ written to standard error output)"),
"enable normalizing integer types (default: no)"),
sanitizer_dataflow_abilist: Vec<String> = (Vec::new(), parse_comma_list, [TRACKED],
"additional ABI list files that control how shadow parameters are passed (comma separated)"),
sanitizer_kcfi_arity: Option<bool> = (None, parse_opt_bool, [TRACKED],
"enable KCFI arity indicator (default: no)"),
sanitizer_memory_track_origins: usize = (0, parse_sanitizer_memory_track_origins, [TRACKED],
"enable origins tracking in MemorySanitizer"),
sanitizer_recover: SanitizerSet = (SanitizerSet::empty(), parse_sanitizers, [TRACKED],
9 changes: 9 additions & 0 deletions compiler/rustc_session/src/session.rs
View file
Original file line number Diff line number Diff line change
@@ -382,6 +382,10 @@ impl Session {
self.opts.unstable_opts.sanitizer_cfi_normalize_integers == Some(true)
}

pub fn is_sanitizer_kcfi_arity_enabled(&self) -> bool {
self.opts.unstable_opts.sanitizer_kcfi_arity == Some(true)
}

pub fn is_sanitizer_kcfi_enabled(&self) -> bool {
self.opts.unstable_opts.sanitizer.contains(SanitizerSet::KCFI)
}
@@ -1204,6 +1208,11 @@ fn validate_commandline_args_with_session_available(sess: &Session) {
}
}

// KCFI arity indicator requires KCFI.
if sess.is_sanitizer_kcfi_arity_enabled() && !sess.is_sanitizer_kcfi_enabled() {
sess.dcx().emit_err(errors::SanitizerKcfiArityRequiresKcfi);
}

// LLVM CFI pointer generalization requires CFI or KCFI.
if sess.is_sanitizer_cfi_generalize_pointers_enabled() {
if !(sess.is_sanitizer_cfi_enabled() || sess.is_sanitizer_kcfi_enabled()) {
61 changes: 61 additions & 0 deletions tests/assembly/sanitizer/kcfi/emit-arity-indicator.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Verifies that KCFI arity indicator is emitted.
//
//@ add-core-stubs
//@ revisions: x86_64
//@ assembly-output: emit-asm
//@[x86_64] compile-flags: --target x86_64-unknown-linux-gnu -C llvm-args=-x86-asm-syntax=intel -Ctarget-feature=-crt-static -Zsanitizer=kcfi -Zsanitizer-kcfi-arity -Copt-level=0
//@[x86_64] needs-llvm-components: x86
//@ min-llvm-version: 20.1.0

#![crate_type = "lib"]

pub fn add_one(x: i32) -> i32 {
// CHECK-LABEL: __cfi__{{.*}}7add_one{{.*}}:
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: mov eax, 2628068948
x + 1
}

pub fn add_two(x: i32, _y: i32) -> i32 {
// CHECK-LABEL: __cfi__{{.*}}7add_two{{.*}}:
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: mov eax, 2505940310
x + 2
}

pub fn do_twice(f: fn(i32) -> i32, arg: i32) -> i32 {
// CHECK-LABEL: __cfi__{{.*}}8do_twice{{.*}}:
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: nop
// CHECK-NEXT: mov eax, 653723426
Copy link
Member Author

@rcvalle rcvalle Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@ojeda @scottconstable I think I've it mostly done, but I suspect this test isn't right. If I understood it correctly, according to the table in llvm/llvm-project#121070 (comment), this operation should use EDX(?) because callers would pass two parameters before calling foo in RDI and RSI(?).

(Note that this isn't actually arity that is being indicated. This function's arity is three (3): two parameters, and a return value.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I am not sure what you mean. Arity is the number of parameters, without the return value, so it should use 2, thus it should use EDX.

From a quick test with Clang, that is the case. I also get the same result if I take rustc's LLVM IR and add the arity module flag.

Are you seeing a different result?

By the way, why don't we test a simpler function, e.g. one that just sums two numbers? i.e. the intention here is to notice the difference between the disabled and enabled cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the return value is not included. You're right. What I was noting there, if I understood it correctly, is that "arity" is being defined there in that PR and table at the ABI level with certain things being excluded such as parameters passed on the stack, which would cause it the "arity" defined there to diverge from the actual arity of a function, but that is not relevant here, I was just pointing it out again as I did when it was initially proposed.

Let me try a simpler pair of functions and get back to you.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still getting EAX used with a new test using a simpler pair of functions with different arity. My LLVM version is

rcvalle@rcvalle:~/rust$ build/x86_64-unknown-linux-gnu/stage1/bin/rustc --verbose --version
rustc 1.87.0-dev
binary: rustc
commit-hash: unknown
commit-date: unknown
host: x86_64-unknown-linux-gnu
release: 1.87.0-dev
LLVM version: 20.1.0

Which should include it, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which should include it, right?

I don't think so, upstream LLVM 20.1.0 does not contain it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I wanted this one landed in nightly mostly so that the kernel people could test it easily by downloading the nightly, but if rustc's LLVM does not have the patch anyway, that does not help much.

I also don't want to give too much trouble to rustc maintainers applying a patch to their LLVM either just for this.

I will probably build a test toolchain for Peter with this PR applied for the time being.

Copy link
Member Author

Choose a reason for hiding this comment

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

SG! Let me know if there is anything I can help with in the meantime. If needed, I can also remove the checks and test and get it merged, and add these later. Just let me know!

Copy link
Contributor

Choose a reason for hiding this comment

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

If you look at the llvm-main tag, you can see that we regularly condition things things to not-yet-released LLVM (e.g. LLVM 21 in this case) so that they start working as soon as released, or if rustc is built against a pre-release LLVM

Copy link
Member Author

Choose a reason for hiding this comment

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

Will take a look. Thank you! Should we condition this on LLVM 21? Or will there be any minor version release that will have this feature?

Copy link
Contributor

@maurer maurer Mar 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would condition this on LLVM 21 - My understanding of current LLVM version numbers are:

  • "Version X.0.Y" - Development version that will eventually become version X. Y is mostly meaningless, features may come and go.
  • "Version X.1.Y" - LLVM Version X, stable/released. Patchlevel Y updated to indicate bugfix backports.

So I'd just condition on the major version, e.g. >= 21.0, so that dev versions start working immediately.

There will be LLVM builds which report 21.0 which do not have support for this, but people using unreleased LLVMs usually know they need to be careful of this kind of thing, and there's not much we can do to help them. This also matches how we treat the .0 versions when dealing with LLVM compatibility issues like removed or changed APIs - we assume that if you're building against a .0 version, you're building against the latest.

f(arg) + f(arg)
}
19 changes: 19 additions & 0 deletions tests/codegen/sanitizer/kcfi/add-kcfi-arity-flag.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Verifies that "kcfi-arity" module flag is added.
//
//@ add-core-stubs
//@ revisions: x86_64
//@ [x86_64] compile-flags: --target x86_64-unknown-none
//@ [x86_64] needs-llvm-components: x86
//@ compile-flags: -Ctarget-feature=-crt-static -Zsanitizer=kcfi -Zsanitizer-kcfi-arity
//@ min-llvm-version: 20.1.0

#![feature(no_core, lang_items)]
#![crate_type = "lib"]
#![no_core]

extern crate minicore;
use minicore::*;

pub fn foo() {}

// CHECK: !{{[0-9]+}} = !{i32 4, !"kcfi-arity", i32 1}
9 changes: 9 additions & 0 deletions tests/ui/sanitizer/kcfi-arity-requires-kcfi.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// Verifies that `-Zsanitizer-kcfi-arity` requires `-Zsanitizer=kcfi`.
//
//@ needs-sanitizer-kcfi
//@ compile-flags: -Cno-prepopulate-passes -Ctarget-feature=-crt-static -Zsanitizer-kcfi-arity
//@ min-llvm-version: 20.1.0

#![feature(no_core)]
#![no_core]
#![no_main]
4 changes: 4 additions & 0 deletions tests/ui/sanitizer/kcfi-arity-requires-kcfi.stderr
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
error: `-Zsanitizer-kcfi-arity` requires `-Zsanitizer=kcfi`

error: aborting due to 1 previous error

Loading