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

[Jeff Law <jeffreyalaw@gmail.com>]

On 9/16/25 12:04 AM, Kees Cook wrote:

>>> +/* Apply KCFI wrapping to call pattern if needed.  */
>>> +rtx
>>> +riscv_maybe_wrap_call_with_kcfi (rtx pat, rtx addr)
>> So our coding standards require a bit more for that function comment. What
>> are PAT and ADDR and how are they used?
> 
> Sure, I can document those more fully in the next version.
Thanks.


> 
>>> +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 = (uint32_t) INTVAL (operands[3]);
>> Do we know operands[3] is a CONST_INT?
> 
> Yes, these all come from their respective RTL's:
> 
>           (match_operand 3 "const_int_operand"))
Perfect.  Just wanted to make sure.  It's a fairly common goof to try to 
extract an integer value from a non-integer node.


>> ISTM that this will need some kidn of adjustment if someone were to compile
>> with -ffixed-reg.  Maybe all we really need is a sorry() so that if someone
>> were to try to fix the temporary registers they'd get a loud complaint from
>> teh compiler.
> 
> Yeah, this needs more careful management of the scratch registers. I
> have not been able to find a sane way to provide working constraints to
> the RTL patterns, but I'd _much_ rather let the register allocator do
> all this work.
Usually you end up having to define a register class with the single 
register you want.  Of course once you do that you also need to start 
defining union classes and you also have to audit all kinds of code to 
make sure it's doing something sensible.  Yea, it's painful.


> 
>>> +
>>> +  /* 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);
>> Rather than using DImode for the PLUS, shouldn't it instead use Pmode so
>> that it at least tries to work on rv32?  Or is this stuff somehow defined as
>> only working for rv64?
> 
> It was designed entirely for rv64. I'm not against making it work with
> rv32, but I just haven't tried or tested it there.
ACK.  This may never end up being used on rv32.  But we should at least 
fix the obvious stuff since it's just the right thing to do.

Conceptually any pointer should be using Pmode.  If you keep that in 
mind, that covers one big blob of issues.  It also means that if someone 
where to try to light up 32 bit pointers on rv64 that your code is ready 
for that (and yes, we've had those kinds of requests, though to date 
none of that code has been ready to integrate).


>>
>> We generally prefer to not generate assembly code like you've done, but
>> instead prefer to generate actual RTL.  Is there some reason why you decided
>> to use output_asm_insn rather than generating RTL and letting usual
>> mechanisms for generating assembly code kick in?
> 
> Yeah, I covered this a bit in patch #2 in the series which describe the
> design requirements. The main issue is that the typeid validation check
> cannot be separated from the call, and the instruction pattern needs to
> have very close control over the register usage so we don't introduce
> any new indirect call gadgets (pop %target, call %target).
> 
> So, this is a replacement of the regular CALL rtl pattern. I am totally
> open to any other way to do this. I have been bumbling around in here
> (and on the other architectures) trying to find ways to make it all
> work, but it still feels like a bit of a hack. :)
I didn't really see anything in patch#2 which would indicate we want to 
generate blobs of assembly code.  It feels like there's something 
missing in both our understandings.





> 
> Okay, noted. Are there any restrictions on function pointer alignment?
> Regardless, I should probably rewrite this language a bit to try to
> better say "we don't care about alignment padding since the preamble
> typeid contents are a multiple of instruction size" (which would still
> be true for 2 byte alignemnt).
Nope.  The architecture will require them to be 2 byte aligned if "C" is 
enabled or 4 byte aligned if "C" is not enabled.  In both cases those 
alignments correspond to the minimum instruction size.

Jeff