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

[Jeff Law <jeffrey.law@oss.qualcomm.com>]

On 10/5/26 9:40 AM, Kees Cook wrote:
> Implement RISC-V-specific KCFI backend. This is rv64-only: the emitted
> typeid construction uses addiw, which exists only on RV64/RV128. An rv32
> backend would need an alternate sequence (e.g. addi); since the only
> current user of KCFI on riscv is the rv64 build of the Linux kernel, the
> support hook rejects rv32.
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.


>
> - Scratch register allocation using t1/t2 (x6/x7) following RISC-V
>    procedure call standard for temporary registers (already
>    caller-saved), and t3 (x28) 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
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.

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.


>
> Build and run tested with Linux kernel ARCH=riscv.
>
> Assisted-by: LLM [tests]
>
> 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-direct-call-shapes.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-trap-section-per-func.c: Add riscv patterns.
> 	* gcc.dg/kcfi/kcfi-riscv-32bit.c: New test.
> 	* 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 <kees@kernel.org>
> ---
>
>
>
>
>
> +
> +/* 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[2] 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)
> +{
> +  /* Target register.  */
> +  rtx target_reg = operands[0];
> +  gcc_assert (REG_P (target_reg));
> +
> +  /* Get KCFI type ID.  */
> +  uint32_t expected_type = UINTVAL (operands[2]);
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 
doesn't matter in practice here, but  if you're pulling data out via 
[U]INTVAL, then destination data type should generally be a 
HOST_WIDE_INT or unsigned variant of the same.
> +
> +  /* Calculate typeid offset from call target.  */
> +  HOST_WIDE_INT offset = -kcfi_get_typeid_offset ();
> +
> +  /* Choose scratch registers that don't conflict with target.  */
> +  unsigned temp1_regnum = T1_REGNUM;
> +  unsigned temp2_regnum = T2_REGNUM;
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.




> +
> +  rtx temp_operands[3];
> +
> +  /* The check sequence (typeid load through indirect jump) must be emitted
> +     as a single atomic unit: a mismatch must be caught before the jalr
> +     executes, with no opportunity for the linker or assembler to interleave
> +     anything.  Disable linker relaxation (which can rewrite jalr/call forms
> +     and shift offsets) and the C extension (which can shrink instructions
> +     and perturb sizes the length attribute reports) for the duration.  */
> +  output_asm_insn (".option push", operands);
> +  output_asm_insn (".option norelax", operands);
> +  output_asm_insn (".option norvc", operands);
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.



> +
> +  /* Load actual type from memory at offset.  */
> +  temp_operands[0] = gen_rtx_REG (SImode, temp1_regnum);
> +  temp_operands[1] = gen_rtx_MEM (SImode,
> +				  gen_rtx_PLUS (DImode, target_reg,
> +						GEN_INT (offset)));
> +  output_asm_insn ("lw\t%0, %1", temp_operands);
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 
destination and memory source.  So instead of output_asm_insn, you'd do 
something like:


emit_move_insn (temp_operands[0], temp_operands[1]);


> +
> +  /* Load expected type using lui + addiw for proper sign extension.  */
> +  temp_operands[0] = gen_rtx_REG (SImode, temp2_regnum);
> +  temp_operands[1] = GEN_INT (hi20);
> +  output_asm_insn ("lui\t%0, %1", temp_operands);
> +
> +  temp_operands[0] = gen_rtx_REG (SImode, temp2_regnum);
> +  temp_operands[1] = gen_rtx_REG (SImode, temp2_regnum);
> +  temp_operands[2] = GEN_INT (lo12);
> +  output_asm_insn ("addiw\t%0, %1, %2", temp_operands);
All that turns into dest = force_reg (SImode, GEN_INT (expected_type));


> +
> +  /* Output conditional branch to call label.  */
> +  fprintf (asm_out_file, "\tbeq\t%s, %s, ",
> +	   reg_names[temp1_regnum], reg_names[temp2_regnum]);
> +  assemble_name (asm_out_file, call_name);
> +  fputc ('\n', asm_out_file);
And you'd emit a conditional branch here via emit_jump_insn after 
generating appropriate RTL.

> +
> +  /* Output trap label and ebreak instruction.  */
> +  ASM_OUTPUT_LABEL (asm_out_file, trap_name);
> +  output_asm_insn ("ebreak", operands);
This is a "trap" insn.  So

emit_insn (gen_trap ()); or something along those lines.


> +
> +  /* Use common helper for trap section entry.  */
> +  rtx trap_label_sym = gen_rtx_SYMBOL_REF (Pmode, trap_name);
> +  kcfi_emit_traps_section (asm_out_file, trap_label_sym, labelno);
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.


If you're rewriting the trap, then I'd probably do somethign like

1. Define a new insn that has the same basic properties as a barrier, 
but emits the .option thingies at the start of the sequence.
2. Define another new insn also with barrier semantics that emits the 
option pop.
3. Convert riscv_output_kcfi_insn to emit RTL instead and hook into the 
define_expands.  It'll need to generate the insns you created in steps 
#1 and #2.

Note that steps #1 and #2 would in turn allow others to simplify other 
parts of the RISC-V port as well.  Not the main motivation, but worth 
noting.


> +
>   /* 'Unpack' up the internal tuning structs and update the options
>       in OPTS.  The caller must have set up selected_tune and selected_arch
>       as all the other target-specific codegen decisions are
> @@ -17360,6 +17535,30 @@ riscv_memtag_tag_bitsize ()
>   #undef TARGET_MEMTAG_TAG_BITSIZE
>   #define TARGET_MEMTAG_TAG_BITSIZE riscv_memtag_tag_bitsize
>   
> +/* Return true if the target supports KCFI.
> +   KCFI requires 64-bit mode and the T1, T2, and T3 registers.  */
> +
> +static bool
> +riscv_kcfi_supported_p (void)
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 
problem.  There are ways to identify multiple entry points, but those 
assume you've got a control flow graph for the current function.  So 
they probably won't work here and we can't really check for what 
language is in use (think about LTO).

>   
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index f0b1314d1731..b1ebae37ed89 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -17631,6 +17631,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.
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.


>    When used with @option{-fpatchable-function-entry},
> +the type identifier is placed before any prefix NOPs.  The runtime check
> +loads the actual type using @code{lw t1, OFFSET(target_reg)}, where the
Is the offset always at -4?  Just helps me understand the underlying 
assumptions you need to make.

Jeff