Re: [PATCH v13 2/7] kcfi: Add core Kernel Control Flow Integrity infrastructure

On Sat, Jun 27, 2026 at 03:57:36PM -0700, Andrea Pinski wrote:
> On Thu, Jun 18, 2026 at 1:45 PM Kees Cook <[email protected]> wrote:
> > [...]
> > diff --git a/gcc/kcfi.h b/gcc/kcfi.h
> > new file mode 100644
> > index 000000000000..ea7e7881040f
> > --- /dev/null
> > +++ b/gcc/kcfi.h
> > [...]
> > +#include "config.h"
> > +#include "system.h"
> > +#include "coretypes.h"
> > +#include "rtl.h"
> 
> This is wrong. Don't include config.h nor system.h in a header file.

Okay, removing.

> 
> > +
> > +/* Common helper for RTL patterns to emit .kcfi_traps section entry.
> > +   Call after emitting trap label and instruction with the trap symbol
> > +   reference.  */
> > +extern void kcfi_emit_traps_section (FILE *file, rtx trap_label_sym,
> > +                                     int labelno);
> > +
> > +/* Extract KCFI type ID from current GIMPLE statement.  */
> > +extern rtx internal_kcfi_get_type_id_for_expanding_gimple_call (void);
> > +
> > +/* Convenience wrapper to check for SANITIZE_KCFI.  */
> > +static inline rtx
> Remove static.

Okay. This is a Linux-ism in my finger, I think. :)

> > [...]
> > --- /dev/null
> > +++ b/gcc/kcfi.cc
> > @@ -0,0 +1,696 @@
> > +/* Kernel Control Flow Integrity (KCFI) support for GCC.
> > +   Copyright (C) 2025 Free Software Foundation, Inc.
> 
> 2026.

Yup, I went and hunted down all of these.

> > [...]
> > +#include "config.h"
> `#define INCLUDE_STRING` is needed based on review of the previous patch.

Got it.

> > [...]
> > +/* KCFI label counter, incremented by KCFI insn emission.  */
> > +static int kcfi_labelno = 0;
> 
> I am not 100% sure if this needs a GTY marker or not. I suspect no
> because we should not have emitted assembly code yet.

If this moves with kcfi_next_labelno into final, I think it's okay
without GTY?

> > +
> > +/* Get next KCFI label number.  Returns the current KCFI label number
> > +   and increments the internal counter for the next call.  The label is
> > +   used to provide a unique number for each indirect callsite of the
> > +   current module of the compilation.  */
> > +
> > +int
> > +kcfi_next_labelno (void)
> > +{
> > +  return kcfi_labelno++;
> > +}
> 
> Seems like this should be instead in `final.{cc,h}` rather than `kfci.{cc,h}`.

I think you want this because it's exclusively used during output phase?
I'll get it moved.

> > +/* Function attribute to store KCFI type ID.  */
> > +static tree kcfi_type_id_attr = NULL_TREE;
> 
> This 100% needs a GTY marker. to make sure the identifier does not get freed.

Got it.

> > +
> > +/* Get KCFI type ID for a function type.  Set it if missing.  FN_TYPE
> > +   is the function type tree node.  Returns the cached or newly computed
> > +   32-bit KCFI type identifier, storing it as a type attribute.  */
> > +
> > +static uint32_t
> > +kcfi_get_type_id (tree fn_type)
> > +{
> > +  uint32_t type_id;
> > +
> > +  /* Cache the attribute identifier for build_tree_list usage.  */
> > +  if (!kcfi_type_id_attr)
> > +    kcfi_type_id_attr = get_identifier ("kcfi_type_id");
> > +
> > +  tree attr = lookup_attribute ("kcfi_type_id", TYPE_ATTRIBUTES (fn_type));
> > +  if (attr)
> > +    {
> > +      tree value = TREE_VALUE (attr);
> > +      gcc_assert (value && TREE_CODE (value) == INTEGER_CST);
> gcc_assert (tree_fits_uhwi_p (value));
> type_id = tree_to_uhwi (value);

Updated.

> > +      type_id = (uint32_t) TREE_INT_CST_LOW (value);
> > +    }
> > +  else
> > +    {
> > +      type_id = compute_kcfi_type_id (fn_type);
> > +
> > +      tree type_id_tree = build_int_cst (unsigned_type_node, type_id);
> > +      tree attr = build_tree_list (kcfi_type_id_attr, type_id_tree);
> > +
> > +      TYPE_ATTRIBUTES (fn_type) = chainon (TYPE_ATTRIBUTES (fn_type), attr);
> > +    }
> 
> Instead of an attribute there must be a better way of doing this.
> Maybe a hashset instead.

Perhaps? I will go examine this vs LTO, etc.

> > [...]
> > +rtx
> > +internal_kcfi_get_type_id_for_expanding_gimple_call (void)
> > +{
> > +  gcc_assert (currently_expanding_gimple_stmt);
> 
> gcall *call_stmt = dyn_cast<gcall *> (currently_expanding_gimple_stmt);
> gcc_assert (call_stmt);

Updated.

> > [...]
> > +  /* Emit .type directive.  */
> > +  ASM_OUTPUT_TYPE_DIRECTIVE (asm_file, cfi_symbol_name.c_str (), "function");
> > +  ASM_OUTPUT_LABEL (asm_file, cfi_symbol_name.c_str ());
> > +
> > +  /* Emit any needed alignment padding NOPs using target's NOP template.  */
> > +  for (int i = 0; i < kcfi_alignment_padding_nops; i++)
> > +    output_asm_insn (kcfi_nop, NULL);
> I think you need to calculate kcfi_nop each time you call this
> function. Instead of caching it. For GC reasons.

Uh, do I have to? This seems wasteful: the nop is generated frequently.
Should kcfi_nop gain GTY instead? I will take a closer look at this.

> > [...]
> > +static unsigned int
> > +ipa_kcfi_execute (void)
> > +{
> > +  struct cgraph_node *node;
> > +
> > +  /* Prepare global KCFI alignment NOPs calculation once for all functions.  */
> > +  kcfi_prepare_alignment_nops ();
> > +
> > +  /* Process all functions - both local and external.  */
> > +  FOR_EACH_FUNCTION (node)
> > +    {
> > +      tree fndecl = node->decl;
> > +
> > +      /* Skip all non-NORMAL builtins (MD, FRONTEND) entirely.
> > +        For NORMAL builtins, skip those that lack an implicit
> > +        implementation (closest way to distinguishing DEF_LIB_BUILTIN
> > +        from others).  E.g. we need to have typeids for memset().  */
> > +      if (fndecl_built_in_p (fndecl))
> > +       {
> > +         if (DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
> > +           continue;
> > +         if (!builtin_decl_implicit_p (DECL_FUNCTION_CODE (fndecl)))
> > +           continue;
> > +       }
> > +
> > +      /* Cache the type_id in the function type.  */
> > +      kcfi_get_type_id (TREE_TYPE (fndecl));
> 
> Why are you needing to cache here?

This was originally added for the __kcfi_typeid emission for functions
known to the TU since at the time I couldn't figure out how to deal with
lacking FN_TYPE (to see the function argument types), but that logic
changed in v3, and I never noticed I didn't have to cache it any more.
It looks like I can drop this.

So... we could just not cache this at all, I guess? It seems wrong to
keep resolving the same typeid over and over though. I will look at
hashset, though, as you suggested.

-Kees

-- 
Kees Cook