| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <dbf9a593-e19d-4f53-96d0-d067868f40b5@gmail.com>
Date: Mon, 15 Sep 2025 21:40:53 -0600
From: Jeff Law <jeffreyalaw@...il.com>
To: Kees Cook <kees@...nel.org>, Qing Zhao <qing.zhao@...cle.com>
Cc: Andrew Pinski <pinskia@...il.com>, Richard Biener <rguenther@...e.de>,
Joseph Myers <josmyers@...hat.com>, Jan Hubicka <hubicka@....cz>,
Richard Earnshaw <richard.earnshaw@....com>,
Richard Sandiford <richard.sandiford@....com>,
Marcus Shawcroft <marcus.shawcroft@....com>,
Kyrylo Tkachov <kyrylo.tkachov@....com>, Kito Cheng <kito.cheng@...il.com>,
Palmer Dabbelt <palmer@...belt.com>, Andrew Waterman <andrew@...ive.com>,
Jim Wilson <jim.wilson.gcc@...il.com>, Peter Zijlstra
<peterz@...radead.org>, Dan Li <ashimida.1990@...il.com>,
Sami Tolvanen <samitolvanen@...gle.com>,
Ramon de C Valle <rcvalle@...gle.com>, Joao Moreira
<joao@...rdrivepizza.com>, Nathan Chancellor <nathan@...nel.org>,
Bill Wendling <morbo@...gle.com>, gcc-patches@....gnu.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 6/7] riscv: Add RISC-V Kernel Control Flow Integrity
implementation
On 9/4/25 18:24, Kees Cook wrote:
> Implement RISC-V-specific KCFI backend.
>
> - Function preamble generation using .word directives for type ID storage
> at offset from function entry point (no alignment NOPs needed due to
> fix 4-byte instruction size).
>
> - Scratch register allocation using t1/t2 (x6/x7) following RISC-V
> procedure call standard for temporary registers.
>
> - 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
>
> 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.
>
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---
> gcc/config/riscv/riscv-protos.h | 3 +
> gcc/config/riscv/riscv.cc | 147 ++++++++++++++++++++++++++++++++
> gcc/config/riscv/riscv.md | 74 ++++++++++++++--
> gcc/doc/invoke.texi | 13 +++
> 4 files changed, 231 insertions(+), 6 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv-protos.h b/gcc/config/riscv/riscv-protos.h
> index 2d60a0ad44b3..0e916fbdde13 100644
> --- a/gcc/config/riscv/riscv-protos.h
> +++ b/gcc/config/riscv/riscv-protos.h
> @@ -126,6 +126,9 @@ extern bool riscv_split_64bit_move_p (rtx, rtx);
> extern void riscv_split_doubleword_move (rtx, rtx);
> extern const char *riscv_output_move (rtx, rtx);
> extern const char *riscv_output_return ();
> +extern rtx riscv_maybe_wrap_call_with_kcfi (rtx, rtx);
> +extern rtx riscv_maybe_wrap_call_value_with_kcfi (rtx, rtx);
> +extern const char *riscv_output_kcfi_insn (rtx_insn *, rtx *);
> extern void riscv_declare_function_name (FILE *, const char *, tree);
> extern void riscv_declare_function_size (FILE *, const char *, tree);
> extern void riscv_asm_output_alias (FILE *, const tree, const tree);
> @@ -11346,6 +11347,149 @@ riscv_convert_vector_chunks (struct gcc_options *opts)
> return 1;
> }
>
> +/* 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?
> +}
> +
> +/* Apply KCFI wrapping to call_value pattern if needed. */
> +rtx
> +riscv_maybe_wrap_call_value_with_kcfi (rtx pat, rtx addr)
Similarly here.
> +
> +/* Output the assembly for a KCFI checked call instruction. */
> +const char *
> +riscv_output_kcfi_insn (rtx_insn *insn, rtx *operands)
And here.
> +{
> + /* 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?
> +
> + /* Calculate typeid offset from call target. */
> + HOST_WIDE_INT offset = -(4 + kcfi_patchable_entry_prefix_nops);
> +
> + /* Choose scratch registers that don't conflict with target. */
> + unsigned temp1_regnum = T1_REGNUM;
> + unsigned temp2_regnum = T2_REGNUM;
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.
> +
> + /* 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?
> +
> + /* Execute the indirect call. */
> + if (SIBLING_CALL_P (insn))
> + {
> + /* Tail call uses x0 (zero register) to avoid saving return address. */
> + temp_operands[0] = gen_rtx_REG (DImode, 0); /* x0 */
> + temp_operands[1] = target_reg; /* target register */
> + temp_operands[2] = const0_rtx;
> + output_asm_insn ("jalr\t%0, %1, %2", temp_operands);
> + }
> + else
> + {
> + /* Regular call uses x1 (return address register). */
> + temp_operands[0] = gen_rtx_REG (DImode, RETURN_ADDR_REGNUM); /* x1 */
> + temp_operands[1] = target_reg; /* target register */
> + temp_operands[2] = const0_rtx;
> + output_asm_insn ("jalr\t%0, %1, %2", temp_operands);
> + }
More cases where we probably should be using Pmode.
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?
> rtx target = riscv_legitimize_call_address (XEXP (operands[0], 0));
> - emit_call_insn (gen_sibcall_internal (target, operands[1], operands[2]));
> + rtx pat = gen_sibcall_internal (target, operands[1], operands[2]);
> + pat = riscv_maybe_wrap_call_with_kcfi (pat, target);
> + emit_call_insn (pat);
> DONE;
> })
>
> +;; KCFI sibling call - matches KCFI wrapper RTL
> +(define_insn "*kcfi_sibcall_insn"
> + [(kcfi (call (mem:SI (match_operand:DI 0 "call_insn_operand" "l"))
> + (match_operand 1 ""))
> + (match_operand 3 "const_int_operand"))
> + (use (unspec:SI [(match_operand 2 "const_int_operand")] UNSPEC_CALLEE_CC))]
> + "SIBLING_CALL_P (insn)"
I think the DI for the memory operand should probably be :P instead so
that we're not so tied to rv64.
> +;; KCFI sibling call with return value - matches KCFI wrapper RTL
> +(define_insn "*kcfi_sibcall_value_insn"
> + [(set (match_operand 0 "")
> + (kcfi (call (mem:SI (match_operand:DI 1 "call_insn_operand" "l"))
> + (match_operand 2 ""))
> + (match_operand 4 "const_int_operand")))
> + (use (unspec:SI [(match_operand 3 "const_int_operand")] UNSPEC_CALLEE_CC))]
> + "SIBLING_CALL_P (insn)"
> +{
> + return riscv_output_kcfi_insn (insn, &operands[1]);
> +}
> + [(set_attr "type" "call")
> + (set_attr "length" "24")])
Similarly for this pattern.
>
> +;; KCFI indirect call - matches KCFI wrapper RTL
> +(define_insn "*kcfi_call_internal"
> + [(kcfi (call (mem:SI (match_operand:DI 0 "call_insn_operand" "l"))
> + (match_operand 1 "" ""))
> + (match_operand 3 "const_int_operand"))
> + (use (unspec:SI [(match_operand 2 "const_int_operand")] UNSPEC_CALLEE_CC))
> + (clobber (reg:SI RETURN_ADDR_REGNUM))]
> + "!SIBLING_CALL_P (insn)"
> +{
> + return riscv_output_kcfi_insn (insn, operands);
> +}
> + [(set_attr "type" "call")
> + (set_attr "length" "24")])
And this one.
>
> +;; KCFI call with return value - matches KCFI wrapper RTL
> +(define_insn "*kcfi_call_value_insn"
> + [(set (match_operand 0 "" "")
> + (kcfi (call (mem:SI (match_operand:DI 1 "call_insn_operand" "l"))
> + (match_operand 2 "" ""))
> + (match_operand 4 "const_int_operand")))
> + (use (unspec:SI [(match_operand 3 "const_int_operand")] UNSPEC_CALLEE_CC))
> + (clobber (reg:SI RETURN_ADDR_REGNUM))]
> + "!SIBLING_CALL_P (insn)"
> +{
> + return riscv_output_kcfi_insn (insn, &operands[1]);
> +}
> + [(set_attr "type" "call")
> + (set_attr "length" "24")])
THis one too.
64).
>
> +On 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 4-byte instruction alignment eliminates the need for
> +additional padding NOPs. When used with @option{-fpatchable-function-entry},
> +the type identifier is placed before any patchable NOPs.
Note that many designs implement the "C" extension and as a result only
have a 2 byte alignment for instructions.
Jeff
Powered by blists - more mailing lists