In D119296#3684022, @joaomoreira wrote:

Some minor suggestions/not so relevant:

• Change the name of CFIType to CFIHash, as it is probably more descriptive.

• Ensured that we don't emit ENDBR64/32 as the type hash on X86.

In D119296#3673297, @MaskRay wrote:

It seems that we can miss the branch to have more time mature the feature and have it for 16.0.

Addressed conflicts with X86 retbleed mitigations (https://lore.kernel.org/lkml/20220716230344.239749011@linutronix.de/):

Based on the recent LKML discussions about X86 retbleed mitigations (https://lore.kernel.org/lkml/20220716230344.239749011@linutronix.de/), we're going to need some changes to the code generated for X86.

• Simplified AArch64 by dropping the KCFI_CHECK_BTI pseudo and just using a different caller-saved temporary register if needed.
• Added -verify-machineinstrs to llc tests.

Addressed Fangrui's feedback.

• Added a missing equivalency check to MachineInstr.

Addressed the rest of Fangrui's comments after clarifying the open questions with him offline.

In D119296#3625796, @MaskRay wrote:

It uses LLVM prefix data to store a type identifier for each function and injects verification code before indirect calls.

Is "prefix data" stale now?

Addressed Fangrui's feedback.

In D119296#3623059, @nickdesaulniers wrote:

I see you modified the mir parser & printer; consider adding a .mir test.

Added a MIR parser test for cfi-type, address Nick's other feedback.

• Moved to a standalone function for generating KCFI type hashes in Clang.
• Switched from pseudo instructions to a MachineInstr::ExtraInfo + SDNode attributes.
• Added KCFI passes for emitting the indirect call checks.

Fixed the debug output in InstCombine to use metadata as well.

Switched from prefix data + attribute to a metadata node based on previous discussion. This seems to be a cleaner solution overall.

Rebased after ToT member function name changes.

• Per Fangrui's request, added comments explaining the X86 preamble format, and a note about it to the patch summary.
• Switched back to .weak for the __kcfi_typeid_ symbols to fix compatibility with LTO. Whether we need a warning for symbol value mismatches and how this should be implemented can be addressed later. The warning isn't critical for the functionality and mismatches should be extremely rare.

In D119296#3562409, @MaskRay wrote:

ELF linkers don't error for two SHN_ABS STB_GLOBAL symbols of the same st_value.
When the st_value fields differ, there will be a diagnostic. If needed, the specialized diagnostic can be added there.

• Addressed Fangrui's feedback.
• Renamed KCFI_* DAG nodes and pseudo instructions to CFI_* for now based on Joao's feedback. Still looking for more feedback on the best way to implement this part.

In D119296#3562409, @MaskRay wrote:

a) Naming

About the 'k' prefix: this is generic and does not need to be coupled with "kernel",
but perhaps an argument can be made that the 'k' does not need to refer to "kernel".

• Changed Clang to emit operand bundles for indirect calls as pcc suggested, and dropped the llvm.kcfi.check intrinsic.
• Based on further LKML discussion, implemented arch-specific lowering that ensures the KCFI check can be placed immediately before the call instruction on X86.
• Switched to relative offsets in .kcfi_traps and fixed the __cfi_ preamble linkage on X86.

Added a test for the Clang -O2 pipeline dropping unneeded checks.

• Handle FP, LR, and XZR register arguments in the AArch64 llvm.kcfi.check lowering.

Based on LKML feedback:

• Fixed a bug in Twine usage.
• Changed AArch64 to encode register information into the ESR and dropped .kcfi_traps generation for the arch.
• Changed X86 to generate valid instructions for the type data.
• Dropped the .kcfi_types section entirely.

OK, I confirmed that we won't need this after all. I'll abandon this patch and revisit if it becomes necessary in future.

LKML RFC: https://lore.kernel.org/lkml/20220429203644.2868448-1-samitolvanen@google.com/

• Renamed the builtin.
• Addressed Nick's comments.

In D119296#3481573, @joaomoreira wrote:

At first I was on the boat of KCFI being implemented in the IR level because being architecture agnostic is certainly a great plus.

In D119296#3480793, @pcc wrote:

Yes, I suspect this might be an issue with Clang's existing CFI schemes too, and would probably require an additional pass to drop checks before calls that were either inlined or optimized into direct calls.

IIRC, this wasn't really an issue for -fsanitize=cfi because 99% of the time indirect calls that were going to resolve to direct calls had already done so by the time we got to the LowerTypeTests pass (which was only run during LTO).

• Moved the KCFI pass to InstCombine

In D124211#3480773, @nickdesaulniers wrote:

Perhaps it's time to start a discussion on LKML then, before landing this?

In D124211#3480652, @nickdesaulniers wrote:

Can you link to the lore thread on discussions around the builtin?

• Added an LLVM pass to remove unneeded llvm.kcfi.check calls.
• Switched from zeros to 0xcc for x86 type identifier padding.

In D119296#3466217, @joaomoreira wrote:

Oh, one other tiny detail I forgot to mention. I noticed that the tag is pushing the functions 6 bytes forward, regardless of any prepending padding nops that were added to ensure 16b alignment. It would be cool to care about the proper function alignment and also to not emit dummy padding nops when the padding area can be filled with the tag itself.

In D119296#3466027, @joaomoreira wrote:

I played a little bit with kcfi and here are some thoughts:

• under -Os I saw functions being inlined, regardless of the source code calling them indirectly. In these scenarios, the KCFI check was still in place, even though there was not a pointer involved in the call. Although not semantically incorrect, it would be great to prevent the unnecessary overhead (see attached source/compile it with -Os and -fsanitize=kcfi).

Addressed another round of comments.

In D119296#3457740, @nickdesaulniers wrote:

Please add a test where calls to @llvm.kcfi.check produce KCFI_CHECK during instruction selection. (You can use -stop-before=finalize-isel to dump the IR prior to isel; this can stay a .ll test, I think, rather than .mir).

Addressed Nick's comments.

Dropped -fsanitize-kcfi-offset and added an error when used with -fpatchable-function-entry=N,M, where M > 0.

Alright, thanks for the feedback everyone! I'll abandon this patch and look into adding a built-in function instead.

In D122673#3443498, @aaron.ballman wrote:

I tend to be very wary of modifying the type system with attributes -- questions always arise of what the type system effects are of the attribute. e.g., does it impact overload sets or template specialization? Name mangling? If it doesn't have type system impacts... why does it need to be in the type system at all? This also matters because adding more bits to types can have unintended side effects like accidentally reducing the depth of template instantiations we can process (because of the extra memory pressure). So while I'm not certain what you and @pcc talked about, it does seem like an approach at least worth thinking about, especially because this patch needs to bump the size of Type.

In D119296#3437604, @pcc wrote:

Note that if additional data has been injected between the KCFI
type identifier and the start of the function, e.g. by using
-fpatchable-function-entry, the offset in bytes must be specified
using -fsanitize-kcfi-offset=<value> to avoid errors. The offset
must be the same for all indirectly called functions in every
translation unit.

On x86 the specific constant 6 is necessary to ensure that the constant embedded in the cmpl operand can't be used as a gadget. So any value other than 6 will potentially impact the security of KCFI.

I would prefer not to design an interaction between -fpatchable-function-entry and KCFI until the specific use case is known.

Note that this was split from D119296.

• Split the kcfi_unchecked attribute into a separate patch.
• Based on feedback from kernel developers, switched to 32-bit type identifiers, added the llvm.kcfi.check intrinsic with arch-specific lowering, and added sections for locating both type identifiers and traps.
• Addressed the remaining review comments.

Thanks for the pointers, Aaron. I'll rework the attribute code and also address the issues Nick pointed out in the next revision.

Addressed Nick's comments about NoCFIValue::handleOperandChangeImpl.

See https://reviews.llvm.org/D113613.

Applied pcc's changes.

Based on more offline feedback from pcc, moved the conditional assignment handling from MCStreamer to MCObjectStreamer. This allows us to drop the emittedSymbols set as MCAssembler already keeps track of the symbols. As MCAsmStreamer now passes through the .lto_set_conditional directives, changed the tests to look at the generated objects instead.

Changed the key of the pendingAssignments map to const MCSymbol * and switched from StringMap/Set to DenseMap/Set based on feedback from pcc.

Addressed comments.

Use standard l-value conversions, and add a test case for constexpr.

In D108479#3150298, @rjmccall wrote:

Your builtin is using custom type-checking (t), which suppresses all the normal conversions that happen on expressions. Specifically, it skips lvalue-to-rvalue conversion, so in this example the argument ends up being an l-value reference to a variable rather than an r-value loaded from that variable.

Changed the code to evaluate the argument as a constant expression.

In D108479#3149297, @rjmccall wrote:

In D108479#3149228, @samitolvanen wrote:

I worked around this for now by explicitly allowing __builtin_function_start in CheckLValueConstantExpression, but this seems terribly hacky. What would be the correct way to solve this issue?

Try to generalize what we do for __builtin___CFStringMakeConstantString.

In D108479#3140688, @rjmccall wrote:

In D108479#3140638, @samitolvanen wrote:

Sure, I can take a look at how that would work. Basically, in PointerExprEvaluator::VisitBuiltinCallExpr we should not evaluate the l-value and just leave it at Result.set(E)?

Yes, exactly. Since the builtin already requires a constant operand in non-dependent contexts, that should be enough.

Refactored to avoid evaluating the expression into an l-value.

Renamed to .lto_set_conditional.

Updated tests to use # and reordered the CHECKs for clarity.

In D108479#3140688, @rjmccall wrote:

In D108479#3140638, @samitolvanen wrote:

In D108479#3133578, @rjmccall wrote:

If we do need to support constant expressions of this

Yes, we need this also in constant expressions.

Okay. I assume just static initializers, and not things like template arguments?

Rebased.

In D108479#3133578, @rjmccall wrote:

If we do need to support constant expressions of this