Re: [PATCH v17 7/7] riscv: Add RISC-V Kernel Control Flow Integrity implementation

[Kees Cook <kees@kernel.org>]

On Thu, Oct 08, 2026 at 10:39:50PM -0600, Jeff Law wrote:
> Note that addiw explicitly sign extends the result from bit 32 out to
> bit 63.   That seems to match what you want with the implementation
> (since you do a lw do load the ID from the preamble and that's
> sign-extending from bit 32 to bit 63.

Right, both sides of the beq are sign-extended the same way, so the
comparison works for type IDs with bit 31 set.  I was suspicious of
the sign extension myself and had double-checked it at the edges
just to be sure when I first worked on this. :)

> So what I can't recall ever seeing is an explanation of why this
> sequence needs to be emitted as a single assembly block.  That's
> something we generally try to avoid.  Now if that design decision/need
> is covered in the talk from the Cauldron, that's fine.  I was
> intercepted and couldn't get there in time, but I'll certainly make a
> point to watch the whole thing.

You asked this back in v2, and my reply then[1] wasn't very helpful.
Sorry about that!  Here's hopefully more complete reasoning; v18 will
also put it in the design comment at the top of kcfi.cc, since you're
right that it was only asserted there.

1. The checked target must stay in the checked register until the call.
   If the register allocator spilled it after the check and reloaded it
   (or rematerialized it) before the call, memory an attacker can write
   would supply an unchecked target, effectively restoring the
   "pop %target; call %target" gadget that KCFI seeks to remove.
   Separate insns would give reload and the post-reload passes the
   freedom to create it again, which needs to be avoided.

2. The trap isn't noreturn. It is designed to be continuable: Linux
   has a permissive (warn-only) mode where the trap handler reports the
   mismatch and returns, and execution continues into the call right
   after it. So the trap can't be gen_trap() with a barrier after it,
   and the trap block must not be moved out of line by bb-reorder or
   hot/cold partitioning, even though it looks cold.

3. The kernel depends on the exact layout. (This is, of course,
   changeable, but it would require coordination with Linux and Clang.)
   Specifically on RISC-V, Linux's trap handler (arch/riscv/kernel/cfi.c)
   reads the beq just before the ebreak to find the register holding the
   expected type, and the jalr just after it to find the target. On x86,
   the kernel may rewrite every KCFI call site in place into the FineIBT
   sequence at boot, patching a fixed number of bytes before the call.

FWIW, Clang KCFI is built the same way: its RISC-V backend emits a
KCFI_CHECK pseudo that must immediately precede the call through the
same register (it asserts this), and expands it with specific scratch
registers when the assembly is printed (RISCVAsmPrinter::LowerKCFI_CHECK).

> And note that if it really needs to be an atomic sequence, there's still
> ways to achieve that that avoid the blobs of assembly code.  See below.

> Generally you don't want to be using types like uint32_t, uint64_t,
> etc.  Those are properties of the host, not the target.   Given that
> you're using "lw" and a sign extending "addiw", you probably want this
> to be a HOST_WIDE_INT (vs an unsigned HOST_WIDE_INT).  It probably

Ah, yes. This is now fixed in v18: RISC-V will hold it as a HOST_WIDE_INT,
sign-extended from bit 31 to match the lw and addiw, and the other three
backends will hold it as an unsigned HOST_WIDE_INT. The generated code
doesn't change.

I went through the rest of the series for the same problem. The type ID
itself will stay a uint32_t: the ABI defines it as a 32-bit FNV-1a hash,
computed on the host, so that's the arithmetic it needs. Now
__builtin_linux_abi_kcfi_hash will return a 32-bit unsigned type even
where int is 16 bits, and the type ID's size in the preamble will be a
named constant instead of sizeof (uint32_t).

> Why use fixed registers?   It would seem to work better if you just
> generated pseudos and let the register allocator do the right thing and
> assign them to whatever temporary is best.  That would also remove the
> restrictions around -ffixed-reg.

I'd love to, but the KCFI check is part of/within the call insn and LRA
allows no output reloads in a CALL_INSN:

	/* CALL_INSNs are not allowed to have any output reloads.  */
	if (CALL_P (curr_insn))
	  no_output_reloads_p = true;

So under register pressure the allocator can't provide a scratch register
for the call pattern, so I had to have each backend instead pick scratch
registers that are dead at the call (caller-saved temporaries that aren't
argument registers or the target). I went through this with Andrea for
AArch64[2][3]. Moving the check out into separate insns, so that pseudos
would work, runs into point 1 above.

And trying to create an entirely new mode of register allocation for
use within a CALL_INSN seemed like serious over-kill, given the other
narrow constraints on KCFI.

The -ffixed-reg restriction could go, though: Clang uses t1 and t2 and
falls back to t3 through t6, skipping any the user reserved, so it never
has to reject fixed registers.  I could do the same here, but I had so
much pain making the ARM backend do this that I didn't want to tackle
it again. :P  But I can if you'd like to see that here too.

> Why does this need to be an atomic sequence with restrictions around
> relaxing and compression avoidance?  From other comments I'm guessing
> you're rewriting that ebreak.  At the least you need to explain why its
> an atomic sequence.

The atomicity is for the reasons above; the kernel doesn't rewrite the
ebreak on RISC-V (x86 is where call sites get rewritten).

The directives came from your v12 review[4], where you said that if the
sequence really must be atomic, it should be wrapped in directives so
the assembler and linker don't expand macros or relax. However, in
looking at it more closely now, neither can affect this sequence:

- Nothing in it can be relaxed: the lw is register-relative, the lui and
  addiw take plain constants with no relocations, the beq goes to a
  local label, and the jalr is through a register. The linker only
  relaxes symbol-based sequences marked R_RISCV_RELAX.

- None of the instructions is an assembler macro.

- Compression is harmless: the assembler may use c.lui, c.addiw, c.jalr,
  or c.ebreak, but the beq can never be compressed (t1 and t2 aren't in
  the compressed register set, and c.beqz only compares against zero),
  and the kernel's decoder reads the beq at epc - 4, accepts jalr or
  c.jalr, and steps over the ebreak by its actual length.

So v18 will drop ".option norelax" and ".option norvc", which also lets
the sequence shrink with C, and matches Clang, which has none of these
directives either.

> So if we really don't need to generate an atomic sequence, then you've
> got the skeleton for generating RTL here.  You've got the register

> This is a "trap" insn.  So
>
> emit_insn (gen_trap ()); or something along those lines.

As in point 2, it can't be a trap with the usual noreturn semantics,
since execution continues from it into the call in permissive mode.

> So do you do something fun like rewrite the trap?  Is that why you're
> generating the atomic sequence?  ANd if so, we can still get you an
> atomic sequence without emitting blobs of assembly code via emit_barrier
> () at the start and end of the atomic sequence.

Marker insns at the start and end would keep the scheduler from moving
things across them, but they wouldn't stop reload from inserting spill
code between the check and the call (point 1), and the beq would split
the sequence into basic blocks that bb-reorder and partitioning are free
to rearrange (point 2). If there's a way to pin all of that down, I'm
happy to try it, but it seemed to fight the compiler at every step,
while emitting the sequence from the call pattern's output routine
appears to be the simplest and most robust way to get the right output.
(And, FWIW, Clang ended up in the same place too.)

> We probably need to avoid for C++ due to thunks and Fortran due to
> multiple entry points.  And you may not have a good way to test for
> those, particularly in an LTO build.  We need to think about that

KCFI is C-only: process_options() rejects -fsanitize=kcfi for any other
front end. And while I tried to enforce this under LTO too, I did now
find a way that it fails. The check for language accepted LTO's
"GNU GIMPLE", but that meant a C++ object built with -flto and linked
with -fsanitize=kcfi would crash on a METHOD_TYPE. v18 will check the
language of every translation unit the IL came from
(TRANSLATION_UNIT_LANGUAGE survives streaming), and reject anything that
isn't C, reporting the offending file:

  thunk.cc:3:49: sorry, unimplemented: '-fsanitize=kcfi' is only supported for C, but 'thunk.cc' was compiled by the GNU C++20 front end

I tested this by hand (and that C-only LTO links are still instrumented),
but I couldn't find a way to add a regression test for it: the case needs
-fsanitize=kcfi at link time only, which means lto.exp, and lto.exp has
no way to expect a link to fail; any failed link is a FAIL. If there's
a way to do this that I've missed, I'd be happy to add one.

> Really?  Natural instruction alignment for the designs I expect to see
> almost everywhere is 16 bits (via the C extension which is mandatory for
> rva23 and used in nearly design I'm aware of).  So you've got a bit of a
> problem here as unaligned access isn't necessary guaranteed to work.
> You could do something like force the function alignment to 4 bytes when
> KFCI is on.  A bit hackish, but probably sensible in practice.

Oops, yes, thank you for persisting on this. :P  You mentioned it in v12
too, but I misunderstood it: ".option norvc" around the call sequence
did nothing for the alignment of the functions it calls. With the C
extension the preamble was only 2-byte aligned, so the lw could be
misaligned (and so could the ldr on Thumb at -Os). Clang aligns every
function that has a KCFI type ID to at least 4 bytes, on all targets,
"to avoid unaligned access". v18 will do the same, and fix that paragraph
of the documentation.

> Is the offset always at -4?  Just helps me understand the underlying
> assumptions you need to make.

It's -4 plus the size of any prefix NOPs from
-fpatchable-function-entry=N,M, since the type ID sits before them.
(Call sites can't see per-function patchable_function_entry attributes,
so KCFI rejects those and uses only the global option.)

But checking this turned up the bug you were predicting about NOP sizing
in your 2/7 review. I had sized each prefix NOP from the nop insn's length
attribute (4 bytes), but with the C extension the assembler emits c.nop
(2 bytes), so with prefix NOPs the call sites loaded from the wrong
offset. As you pointed out, lengths are only upper bounds. v18 will add
a TARGET_KCFI_NOP_SIZE hook for the size the assembler actually emits;
RISC-V will return 2 with C or Zca, matching Clang.  More on this in
the 2/7 reply...

Thank you for the review! :)

-Kees

[1] https://inbox.sourceware.org/gcc-patches/202509152254.8A38F96D91@keescook/
[2] https://inbox.sourceware.org/gcc-patches/202509141233.33B8C6D4@keescook/
[3] https://inbox.sourceware.org/gcc-patches/202509171251.BA32F4B@keescook/
[4] https://inbox.sourceware.org/gcc-patches/6ab27ac0-619b-4960-b37c-e83389d75760@qti.qualcomm.com/

-- 
Kees Cook