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

[Kees Cook <kees@kernel.org>]

On Thu, Sep 18, 2025 at 04:59:56PM +0000, Qing Zhao wrote:
> 
> 
> > On Sep 17, 2025, at 17:09, Kees Cook <kees@kernel.org> wrote:
> > 
> > On Wed, Sep 17, 2025 at 01:42:32PM +0000, Qing Zhao wrote:
> >> This version of the middle-end change is much simpler and cleaner-:).
> > 
> > Thanks! I think it's getter closer (hopefully). :)
> > 
> >>> On Sep 13, 2025, at 19:23, Kees Cook <kees@kernel.org> wrote:
> [...]
> The above examples explain the whole picture very well.
> It might be a good idea to include them as comments of the routine “kcfi_prepare_alignment_nops”. 

I've expanded this much more now for the future v4.

> >>> +- External functions that are address-taken have a weak __kcfi_typeid_$func
> >>> +  symbol added with the typeid value available so that the typeid can be
> >>> +  referenced from assembly linkages, etc, where the typeid values cannot be
> >>> +  calculated (i.e where C type information is missing):
> >>> +
> >>> +    .weak   __kcfi_typeid_$func
> >>> +    .set    __kcfi_typeid_$func, $typeid
> >>> +
> >> 
> >> From my previous understanding, the above weak symbol is emitted for external functions
> >> that are address-taken AND does not have a definition in the compilation. So the weak symbols
> >> Is emitted at the declaration site of the external function, is this true?
> >> 
> >> If so, could you please clarify this in the above?
> > 
> > Yes, this happens via assemble_external_real, which can be called under
> > a few conditions in gcc/varasm.cc.
> 
> Okay. Please clarify this in the design doc. 

I mention it later in the "behavioral" section:

- assemble_external_real calls kcfi_emit_typeid_symbol to add the
  __kcfi_typeid_$func symbols.

I had left off implementation details (i.e. "called from
assemble_external_real") in the "constraints" section. How would you
like this arranged?

> >>> +- Keep indirect calls from being merged (see earlier example) by
> >>> +  checking the KCFI insn's typeid for equality.
> >> 
> >> Is this resolved by the following code:
> >> 
> >> rtlanal.cc
> >> index 63a1d08c46cf..5016fe93ccac 100644
> >> --- a/gcc/rtlanal.cc
> >> +++ b/gcc/rtlanal.cc
> >> @@ -1177,6 +1177,11 @@ reg_referenced_p (const_rtx x, const_rtx body)
> >>    case IF_THEN_ELSE:
> >>      return reg_overlap_mentioned_p (x, body);
> >> 
> >> +    case KCFI:
> >> +      /* For KCFI wrapper, check both the wrapped call and the type ID.  */
> >> +      return (reg_overlap_mentioned_p (x, XEXP (body, 0))
> >> +      || reg_overlap_mentioned_p (x, XEXP (body, 1)));
> >> +
> > 
> > The above is needed for accurate register "liveness" checking. When the
> > above code is removed, the kcfi-move-preservation.c regression test
> > fails (since it doesn't see the clobbers).
> Okay.  I see. 
> > 
> > AFAICT, simply making it a new type of RTL (the DEF_RTL_EXPR), made it
> > unmergeable.
> 
> Then is it possible some legal merging might not work anymore with this change? 

Perhaps? I will see if I can construct a case where there should be a
"merged" call (when the typeid matches).

> 
> > I assume this is because whatever was doing the call
> > merging was looking strictly for "CALL" types, but I honestly don't know
> > where that was happening.
> > 
> >>> +/* Common helper for RTL patterns to emit .kcfi_traps section entry.  */
> >> 
> >> I noticed that you didn’t explain each parameter of the function in all the comments for the functions.
> >> This need to be updated for all the new functions.
> > 
> > For externs like these, should the parameter documentation go in the .h
> > file, or the .cc file?
> 
> My understanding is the parameter doc going in the .cc file (just double checked some gcc files to make sure this) -:)

Okay, thanks! I will update these.

> > 
> >>> +void
> >>> +kcfi_emit_traps_section (FILE *file, rtx trap_label_sym)
> >>> +{
> >>> +  /* Generate entry label internally and get its number.  */
> >>> +  rtx entry_label = gen_label_rtx ();
> >>> +  int entry_labelno = CODE_LABEL_NUMBER (entry_label);
> >> 
> >> Is the only usage of the new RTX “entry_label” is to generate a label_number? 
> >> If so, the entry_label is not needed at all.  You can get a distinct labelno for each
> >> Lkcfi_entry, for example, the function id for the current function.
> > 
> > It is, yes. I can't use the function id because it's only incremented per
> > function and a given function may have multiple kcfi call sites within
> > it.
> 
> Okay.  I see. 
> 
> So, you need a unique lableno for each Lkcfi_entryN? Any other requirement?

Right, I need unique labels for each of trap, call, and entry. But they
are all associated together, so they could use a single counter.

> > I did have a version of this logic that used a kcfi-specific global
> > counter but (at the time) I was having trouble with it
> 
> What kind of issue? 
> 
> > and had seen that
> > other "custom label" examples in the code base used this style, so I
> > switched to that.
> 
> My concern is, the new generated RTX "entry_label” is not used at all, will there be any member leak from this?
> 
> 
> > 
> > I have since figured out why the global counter wasn't work (I was using
> > it during expansion and not during insn output, so I had cases where a
> > call was getting duplicated and I had a repeated label). If it's
> > preferred, I could try switching back to the global counter to avoid
> > these "useless" gen_label_rtx calls?
> 
> Yes, global counter approach is better. 

Okay, I will switch to that.

> 
> > 
> >>> +static uint32_t
> >>> +kcfi_get_type_id (tree fn_type)
> >>> +{
> >>> +  uint32_t type_id;
> >>> +
> >>> +  /* Cache the attribute identifier.  */
> >>> +  if (!kcfi_type_id_attr)
> >>> +    kcfi_type_id_attr = get_identifier ("kcfi_type_id");
> >>> +
> >>> +  tree attr = lookup_attribute (IDENTIFIER_POINTER (kcfi_type_id_attr),
> >>> + TYPE_ATTRIBUTES (fn_type));
> >> 
> >> The above can be simplified as:
> >> +  tree attr = lookup_attribute (“kcfi_type_id”, TYPE_ATTRIBUTES (fn_type));
> > 
> > Ugh, I totally misunderstood the examples I saw of this. I thought they
> > were caching the string lookup, but now that I look more closely, I see:
> > 
> > #define IDENTIFIER_POINTER(NODE) \
> >  ((const char *) IDENTIFIER_NODE_CHECK (NODE)->identifier.id.str)
> > 
> > it's just returning the string!
> > 
> > I will throw away the "caching" I was doing. I thought it would actually
> > look up the attribute using the tree returned by get_identifier, but I
> > see there is no overloaded lookup_attribute that takes a tree argument.
> > 
> > *face palm*
> 
> -:)

Okay, so I tried to remove this and remembered that it's actually cached
not for lookup_attribute, but for build_tree_list call case:

      tree attr = build_tree_list (kcfi_type_id_attr, type_id_tree);

      TYPE_ATTRIBUTES (fn_type) = chainon (TYPE_ATTRIBUTES (fn_type), attr);

For _that_, I need a "tree" argument. So instead of building it each
time, I have it built already, and I can get at its string for
lookup_attribute too. So I think this code is good as-is.

Thanks!

-Kees

-- 
Kees Cook