Skip to content

[CrossDSO CFI] Make sure __cfi_check has BTI attribute. #131224

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

Closed
wants to merge 2 commits into from
Closed

[CrossDSO CFI] Make sure __cfi_check has BTI attribute. #131224

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbdee73
[CrossDSO CFI] Make sure __cfi_check has BTI attribute.
efriedma-quic Mar 13, 2025
256aeb1
Use createWithDefaultAttr instead of explicitly modifying attributes.
efriedma-quic Mar 14, 2025
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
8 changes: 8 additions & 0 deletions llvm/lib/Transforms/IPO/CrossDSOCFI.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,14 @@ void CrossDSOCFI::buildCFICheck(Module &M) {
Triple T(M.getTargetTriple());
if (T.isARM() || T.isThumb())
F->addFnAttr("target-features", "+thumb-mode");
if (T.isAArch64()) {
Copy link
Member

@DanielKristofKiss DanielKristofKiss Mar 14, 2025

Choose a reason for hiding this comment

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

M.getOrInsertFunction could be replaced with the Function::createWithDefaultAttr to create the functions with the right set of attributes. It is used in other places in the sanitisers to create functions. It will solve this and future problems.

Copy link
Collaborator Author

@efriedma-quic efriedma-quic Mar 14, 2025

Choose a reason for hiding this comment

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

Gave this a shot. Seems to be okay for aarch64, but I'm a little concerned this will cause issues for other targets (see #98673).

Copy link
Contributor

@pcc pcc Mar 17, 2025

Choose a reason for hiding this comment

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

The way I think this should work is that the function is created by the frontend with the correct attributes and then filled in by the LTO backend. See #100833 for details. That approach is compatible with #98673 because the attributes are still coming from the frontend.

Copy link
Collaborator Author

@efriedma-quic efriedma-quic Mar 17, 2025

Choose a reason for hiding this comment

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

I briefly described why that doesn't work in the commit message, but I'll go into more detail:

In ThinLTO, the code is in modules which correspond to translation units in the original source code. However, depending on the exact structure of the inputs, you can end up with an additional "base" module that only contains some global variables, and no code. LTO chooses to generate __cfi_check into that module; when it does, there is no function to take the attributes from.

So there's a function named __cfi_check, created by the frontend, in every module except the one that actually matters. So I'm just trying to repair the IR the best I can once we've reached this point.

If you think something is supposed to happen earlier in the pipeline, can you describe what, exactly, that looks like?

Copy link
Contributor

@pcc pcc Mar 17, 2025

Choose a reason for hiding this comment

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

My expectation would be that __cfi_check always appears on the full LTO side and has the correct attributes. If we end up generating code for that function without attributes, it sounds like a bug. Do you have a reproducer showing how __cfi_check can end up being code generated without attributes?

Copy link
Collaborator Author

@efriedma-quic efriedma-quic Mar 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

$ cat a.c
int a = 3;
void f(){}
$ cat b.c
void g(){}
$ clang a.c b.c -flto=thin -fsanitize=cfi -fsanitize-cfi-cross-dso -O2 -c -fvisibility=default
$ clang a.o b.o -flto=thin -fsanitize=cfi -fsanitize-cfi-cross-dso -O2 -fuse-ld=lld -shared -Wl,-save-temps -fvisibility=default

__cfi_check is generated in a.out.0.5.precodegen.bc, which doesn't contain a stub.

Copy link
Contributor

@pcc pcc Mar 17, 2025

Choose a reason for hiding this comment

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

Thanks. With the attached patch (not extensively tested) it looks like __cfi_check now goes to the correct module. Next step to fix this bug would be to resolve issues pointed out in #100833 so that the frontend gives it the correct attributes.

0001-ThinLTOBitcodeWriter-Move-__cfi_check-to-merged-modu.patch.txt

Copy link
Collaborator Author

@efriedma-quic efriedma-quic Aug 15, 2025

Choose a reason for hiding this comment

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

Revisiting this because it came up internally again...

I tried out your patch for the ThinLTOBitcodeWriter: it seems to do the right thing. And the frontend puts the correct attributes on __cfi_check (on main, back to at least LLVM 20). So overall it works, although the solution doesn't really generalize well to other contexts.

Do you want to move forward with ThinLTOBitcodeWriter patch? Or some variation of my patch?

Copy link
Contributor

@pcc pcc Aug 21, 2025

Choose a reason for hiding this comment

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

Let me send my patch for review.

Copy link
Contributor

@pcc pcc Aug 21, 2025

Choose a reason for hiding this comment

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

#154833

if (const auto *BTE = mdconst::extract_or_null<ConstantInt>(
M.getModuleFlag("branch-target-enforcement"))) {
if (BTE->getZExtValue() != 0) {
F->addFnAttr("branch-target-enforcement");
}
}
}

auto args = F->arg_begin();
Value &CallSiteTypeId = *(args++);
Expand Down
53 changes: 53 additions & 0 deletions llvm/test/Transforms/CrossDSOCFI/aarch64-bti.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
; RUN: opt -S -passes=cross-dso-cfi < %s | FileCheck %s

; CHECK: define void @__cfi_check({{.*}}) [[ATTR:#[0-9]+]] align 4096
; CHECK: [[ATTR]] = { "branch-target-enforcement" }

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "aarch64-unknown-linux-gnu"

@_ZTV1A = constant i8 0, !type !4, !type !5
@_ZTV1B = constant i8 0, !type !4, !type !5, !type !6, !type !7

define signext i8 @f11() "branch-target-enforcement" !type !0 !type !1 {
entry:
ret i8 1
}

define signext i8 @f12() "branch-target-enforcement" !type !0 !type !1 {
entry:
ret i8 2
}

define signext i8 @f13() "branch-target-enforcement" !type !0 !type !1 {
entry:
ret i8 3
}

define i32 @f21() "branch-target-enforcement" !type !2 !type !3 {
entry:
ret i32 4
}

define i32 @f22() "branch-target-enforcement" !type !2 !type !3 {
entry:
ret i32 5
}

define weak_odr hidden void @__cfi_check_fail(ptr, ptr) "branch-target-enforcement" {
entry:
ret void
}

!llvm.module.flags = !{!8, !9}

!0 = !{i64 0, !"_ZTSFcvE"}
!1 = !{i64 0, i64 111}
!2 = !{i64 0, !"_ZTSFivE"}
!3 = !{i64 0, i64 222}
!4 = !{i64 16, !"_ZTS1A"}
!5 = !{i64 16, i64 333}
!6 = !{i64 16, !"_ZTS1B"}
!7 = !{i64 16, i64 444}
!8 = !{i32 4, !"Cross-DSO CFI", i32 1}
!9 = !{i32 8, !"branch-target-enforcement", i32 1}
Loading