Message-ID: <202509101705.92474D66@keescook>
Date: Wed, 10 Sep 2025 20:05:11 -0700
From: Kees Cook <kees@...nel.org>
To: 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" <gcc-patches@....gnu.org>,
	"linux-hardening@...r.kernel.org" <linux-hardening@...r.kernel.org>
Subject: Re: [PATCH v2 2/7] kcfi: Add core Kernel Control Flow Integrity
 infrastructure

On Tue, Sep 09, 2025 at 06:49:22PM +0000, Qing Zhao wrote:
> 
> > On Sep 4, 2025, at 20:24, Kees Cook <kees@...nel.org> wrote:
> > +For indirect call sites:
> > +
> > +- Keeping indirect calls from being merged (see above) by adding a
> > +  wrapping type so that equality was tested based on type-id.
> 
> I still think that the additional new created wrapping type and the new assignment stmt
> 
> wrapper_tmp = (wrapper_ptr_type) fn
> is not necessary.
> 
> All the information can be get from function type + type-id which is attached as an attribute
> to the original_function_type of “fn”. 
> Could you explain why the wrapper type and the new temporary, new assignment is 
> necessary?

I couldn't find a way to stop merging just using the attributes. I need
a way to directly associated indirect call sites with the typeid.

> > +
> > +- Keeping typeid information available through to the RTL expansion
> > +  phase was done via a new KCFI insn that wraps CALL and the typeid.
> 
> Is the new KCFI insn the following:
> wrapper_tmp = (wrapper_ptr_type) fn?

This bullet is speaking about the backend change to support the KCFI
check-call insn sequences:

+/* KCFI wrapper for call expressions.
+   Operand 0 is the call expression.
+   Operand 1 is the KCFI type ID (const_int).  */
+
+DEF_RTL_EXPR(KCFI, "kcfi", "ee", RTX_EXTRA)

> Why the type-id attached as the attribute is not enough? 

Doing the wrapping avoided needing to update multiple optimization passes
to check for the attribute. And it still needed a way to distinguish
between direct and indirect calls, so I need to wrap only the indirect
calls, where as the typeid attribute is for all functions for all typeid
needs, like preamble generation, etc.

> > +
> > +- To make sure KCFI expansion is skipped for inline functions, the
> > +  inlining is marked during GIMPLE with a new flag which is checked
> > +  during expansion.
> > +
> > +For indirect call targets:
> > +
> > +- kcfi_emit_preamble() uses function_needs_kcfi_preamble(),
> > +  to emit the preablem,
> Typo: preamble. 

Fixed.

> > +HOST_WIDE_INT kcfi_patchable_entry_prefix_nops = 0;  /* For callsite offset */
> > +static HOST_WIDE_INT kcfi_patchable_entry_arch_alignment_nops = 0;  /* For preamble alignment */
> > +
> > +/* Common helper for RTL patterns to emit .kcfi_traps section entry.  */
> 
> there should be one empty line between the comments of the function and the start of the function. 
> I noticed that you need such empty line for all the new functions you added.  -:)

Oh! Wow, yeah, I totally missed this coding style requirement. Fixed.

> > +/* Check if a function needs KCFI preamble generation.
> > +   ALL functions get preambles when -fsanitize=kcfi is enabled, regardless
> > +   of no_sanitize("kcfi") attribute.  */
> 
> Why no_sanitize(“kcfi”) is not considered here?

no_sanitize(“kcfi”) is strictly about whether call-site checking
is performed within the function. It is not used to mark a function as
not being the target of a KCFI call.

> > +/* Get KCFI type ID for a function declaration during assembly output phase.
> > +   Fatal error if type ID was not previously set during IPA phase.  */
> > +static uint32_t
> > +get_function_kcfi_type_id (tree fndecl)
> > +{
> > +  if (!kcfi_type_id_attr)
> > +    kcfi_type_id_attr = get_identifier ("kcfi_type_id");
> > +
> > +  tree attr = lookup_attribute_by_prefix ("kcfi_type_id",
> > +   DECL_ATTRIBUTES (fndecl));
> > +  if (attr && TREE_VALUE (attr) && TREE_VALUE (TREE_VALUE (attr)))
> > +    {
> > +      tree value = TREE_VALUE (TREE_VALUE (attr));
> > +      if (TREE_CODE (value) == INTEGER_CST)
> > + return (uint32_t) TREE_INT_CST_LOW (value);
> The indentation of above is off.

I understand GCC code style indentation to be "leading spans of 8 spaces
should be replaced with a tab character". This is what I followed:

first line indents to column 6, so 6 spaces. Second line indents to
column 8, so 1 tab:

SSSSSSif (TREE_CODE (value) == INTEGER_CST)
Treturn (uint32_t) TREE_INT_CST_LOW (value);

This seems to match everywhere else? Randomly picking line get_section()
from varasm.cc, I see the same:

      if (not_existing)
	internal_error ("section already exists: %qs", name);

> > +  /* Calculate alignment NOPs needed */
> > +  arch_alignment = (alignment_bytes - ((kcfi_patchable_entry_prefix_nops + typeid_size) % alignment_bytes)) % alignment_bytes;
> The above line is too long. 

Oops, yes, thank you. Fixed.

What is the right tool for me to run to check for these kinds of code
style glitches? contrib/check_GNU_style.py doesn't report anything. Oh!
It takes _patches_ not _files_. The .sh version specifies "patch" in the
help usage. Okay, I will get this all passing cleanly.

> > +/* Wrap indirect calls with KCFI type for anti-merging.  */
> > +static unsigned int
> > +kcfi_instrument (void)
> > +{
> > +  /* Process current function for call instrumentation only.
> > +     Type ID setting is handled by the separate IPA pass.  */
> > +
> > +  basic_block bb;
> > +
> > +  FOR_EACH_BB_FN (bb, cfun)
> > +    {
> > +      gimple_stmt_iterator gsi;
> > +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > + {
> > +  gimple *stmt = gsi_stmt (gsi);
> > +
> > +  if (!is_gimple_call (stmt))
> > +    continue;
> > +
> > +  gcall *call_stmt = as_a <gcall *> (stmt);
> > +
> > +  // Skip internal calls - we only instrument indirect calls
> > +  if (gimple_call_internal_p (call_stmt))
> > +    continue;
> > +
> > +  tree fndecl = gimple_call_fndecl (call_stmt);
> > +
> > +  // Only process indirect calls (no fndecl)
> > +  if (fndecl)
> > +    continue;
> > +
> > +  tree fn = gimple_call_fn (call_stmt);
> > +  if (!is_kcfi_indirect_call (fn))
> > +    continue;
> > +
> > +  // Get the function type to compute KCFI type ID
> > +  tree fn_type = gimple_call_fntype (call_stmt);
> > +  gcc_assert (fn_type);
> > +  if (TREE_CODE (fn_type) != FUNCTION_TYPE)
> > +    continue;
> > +
> > +  uint32_t type_id = compute_kcfi_type_id (fn_type);
> > +
> > +  // Create KCFI wrapper type for this call
> > +  tree wrapper_type = create_kcfi_wrapper_type (fn_type, type_id);
> Again, the new “type_id” has been attached as an attribute of “fn_type” here, 

The attribute is attached during IPA. This is run before that, but as I
mentioned, this is the call-site handling, and the IPA pass is for
globally associating a type-id to the function for all other uses
(preambles, weak symbols, etc).

> > +  // Create a temporary variable for the wrapped function pointer
> > +  tree wrapper_ptr_type = build_pointer_type (wrapper_type);
> > +  tree wrapper_tmp = create_tmp_var (wrapper_ptr_type, "kcfi_wrapper");
> > +
> > +  // Create assignment: wrapper_tmp = (wrapper_ptr_type) fn
> > +  tree cast_expr = build1 (NOP_EXPR, wrapper_ptr_type, fn);
> > +  gimple *cast_stmt = gimple_build_assign (wrapper_tmp, cast_expr);
> > +  gsi_insert_before (&gsi, cast_stmt, GSI_SAME_STMT);
> > +
> 
> Why the additional wrapper_ptr_type, wrapper_tmp and new assignment stmt
> Are needed here?

Based on my understanding of the requirements for GIMPLE here is that I
needed a cast between the original and the wrapper, and an assignment
for SSA. It's converting the call like:

  Original: result = fn(arg1, arg2)
  Becomes:  result = wrapper(arg1, arg2, type_id)

I'm open to whatever alternative is needed here. I tried to capture the
merging issue with gcc/testsuite/gcc.dg/kcfi/kcfi-call-sharing.c

My other test methodology is "does Linux boot?" ;)

-- 
Kees Cook

Powered by blists - more mailing lists