Message-ID: <202509112321.BFBE82ABBA@keescook>
Date: Fri, 12 Sep 2025 00:32:32 -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 Thu, Sep 11, 2025 at 03:04:01PM +0000, Qing Zhao wrote:
> 
> 
> > 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? 

This is basically what was happening in the RFC, but I kept finding new
corner cases in various passes, so it felt like whack-a-mole. Using the
wrapper appeared to solve it across the board with no special casing.

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

Yeah, they totally vanish as far as I've been able to determine.

> >>> +/* 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? 

Ah, whoops, no. I have added a note to the "no_sanitize" function attribute
docs for v3.

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

Thanks!

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

Ah! Yes, sorry, I see what you mean: the tail portion of
create_kcfi_wrapper_type! Yeah, this is kind of a oversight from
switching to the IPA pass. I was attaching the typeid to DECLs in IPA
and TYPEs in GIMPLE (create_kcfi_wrapper_type). The DECLs were used for
preambles, and the TYPEs were used for RTL expansion. I will attempt to
merge these; they should all be on the TYPE.

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

Thanks!

-- 
Kees Cook

Powered by blists - more mailing lists