Page MenuHomePhabricator

Add __builtin_kcfi_call_unchecked
AbandonedPublic

Authored by samitolvanen on Apr 21 2022, 3:49 PM.

Details

Summary

With -fsanitize=kcfi, disabling indirect call checking using
the no_sanitize attribute is not fine-grained enough in cases
where we want to disable checking only for specific calls. The
__builtin_kcfi_call_unchecked builtin accepts an indirect call
expression and emits the call without KCFI type checking.

Depends on D119296

Diff Detail

Event Timeline

samitolvanen created this revision.Apr 21 2022, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 3:49 PM
samitolvanen requested review of this revision.Apr 21 2022, 3:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2022, 3:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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

clang/lib/Sema/SemaChecking.cpp
436
451

auto *Builtin =

though I would perhaps not use auto here.

clang/lib/Sema/SemaChecking.cpp
436

Also, can this simply be a dyn_cast rather than dyn_cast_or_null? Don't you already check the arg count above?

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

There's no LKML discussion about the built-in, but there were earlier proposals of using an attribute to accomplish the same thing. @pcc suggested using a built-in instead, and the reviewers in D122673 agreed that it's a more appropriate solution. In the kernel, the indirect static_call trampoline calls that are patched into direct calls at boot are the primary reason we need a finer-grained method of disabling CFI checks.

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

There's no LKML discussion about the built-in, but there were earlier proposals of using an attribute to accomplish the same thing. @pcc suggested using a built-in instead, and the reviewers in D122673 agreed that it's a more appropriate solution. In the kernel, the indirect static_call trampoline calls that are patched into direct calls at boot are the primary reason we need a finer-grained method of disabling CFI checks.

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

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

I don't intend to land these patches until there's a consensus on the correct approach from both projects, and I was hoping to get a confirmation that this API looks reasonable from the LLVM side before sending an RFC series to LKML. Since nobody has objected, I suppose there's no reason to delay that.

My only comment about the design is that maybe it should be __builtin_kcfi_call_unchecked, which does not seem like something you need to hold up LKML discussion for.

samitolvanen marked 2 inline comments as done.
  • Renamed the builtin.
  • Addressed Nick's comments.
samitolvanen retitled this revision from Add __builtin_call_kcfi_unchecked to Add __builtin_kcfi_call_unchecked.Apr 29 2022, 10:55 AM
samitolvanen edited the summary of this revision. (Show Details)
nickdesaulniers accepted this revision.Apr 29 2022, 11:06 AM
This revision is now accepted and ready to land.Apr 29 2022, 11:06 AM
samitolvanen abandoned this revision.May 3 2022, 10:34 AM

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