This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Implement KCFI operand bundle lowering
ClosedPublic

Authored by samitolvanen on Apr 14 2023, 3:14 PM.

Details

Summary

With -fsanitize=kcfi (Kernel Control-Flow Integrity), Clang emits
"kcfi" operand bundles to indirect call instructions. Similarly to
the target-specific lowering added in D119296, implement KCFI operand
bundle lowering for RISC-V.

This patch disables the generic KCFI pass for RISC-V in Clang, and
adds the KCFI machine function pass in RISCVPassConfig::addPreSched
to emit target-specific KCFI_CHECK pseudo instructions before calls
that have KCFI operand bundles. The machine function pass also bundles
the instructions to ensure we emit the checks immediately before the
calls, which is not possible with the generic pass.

KCFI_CHECK instructions are lowered in RISCVAsmPrinter to a
contiguous code sequence that traps if the expected hash in the
operand bundle doesn't match the hash before the target function
address. This patch emits an ebreak instruction for error handling
to match the Linux kernel's BUG() implementation. Just like for X86,
we also emit trap locations to a .kcfi_traps section to support
error handling, as we cannot embed additional information to the trap
instruction itself.

Diff Detail

Event Timeline

samitolvanen created this revision.Apr 14 2023, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 3:14 PM
samitolvanen requested review of this revision.Apr 14 2023, 3:14 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 14 2023, 3:14 PM
llvm/lib/Target/RISCV/RISCVKCFI.cpp
107–108

for (MachineInstr &MI : MBB)

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
299–300

if (++NextReg > RISCV::X31)

I can't help but feel the core of this should be target-independent, with TLI or similar hooks to actually do the target-specific bits?

llvm/test/CodeGen/RISCV/kcfi.ll
2

Please try not to introduce tests with hand-written CHECK lines, but if you have to, try and make them look similar in style to the auto-generated lines.

jrtc27 added inline comments.Apr 17 2023, 2:23 PM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
277

What about this code relies on it being 64-bit?

llvm/lib/Target/RISCV/RISCVInstrInfo.td
1898

This formatting clearly doesn't match the surrounding code

1901

Don't introduce two blank lines

craig.topper added inline comments.Apr 17 2023, 2:44 PM
llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
310

Are there two many operands here?

310

Err. "too many"

craig.topper added inline comments.Apr 17 2023, 2:48 PM
llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
81 ↗(On Diff #513763)

Isn't this done automatically by the GN Syncbot when CMakeLists.txt is updated?

samitolvanen marked 9 inline comments as done.Apr 18 2023, 11:30 AM

I can't help but feel the core of this should be target-independent, with TLI or similar hooks to actually do the target-specific bits?

I agree, that might be a nice improvement. The AArch64 version at least is nearly identical to this one, although there's a bit more going on in the X86 version. I'll take a look.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
277

Good point, this doesn't require much tweaking to also work on RV32. We just need to avoid ADDIW.

299–300

That's not quite the same. We want to check the current register, not the next one, which we might not even need. I can certainly not post-increment the NextReg value if it's confusing.

310

Yes there are, thanks for noticing.

llvm/lib/Target/RISCV/RISCVKCFI.cpp
107–108

We don't want to skip bundles here, so we have to use instr_iterator explicitly.

llvm/test/CodeGen/RISCV/kcfi.ll
2

update_llc_test_checks.py doesn't play well with testing function prefix data, but I updated the style to match its output, added RV32 (without duplicating identical code), and moved the MIR tests to separate files with auto-generated checks. Please let me know if there's something specific I'm missing here.

llvm/utils/gn/secondary/llvm/lib/Target/RISCV/BUILD.gn
81 ↗(On Diff #513763)

I remember updating this manually before, but if that's the case, I'm happy to drop this line.

samitolvanen planned changes to this revision.Apr 18 2023, 11:30 AM
samitolvanen marked 5 inline comments as done.

Addressed most of the feedback.

samitolvanen retitled this revision from [RISCV] Implement KCFI operand bundle lowering for RV64 to [RISCV] Implement KCFI operand bundle lowering.Apr 18 2023, 11:45 AM
samitolvanen edited the summary of this revision. (Show Details)
samitolvanen planned changes to this revision.May 4 2023, 3:51 PM

Uploaded D149915 to remove the arch-specific KCFI passes. I'll update this patch once we clean that up.

Dropped the arch-specific KCFI machine function pass.

samitolvanen edited the summary of this revision. (Show Details)Jun 2 2023, 1:08 PM
evandro removed a subscriber: evandro.Jun 2 2023, 1:20 PM
MaskRay added a comment.EditedJun 5 2023, 8:30 PM

Sorry for the very slow response.

KCFI_CHECK lowering has some complexity to allocate a temporary register. This needs to follow the calling convention which can be modified by many compiler options and function attributes.

I wonder whether we can move the if-condition part of the expanded code sequence (i.e. if type-hashes mismatch; crash) to ClangCodeGen (more like`-fsanitize=function`), and change the "kcfi" operand bundle to focus on expanding to a desired trap instruction (ud2 on x86-64).
On the plus side, this gives optimizers more opportunities to place trap basic blocks to cold regions.
On the downside, we cannot assume the code sequence is contiguous but that may be fine.


Some notice for other reviewers:

To emit .kcfi_traps and a preferred trap instruction, the instrumentation cannot be done purely in ClangCodeGen. Instead, the following scheme is used:

  • ClangCodeGen emits "kcfi" operand bundles.
  • In a target-overridden TargetPassConfig::addPreSched, add a KCFI pass to emit KCFI_CHECK pseudo instructions.
  • In a target AsmPrinter, lower KCFI_CHECK pseudo instructions to a code sequence that crash if the type hashes don't match.

If a target doesn't implement KCFI_CHECK lowering, llvm/lib/Transforms/Instrumentation/KCFI.cpp is used to replace "kcfi" operand bundles with if conditions, then no TargetPassConfig::addPreSched or KCFI_CHECK lowering will be needed.
In this case, llvm.debugtrap is used to generate trap instructions.

KCFI_CHECK lowering has some complexity to allocate a temporary register. This needs to follow the calling convention which can be modified by many compiler options and function attributes.

Correct, although in the RISC-V case, finding two temporary registers shouldn't be an issue. The latest Zisslpcfi specification also switched to using a temporary register (x7) for the expected type hash instead of specifying a dedicated register, so I don't believe we have issues with calling conventions in this case.

I wonder whether we can move the if-condition part of the expanded code sequence (i.e. if type-hashes mismatch; crash) to ClangCodeGen (more like`-fsanitize=function`), and change the "kcfi" operand bundle to focus on expanding to a desired trap instruction (ud2 on x86-64).
On the plus side, this gives optimizers more opportunities to place trap basic blocks to cold regions.
On the downside, we cannot assume the code sequence is contiguous but that may be fine.

The initial versions of KCFI actually had the codegen implemented in Clang, but Linux maintainers wanted the exact same instruction sequence for each check, which is why the lowering was moved to the back-end. It also wasn't just about the specific trap instruction, we spent considerable amount of time fine tuning the code that's generated and ensured it's emitted immediately before the call instructions (e.g. see the thread starting at https://lore.kernel.org/lkml/87o7xmup5t.ffs@tglx/).

That being said, on AArch64 we actually do encode register information to the immediate in the trap instruction (so the kernel can produce a useful error message), but on X86 (and in future, RISC-V) the kernel's trap handling code instead needs to look at the instructions precending the trap and decode register information from them, which again relies on the expected instructions being there:

https://elixir.bootlin.com/linux/v6.4-rc5/source/arch/x86/kernel/cfi.c#L16

The kernel also relies on a stable instruction sequence for runtime patching of alternative CFI schemes (FineIBT on X86 systems that support IBT) and runtime re-randomization of the hashes, for example:

https://elixir.bootlin.com/linux/v6.4-rc5/source/arch/x86/kernel/alternative.c#L702

While Zisslpcfi hasn't yet been ratified, a similar runtime patching scheme with KCFI may be preferable there in future so it's possible to ship a single kernel binary that still can use the potentially stronger hardware-assisted CFI scheme when hardware support is available.

MaskRay added a comment.EditedJun 7 2023, 9:30 PM

Thanks for the description. The code generally looks good.

If the kernel wants a specific code sequence, implementing conditions in the LLVMCodeGen looks good to me. Consider adding a link to the discussion.

I wonder whether it will be more useful to update the summary/commit message to give more context to those who are unaware of -fsanitize=kcfi:

This patch implements KCFI operand bundle lowering to generate contiguous code sequences and produce .kcfi_traps sections.

  • clang emits "kcfi" operand bundles
  • SelectionDAGBuilder::visitCall visits the indirect CallInst and builds a SDNode using XXXTargetLowering::LowerCall.
  • In RISCVPassConfig::addPreSched, add a KCFI pass to emit a KCFI_CHECK target-specific pseudo instruction before an indirect call instruction.
  • In RISCVAsmPrinter, lower the KCFI_CHECK pseudo instructions to a contiguous code sequence that crash if the type hashes don't match.

Without the patch, there is no .kcfi_traps and the type hash comparison code sequence may not be contiguous due to basic block placement.

Needs some thoughts to merge the above paragraphs with the existing one "Similarly to the target-specific lowering added in D119296 [...]"

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
291

The prevailing style doesn't add parens for return.

296

As a minor optimization to skip one redundant isRegAvailable call: while (!isRegAvailable(++NextReg));

302

This assert seems redundant as Reg != AddrReg is very close.

316

Just check STI.hasStdExtCOrZca()

332

int64_t is the return type of getImm, not the original KCFI_CHECK operand type. Casting this to a uint32_t is perhaps clearer.

335

Add {} if the then body has more than one lines.

339

Is Hi20 == 0 tested?

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
15594

This switch should probably be turned to an assert. You can use llvm::is_contained if you don't want to write MBBI->getOpcode() twice.

assert(is_contained({RISCV::PseudoCALLIndirect, RISCV::PseudoTAILIndirect}, MBBI->getOpcode()));

15603

This assert seems redundant since we immediately call getReg with an internal assert.

llvm/test/CodeGen/RISCV/kcfi-patchable-function-prefix.ll
5

f1/f2/f3/f4 have the same shape. Without increasing the number of functions, we can make some disturbance (e.g. add one extra indirect call, use inline asm to clobber some registers) to check various cases without using .mir tests. .mir tests are fine, but the signal-to-noise ratio is not great and readers can be lost in the paramemters...

MaskRay added inline comments.Jun 7 2023, 9:33 PM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
13924

If CLI.CFIType != 0, is CLI.CB->isIndirectCall() possible? If not, add an assert?
This is an opportunity to save a redundant check.

samitolvanen marked 9 inline comments as done.

Addressed feedback.

samitolvanen edited the summary of this revision. (Show Details)Jun 14 2023, 1:17 PM

Needs some thoughts to merge the above paragraphs with the existing one

Great points. I updated the commit message and tried to capture most of these, PTAL.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
296

Reg != NextReg here, and using while (!isRegAvailable(++NextReg)) would always skip the first alternative register by first incrementing NextReg before calling isRegsAvailable. What am I missing?

332

I don't think casting this to uint32_t make sense since we'd have to cast it back to a larger type immediately below (to prevent + 0x800 from overflowing). I changed the comment to indicate this is a 32-bit value though.

339

Good catch, added a test.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
13924

Sure. CFIType should be set only for indirect calls when we reach this point.

llvm/test/CodeGen/RISCV/kcfi-patchable-function-prefix.ll
5

There are four functions because we're testing type hash locations in relation to patchable-function-prefix nops in four different cases, in addition to the code generated for KCFI checks. I'm not sure there's a way to do this with fewer functions.

MaskRay accepted this revision.Jun 14 2023, 3:35 PM

Will be good to wait a few days to give regular RISC-V backend developers a chance to leave any final comments.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
296

Ah you are right. Didn't notice the Reg/nextReg difference.

This revision is now accepted and ready to land.Jun 14 2023, 3:35 PM
This revision was automatically updated to reflect the committed changes.