[PATCH] D148385: [RISCV] Implement KCFI operand bundle lowering

Tue Jun 6 08:59:22 PDT 2023

samitolvanen added a comment.

In D148385#4397995 <https://reviews.llvm.org/D148385#4397995>, @MaskRay wrote:

> `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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148385/new/

https://reviews.llvm.org/D148385