Main things that stand out: - **Hard bug: wrong register number for `t3`.** - The patch says “t3 (x8)” in the cover text, and the code uses `T3_REGNUM` as fallback when target is `t1`/`t2`. - On RISC-V, `x8` is `s0/fp`, **not** `t3`. `t3` is `x28`. - If `T3_REGNUM` in GCC maps to architectural `t3` then the comment/email is wrong; if not, the implementation is wrong. This should be explicitly checked because using `s0` as a scratch across calls would be a real ABI violation.
- **`addiw` makes this rv64-only in a stronger way than stated.** - The patch claims “Nothing is conceptually rv64 specific”, but the emitted sequence uses `addiw`, which exists only on RV64/RV128, not RV32. - That’s consistent with the support hook rejecting 32-bit, but the wording is misleading. - **Mode usage looks suspicious in the md patterns.** - The call memory is written as `mem:SI` even for RV64: - `(call (mem:SI (match_operand:DI ...` - That matches existing patterns, so maybe okay, but the new KCFI forms also hardcode `match_operand:DI` for the target reg in all cases. Since support is gated to 64-bit only this is probably fine, but it is more brittle than necessary. - **No clobbers for scratch regs in the new insns.** - `riscv_output_kcfi_insn` unconditionally uses `t1`/`t2`/`t3`, but the `define_insn`s do not model those as clobbers. - Since these are emitted only in the final asm output path, GCC won’t know these registers are killed by the insn. That is usually dangerous unless the backend has an established convention for calls implicitly clobbering these hard regs and nothing live can be placed there across the call. - For caller-saved regs across a call this may happen to work, but it still feels under-modeled, especially for the period *before* the `jalr` within the same insn. I’d want confirmation from a maintainer that this is acceptable for a monolithic call-pattern output function. - **Direct/indirect detection may be too narrow.** - `is_direct_call = SYMBOL_REF_P (addr);` - Are all direct call targets guaranteed to still be plain `SYMBOL_REF` after `riscv_legitimize_call_address`? If some direct calls appear as `CONST`, `LABEL_REF`, etc., this would incorrectly wrap them with KCFI. - **Potential issue with constructing the expected type value.** - The code does: - `lui t2, hi20` - `addiw t2, t2, lo12` - This sign-extends to XLEN on RV64, and comparison is against the loaded `lw` result in `SImode`, so likely okay. - But using `output_asm_insn` with `GEN_INT (hi20)`/`GEN_INT (lo12)` depends on the printer accepting the exact immediate forms intended here. Probably fine, but worth sanity-checking with negative IDs / high-bit-set IDs. - **Trap label handling via `SYMBOL_REF` to an internal label is a bit odd.** - `rtx trap_label_sym = gen_rtx_SYMBOL_REF (Pmode, trap_name);` - If `kcfi_emit_traps_section` expects a symbol ref string name, okay; if it expects a label ref or some canonical internal-label RTX, this could be fragile. - **Doc typo/grammar:** - “Because of the use of these register, they cannot be fixed registers” → “registers”. Tests: - Good coverage overall. - The new negative tests expect both the target-specific error and the generic “sorry, unimplemented” message. That’s probably right for `TARGET_KCFI_SUPPORTED`, but these tests may be a bit brittle if the frontend wording changes. So the two most important review points are: 1. **Verify `T3_REGNUM` / `t3` really means architectural x28, not x8.** 2. **Be sure the backend is allowed to use hard scratch regs in `output_asm_insn` without explicit RTL clobbers.** On 5/15/2026 10:16 AM, Kees Cook wrote: > Implement RISC-V-specific KCFI backend. Nothing is conceptually rv64 > specific, but using an alternative set of instructions for rv32 would be > needed, and at present the only user of KCFI on riscv is the rv64 build > of the Linux kernel. > > - Scratch register allocation using t1/t2 (x6/x7) following RISC-V > procedure call standard for temporary registers (already > caller-saved), and t3 (x8) when either t1 or t2 is already the call > target register. > > - Incompatible with -ffixed-t1, -ffixed-t2, or -ffixed-t3. > > - Integration with .kcfi_traps section for debugger/runtime metadata > (like x86_64). > > Assembly Code Pattern for RISC-V: > lw t1, -4(target_reg) ; Load actual type ID from preamble > lui t2, %hi(expected_type) ; Load expected type (upper 20 bits) > addiw t2, t2, %lo(expected_type) ; Add lower 12 bits (sign-extended) > beq t1, t2, .Lkcfi_call ; Branch if types match > .Lkcfi_trap: ebreak ; Environment break trap on mismatch > .Lkcfi_call: jalr/jr target_reg ; Execute validated indirect transfer Right. So at least one of the rv64-isms is that addiw. I know this isn't necessarily supposed to work on rv32 and we probably don't have a full set of rv64-isms mapped out. But just wanted to get that one noted for the record. > > Build and run tested with Linux kernel ARCH=riscv. > > gcc/ChangeLog: > > config/riscv/riscv-protos.h: Declare KCFI helpers. > config/riscv/riscv.cc (riscv_maybe_wrap_call_with_kcfi): New > function, to wrap calls. > (riscv_maybe_wrap_call_value_with_kcfi): New function, to > wrap calls with return values. > (riscv_output_kcfi_insn): New function to emit KCFI assembly. > config/riscv/riscv.md: Add KCFI RTL patterns and hook expansion. > doc/invoke.texi: Document riscv nuances. > > gcc/testsuite/ChangeLog: > > * gcc.dg/kcfi/kcfi-adjacency.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-basics.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-call-sharing.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-complex-addressing.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-move-preservation.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-no-sanitize-inline.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-no-sanitize.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-offset-validation.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-patchable-entry-only.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-patchable-large.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-patchable-medium.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-patchable-prefix-only.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-tail-calls.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-trap-section.c: Add riscv patterns. > * gcc.dg/kcfi/kcfi-riscv-fixed-t1.c: New test. > * gcc.dg/kcfi/kcfi-riscv-fixed-t2.c: New test. > * gcc.dg/kcfi/kcfi-riscv-fixed-t3.c: New test. > > Signed-off-by: Kees Cook <[email protected]> > --- > > > > > +} > + > +/* Output the assembly for a KCFI checked call instruction. INSN is the > + RTL instruction being processed. OPERANDS is the array of RTL operands > + where operands[0] is the call target register, operands[3] is the KCFI > + type ID constant. Returns an empty string as all output is handled by > + direct assembly generation. */ > + > +const char * > +riscv_output_kcfi_insn (rtx_insn *insn, rtx *operands) It's still not clear to me why this is being output as a single atomic unit. Why not generate proper RTL for the various steps where it obviously can? > lw t1, -4(target_reg) ; Load actual type ID from preamble That's a standard SImode load most likely. > lui t2, %hi(expected_type) ; Load expected type (upper 20 bits) > addiw t2, t2, %lo(expected_type) ; Add lower 12 bits (sign-extended) Similarly. Also note if you use standard mechanisms to do this, then it should work for rv32 and rv64 trivially. > beq t1, t2, .Lkcfi_call ; Branch if types match Also a trivial insn, I don't offhand see a hard need emit this as part of an atomic sequence. > .Lkcfi_trap: ebreak ; Environment break trap on mismatch We can trivially define an insn for this if it doesn't exist. > .Lkcfi_call: jalr/jr target_reg ; Execute validated indirect transfer This the most worrisome. I suspect this needs to stay in a particular form and avoid things like linker relaxation and such. Point being I still see generation of this sequence as a single atomic unit as conceptually wrong. If you really need it as an atomic unit, then you need to be emitting the proper assembly directives around it to ensure the assembler/linker don't expand macros, don't relax, etc etc > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index f2c3fafbb428..79c3180d9184 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -17474,6 +17474,23 @@ allowing the kernel to identify both the KCFI > violation and the involved > registers for detailed diagnostics (eliminating the need for a separate > @code{.kcfi_traps} section as used on x86_64). > > +On 64-bit RISC-V, KCFI type identifiers are emitted as a @code{.word ID} > +directive (a 32-bit constant) before the function entry, similar to AArch64. > +RISC-V's natural instruction alignment eliminates the need for > +additional alignment NOPs. When used with > @option{-fpatchable-function-entry}, > +the type identifier is placed before any prefix NOPs. The runtime check Really? Note the C extension can result in instructions, including function entry points starting at a 2 byte boundary rather than a 4 byte boundary. Jeff
