This is an archive of the discontinued LLVM Phabricator instance.

Add generic KCFI operand bundle lowering
ClosedPublic

Authored by samitolvanen on Oct 6 2022, 3:41 PM.

Details

Summary

The KCFI sanitizer emits "kcfi" operand bundles to indirect
call instructions, which the LLVM back-end lowers into an
architecture-specific type check with a known machine instruction
sequence. Currently, KCFI operand bundle lowering is supported only
on 64-bit X86 and AArch64 architectures.

As a lightweight forward-edge CFI implementation that doesn't
require LTO is also useful for non-Linux low-level targets on
other machine architectures, add a generic KCFI operand bundle
lowering pass that's only used when back-end lowering support is not
available and allows -fsanitize=kcfi to be enabled in Clang on all
architectures.

Diff Detail

Event Timeline

samitolvanen created this revision.Oct 6 2022, 3:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 3:41 PM
samitolvanen requested review of this revision.Oct 6 2022, 3:41 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 6 2022, 3:41 PM
kees added a comment.Oct 6 2022, 4:04 PM

What's the best way to test this in the kernel? I assume add ARCH_SUPPORTS_CFI_CLANG to an arch, and see what blows up? :) Have you tried this on any other arch yet?

What's the best way to test this in the kernel? I assume add ARCH_SUPPORTS_CFI_CLANG to an arch, and see what blows up? :)

Yes, I think that's the best way.

Have you tried this on any other arch yet?

The generic lowering pass is not ideal for kernel use as-is, because it doesn't generate a consistent machine instruction sequence, which makes error handling more difficult. The primary target audience here is firmware running on less common architectures.

That being said, I did compile a 64-bit MIPS kernel using this pass, but I didn't try booting it. I would expect to run into quite a few issues initially.

nickdesaulniers accepted this revision.Oct 7 2022, 1:57 PM
nickdesaulniers added inline comments.
llvm/lib/Transforms/Instrumentation/KCFI.cpp
49–50

prefer a range-for here

for (Instruction &I : F)
  if (auto &CI = dyn_cast<CallInst>(I);
78

CallBase *Call = ...

llvm/test/Transforms/KCFI/kcfi-supported.ll
4 ↗(On Diff #465914)

COM?

This revision is now accepted and ready to land.Oct 7 2022, 1:57 PM
kees added a comment.Oct 7 2022, 8:37 PM

That being said, I did compile a 64-bit MIPS kernel using this pass, but I didn't try booting it. I would expect to run into quite a few issues initially.

I've done a test build with riscv64, and I can see the expected loads and literal compares. Nice!

samitolvanen marked 2 inline comments as done.

Addressed feedback.

llvm/lib/Transforms/Instrumentation/KCFI.cpp
49–50

Function doesn't directly support iterating through instructions. I used inst_iterator here to avoid looping through basic blocks as well. Your example results in:

error: non-const lvalue reference to type 'llvm::Instruction' cannot bind to a value of unrelated type 'llvm::BasicBlock'
llvm/test/Transforms/KCFI/kcfi-supported.ll
4 ↗(On Diff #465914)
llvm/lib/Transforms/Instrumentation/KCFI.cpp
49–50

That's right, sorry, you'd need either a double-nested for loop

for BB : F

for I : BB

or

for I : instructions(F)

Please use that ^.

Switched to instructions(F).

samitolvanen marked an inline comment as done.Oct 10 2022, 2:55 PM
nickdesaulniers accepted this revision.Oct 10 2022, 3:37 PM
MaskRay added inline comments.Nov 1 2022, 7:37 PM
clang/lib/Driver/ToolChain.cpp
1091

This member function is overridden by various toolchains, so adding KCFI here isn't sufficient, e.g. --target=riscv64

MaskRay added inline comments.Nov 1 2022, 7:40 PM
clang/lib/Driver/ToolChain.cpp
1091

Ignore me. This works.

MaskRay added inline comments.Nov 1 2022, 7:51 PM
llvm/lib/Transforms/Instrumentation/KCFI.cpp
48

You can omit , 8 to use the default (also 8).

62

Try avoiding report_fatal_error. Use a proper error handling mechanism like Ctx.diagnose(...) with a DiagnosticInfo subclass. Remove the trailing period for the message.

73

Add const if not modified

98
llvm/test/Transforms/KCFI/kcfi-supported.ll
4 ↗(On Diff #466635)
llvm/test/Transforms/KCFI/kcfi.ll
4

Appending ( to prevent match with another function with the same prefix (not in this test, but this is a good general practice)

samitolvanen marked 6 inline comments as done.

Addressed Fangrui's feedback.

llvm/lib/Transforms/Instrumentation/KCFI.cpp
62

Try avoiding report_fatal_error. Use a proper error handling mechanism like Ctx.diagnose(...) with a DiagnosticInfo subclass.

I was looking at this, but I feel like adding a new class might be a bit of an overkill for this situation. Looking at the code in lib/Transforms, there are 70 uses of report_fatal_error and 17 uses of Ctx.diagnose(...). The latter seem to all be more complex diagnostics than just printing out an error message. Do you think I should nevertheless use Ctx.diagnose for this message?

Remove the trailing period for the message.

Done.

MaskRay added inline comments.Nov 4 2022, 1:28 PM
llvm/lib/Transforms/Instrumentation/KCFI.cpp
62

A lot of report_fatal_error uses are used similar to unreachable for both assertion and non-assertion builds. The code is pretty much reachable (if a user has a patchable_function_entry attribute and use -fsanitize=kcfi). It's bad interface to surface a crash-style diagnostic.

You may add a namespace { class ... : public DiagnosticInfo } in this file. It just needs to overload print which is not much boilerplate.

samitolvanen marked 4 inline comments as done.

Added KCFIDiagnosticInfo and switched from report_fatal_error to LLVMContext::diagnose.

MaskRay accepted this revision.Nov 4 2022, 2:33 PM
MaskRay added inline comments.
llvm/lib/Transforms/Instrumentation/KCFI.cpp
43

If you look at existing DiagnosticInfo subclasses, they are mostly named DiagnosticInfo*

llvm/test/Transforms/KCFI/kcfi-patchable-function-prefix.ll
2

You can omit <

Addressed more feedback.

samitolvanen marked 2 inline comments as done.Nov 4 2022, 3:07 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Nov 17 2022, 2:40 PM
llvm/lib/Transforms/Instrumentation/KCFI.cpp
17

Sorry, I just noticed this include. lib/Transforms/ files cannot depend on CodeGen for layering. You can see that no other Instrumentation uses CodeGen/

samitolvanen reopened this revision.Nov 17 2022, 3:54 PM
This revision is now accepted and ready to land.Nov 17 2022, 3:54 PM

Dropped the CodeGen dependency. Note that Clang already only adds KCFIPass for targets that don't support back-end lowering, so the check in the pass itself was a bit redundant. The existing CodeGen/kcfi.c test also ensures we continue to emit kcfi operand bundles.

MaskRay added inline comments.Nov 17 2022, 5:13 PM
llvm/include/llvm/Transforms/Instrumentation/KCFI.h
25

remove default constructor?

Dropped the unneeded constructor.

samitolvanen marked an inline comment as done.Nov 18 2022, 8:30 AM

Fixed clang-format warnings.

MaskRay accepted this revision.Nov 22 2022, 2:20 PM
MaskRay added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
975

To my self: I'll need to dig up why if (!IsThinLTOPostLink) { is here. I think it's unnecessary. But it's ok to keep KCFI in the same place of sanitizers.

llvm/include/llvm/Transforms/Instrumentation/KCFI.h
21

the declaration is unused now.

This revision was automatically updated to reflect the committed changes.