Message-ID: <A356F147-5B23-46F6-B05E-3BDDC81A768B@oracle.com>
Date: Thu, 11 Sep 2025 15:04:01 +0000
From: Qing Zhao <qing.zhao@...cle.com>
To: Kees Cook <kees@...nel.org>
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 Sep 10, 2025, at 23:05, Kees Cook <kees@...nel.org> wrote:
> 
> 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.
> 
When determining whether two callsites should be merged, is it feasible to adding the different type_id from the
attributes into consideration? 
>>> +
>>> +- 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)

Okay, I see.

> 
>> 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.

Okay, this sounds like a reasonable justification for these additional temporaries 
and assignment stmts. 
One more question, are these additional temporaries and assignment stmts are
finally eliminated by later optimizations? Any runtime overhead due to them?


> 
>>> +
>>> +- 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.

Okay, is this documented somewhere? 
> 
>>> +/* 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:

Yes, that’s right. -:)
> 
> 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);
Okay.  (I guess that the mail client has some issues….)
> 
>>> +  /* 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.

Yeah, I usually use contrib/check_GNU_style.py to cleanup the code before
submitting the patch.
> 
>>> +/* 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).
During IPA, the typeid is attached to the function type through “set_function_kcfi_type_id” for
each function in the callgraph. 

For each indirect callsite in the above, the routine “create_kcfi_wrapper_type” also attaches
the typeid to the original_fn_type, and at the same time, create a new wrapper_type with a type_name
embedding the typeid. 

So, I feel the type_id information is carried redundantly here. 


>>> +  // 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)
Yeah, the gimples are correct. 
The concerns I have is, whether these additional temporizes and stmts impact
Optimization and also incur any runtime overhead. 

> 
> 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

Might need to study a little bit here to see whether better solution is possible without
These additional temporizes and stmts.

Qing
> 
> My other test methodology is "does Linux boot?" ;)
> 
> -- 
> Kees Cook

Powered by blists - more mailing lists