[PATCH] D119296: KCFI sanitizer

nickdesaulniers accepted this revision. nickdesaulniers added a comment. I see you modified the mir parser & printer; consider adding a .mir test. Still LGTM. Might be nice to document the generated asm more for other compiler vendors to better understand the implementation, rather than having t

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3623059 , @nickdesaulniers wrote: > I see you modified the mir parser & printer; consider adding a .mir test. I added a .mir test for parsing the cfi-type. The machine instructions generated for the various call

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. > 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. There are 30+ comments not marked as "done". Having so many is distracting to reviewers as it's unclear whether this is in a ready-for-review state. Comment at: clang/include/clang/Basic/Features.def:233 FEATURE(modules, LangOpts.Modules) +FEATURE(kcf

[PATCH] D119296: KCFI sanitizer

MaskRay requested changes to this revision. MaskRay added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2280 +void CodeGenModule::setKCFIType(const FunctionDecl *FD, llvm::Function *F) { + + if (isa(FD) && !cast(F

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1434 + + /// Emit KCFI type identifier constants and remove unused identifiers + void FinalizeKCFITypes(); End with a period. This function can be lower-case as well. There is no stro

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:125 + + // Emit int3 padding to allow runtime patching of the preamble. + EmitAndCountInstruction(MCInstBuilder(X86::INT3)); A previous comment isn't done. This doesn't explain tha

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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? Yes, I forgot to update t

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: clang/lib/Driver/SanitizerArgs.cpp:63 SanitizerKind::Unreachable | SanitizerKind::Return; -static const SanitizerMask AlwaysRecoverable = -SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress; +static const SanitizerMas

[PATCH] D119296: KCFI sanitizer

samitolvanen marked an inline comment as done. samitolvanen added inline comments. Comment at: clang/lib/Driver/SanitizerArgs.cpp:63 SanitizerKind::Unreachable | SanitizerKind::Return; -static const SanitizerMask AlwaysRecoverable = -SanitizerKind::KernelAddress | Saniti

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. Mostly looks good. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2296 + // functions, which means it's safe to skip unusual names. Subset of + // MCAsmInfo::isAcceptableChar() and MCAsmInfoXCOFF::isAcceptableChar(). + for (const char &C : Name) { -

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:332 + + Register AddrReg = MI.getOperand(0).getReg(); + MaskRay wrote: > Add const. Delete blank line after the declaration. Deleted the blank line, but this can't be `

[PATCH] D119296: KCFI sanitizer

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. Worth waiting a bit and checking whether @pcc has more comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llv

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added a comment. Based on the recent LKML discussions about X86 retbleed mitigations (https://lore.kernel.org/lkml/20220716230344.239749...@linutronix.de/), we're going to need some changes to the code generated for X86. Repository:

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. (Will read new updates.) Note that release/15.x is branching soon (https://discourse.llvm.org/t/llvm-15-0-0-release-schedule/63495): 2022-07-26. It seems that we can miss the branch to have more time mature the feature and have it for 16.0. Repository: rG LLVM Githu

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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. Sure, that sounds reasonable. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D119296: KCFI sanitizer

joaomoreira accepted this revision. joaomoreira added a comment. I really like this revision. It removes the redundancy of having KCFI passes both in CodeGen and in the backend; it detangles CALL instructions from KCFI by creating a new MIR instruction; it fixes alignment while still supporting

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3684022 , @joaomoreira wrote: > Some minor suggestions/not so relevant: > > - Change the name of CFIType to CFIHash, as it is probably more descriptive. My thinking here was that Type is more generic than a Hash,

[PATCH] D119296: KCFI sanitizer

pcc added a comment. > 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= to avoid errors. The offset > must be the same f

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 sp

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 422687. samitolvanen added a comment. Dropped `-fsanitize-kcfi-offset` and added an error when used with `-fpatchable-function-entry=N,M`, where M > 0. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/

[PATCH] D119296: KCFI sanitizer

nickdesaulniers added a comment. 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). Comment at: cl

[PATCH] D119296: KCFI sanitizer

nickdesaulniers added inline comments. Comment at: llvm/lib/MC/MCObjectFileInfo.cpp: + + const MCSectionELF &ElfSec = static_cast(TextSec); + unsigned Flags = ELF::SHF_LINK_ORDER; nickdesaulniers wrote: > or just `cast`. > https://llvm.org/docs/ProgrammersM

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 423748. samitolvanen marked 12 inline comments as done. samitolvanen added a comment. Addressed Nick's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 is

[PATCH] D119296: KCFI sanitizer

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:553-554 + if (LangOpts.Sanitize.has(SanitizerKind::KCFI)) { +EmitKCFIConstants(); +ClearUnusedKCFIPrefixes(); + } This is the only call site for these two methods, an

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 424030. samitolvanen marked 4 inline comments as done. samitolvanen added a comment. Addressed another round of comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2248 + F->setPrefixData(CreateKCFITypeId(FD->getType())); + F->addFnAttr("kcfi"); +} nickdesaulniers wrote: > While string based attributes are easier to work with in LLVM, I wo

[PATCH] D119296: KCFI sanitizer

nickdesaulniers added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2255-2260 + for (const char &C : Name) { +if (llvm::isAlnum(C) || C == '_' || C == '.') + continue; +return false; + } + return true; samitolvanen wrote: > nick

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2255-2260 + for (const char &C : Name) { +if (llvm::isAlnum(C) || C == '_' || C == '.') + continue; +return false; + } + return true; nickdesaulniers wrote: > sami

[PATCH] D119296: KCFI sanitizer

nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land. LGTM; it would be good if you could provide steps to test this on the Linux kernel. (i.e. what kernel patches are needed). Consider waiting for at least one additional review

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2268-2272 +if (!AddressTaken && F.hasLocalLinkage()) { + F.setPrefixData(nullptr); + F.removeFnAttr("kcfi"); + continue; +} nickdesaulniers wrote: > Can we

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. 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 c

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. 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 t

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 scenari

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 en

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 425642. samitolvanen marked an inline comment as done. samitolvanen added a comment. Herald added subscribers: ormris, wenlei, steven_wu, mgorny. - Added an LLVM pass to remove unneeded `llvm.kcfi.check` calls. - Switched from zeros to 0xcc for x86 type i

[PATCH] D119296: KCFI sanitizer

pcc added a comment. In D119296#3467905 , @samitolvanen wrote: > In D119296#3466027 , @joaomoreira > wrote: > >> I played a little bit with kcfi and here are some thoughts: >> >> - under -Os I saw functions bein

[PATCH] D119296: KCFI sanitizer

joaomoreira added a subscriber: craig.topper. joaomoreira added a comment. > This seems like a reasonable approach, and was also the approach taken for > the PAuth ABI. The PAuth ABI attaches an operand bundle to the call > instruction and arranges for the code for the check to be generated toge

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 425942. samitolvanen marked an inline comment as done. samitolvanen added a comment. - Moved the KCFI pass to InstCombine Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 op

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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. The original plan was to just lower the intri

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. > I agree that a separate pass wasn't ideal, but InstCombine seems to be full > of code to "fix what other passes messed up". :) I'm not sure if messed up > is the correct term though, these are checks that were necessary before > optimizations, but are no longer

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. > I looked at your code quickly and I wonder if using operand bundles would be > better than adding an attribute. Thoughts? Perhaps @pcc can provide a better feedback on the hassle (or joys) behind using operand bundles. My insights on the attribute is that it is ve

[PATCH] D119296: KCFI sanitizer

nickdesaulniers requested changes to this revision. nickdesaulniers added a comment. This revision now requires changes to proceed. (tentatively removing my +1 since this patch has changed quite a bit since my initial review) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D119296: KCFI sanitizer

nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land. In D119296#3481573 , @joaomoreira wrote: > I'm not an expert on LLVM's pipeline, but it just feels a little awkward and > redundant

[PATCH] D119296: KCFI sanitizer

joaomoreira added a comment. In D119296#3483176 , @nickdesaulniers wrote: > In D119296#3481573 , @joaomoreira > wrote: > >> I'm not an expert on LLVM's pipeline, but it just feels a little awkward and >> redund

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 427764. samitolvanen added a comment. 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 f

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 428208. samitolvanen added a comment. - Handle FP, LR, and XZR register arguments in the AArch64 `llvm.kcfi.check` lowering. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 418933. samitolvanen added a comment. Herald added subscribers: llvm-commits, pengfei, MaskRay, hiraditya. Herald added projects: LLVM, All. - Split the `kcfi_unchecked` attribute into a separate patch. - Based on feedback from kernel developers, switched

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 428520. samitolvanen added a comment. Added a test for the Clang -O2 pipeline dropping unneeded checks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files:

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 438010. samitolvanen added a comment. Rebased after ToT member function name changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files: clang/docs/Control

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 439099. samitolvanen added a comment. Switched from prefix data + attribute to a metadata node based on previous discussion. This seems to be a cleaner solution overall. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 439137. samitolvanen added a comment. Fixed the debug output in InstCombine to use metadata as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://reviews.llvm.org/D119296 Files: cla

[PATCH] D119296: KCFI sanitizer

pcc added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6694 - if (isExternallyVisible(T->getLinkage())) { + if (isExternallyVisible(T->getLinkage()) || !OnlyExternal) { std::string OutName; It would be better to have a separate functi

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6694 - if (isExternallyVisible(T->getLinkage())) { + if (isExternallyVisible(T->getLinkage()) || !OnlyExternal) { std::string OutName; -

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + samitolvanen wrote: > pcc wrote: > > samitolvanen wrote: > > > joaomoreira wrote: > > > > samitolvanen wrote: > > > > > joaomoreira

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 433795. samitolvanen added a comment. Herald added subscribers: jsji, laytonio, arichardson, emaste. Herald added a reviewer: MaskRay. - Changed Clang to emit operand bundles for indirect calls as pcc suggested, and dropped the `llvm.kcfi.check` intrinsi

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. This lld/ELF change looks like a hack. I'll need to study what this patch does, but in the meanwhile I changed myself to a blocking reviewer as I think the lld/ELF change needs more investigation. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) MaskRay wrote: > This change looks

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) samitolvanen wrote: > MaskRay wrote: >

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) MaskRay wrote: > samitolvanen wrot

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + I did not revise the entire patch yet. With this said, IMHO, this looks like an overcomplication of a simple problem. Is there a rea

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + joaomoreira wrote: > I did not revise the entire patch yet. With this said, IMHO, this looks like > an overcomplication of a simple

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + samitolvanen wrote: > joaomoreira wrote: > > I did not revise the entire patch yet. With this said, IMHO, this looks > > like an ove

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86ISelLowering.h:83 +KCFI_NT_CALL, +KCFI_TC_RETURN, + joaomoreira wrote: > samitolvanen wrote: > > joaomoreira wrote: > > > I did not revise the entire patch yet. With this said, IMHO, t

[PATCH] D119296: KCFI sanitizer

MaskRay requested changes to this revision. MaskRay added a comment. This revision now requires changes to proceed. Please don't land the patch this state. There are several questions which should be sorted out first. a) Naming About the 'k' prefix: this is generic and does not need to be coupl

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 > "ker

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 434974. samitolvanen marked 9 inline comments as done. samitolvanen added a comment. - 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

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: lld/ELF/Symbols.cpp:553 + // incompatible declarations for the same function. + if (isWeak() && getName().startswith("__kcfi_typeid_") && + cast(this)->value != other.value) samitolvanen wrote: > MaskRay wrot

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. 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 d

[PATCH] D119296: KCFI sanitizer

samitolvanen updated this revision to Diff 435685. samitolvanen added a comment. - 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. Whethe

[PATCH] D119296: KCFI sanitizer

ychen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2257 + + F->setPrefixData(CreateKCFITypeId(FD->getType())); + F->addFnAttr("kcfi-target"); FYI: using prefix data may not work for the C++ coroutine. (https://github.com/llvm/llvm-pro

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2257 + + F->setPrefixData(CreateKCFITypeId(FD->getType())); + F->addFnAttr("kcfi-target"); ychen wrote: > FYI: using prefix data may not work for the C++ coroutine. > (https://

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } Can we use another constant blinding scheme, such as a Value++ or anything else? This way, we would prevent endbrs from being

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } joaomoreira wrote: > Can we use another constant blinding scheme, such as a Value++ or anything > else? This way, we would p

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:121 +if (N == Value) + return ~Value; + } samitolvanen wrote: > joaomoreira wrote: > > Can we use another constant blinding scheme, such as a Value++ or anything > > e

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. > OK, so we could just not make the symbols weak and end up failing at link > time if there's a mismatch. That sounds reasonable to me. For `STB_WEAK SHN_ABS` `__kcfi_typeid_*`, there is no duplicate definition error. Is this behavior intentional? Note: I don't think we

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:330 + // comparing the hashes. + unsigned ScratchRegs[] = {AArch64::W16, AArch64::W17}; + const? Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cp

[PATCH] D119296: KCFI sanitizer

samitolvanen added a comment. In D119296#3733753 , @MaskRay wrote: > For `STB_WEAK SHN_ABS` `__kcfi_typeid_*`, there is no duplicate definition > error. Is this behavior intentional? Yes, this is by design. Multiple translation units must be able to tak

[PATCH] D119296: KCFI sanitizer

efriedma added inline comments. Comment at: clang/docs/ControlFlowIntegrity.rst:319 +cross-DSO function address equality. These properties make KCFI easier to +adopt in low-level software. KCFI is limited to indirect call checking only, +and isn't compatible with executable-only

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/docs/ControlFlowIntegrity.rst:319 +cross-DSO function address equality. These properties make KCFI easier to +adopt in low-level software. KCFI is limited to indirect call checking only, +and isn't compatible with executable-o

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:265 + PointerSumTypeMember, + PointerSumTypeMember>, + PointerSumTypeMember> This fails on 32-bit architectures as `PointerEmbeddedInt` doesn't allow storing

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: llvm/include/llvm/CodeGen/MachineInstr.h:265 + PointerSumTypeMember, + PointerSumTypeMember>, + PointerSumTypeMember> samitolvanen wrote: > This fails on 32-bit architectures as `PointerEmbeddedInt` d

[PATCH] D119296: KCFI sanitizer

samitolvanen created this revision. Herald added subscribers: dexonsmith, dang, jdoerfert, cryptoad. Herald added a reviewer: aaron.ballman. samitolvanen requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The KCFI sanitizer, enabled with -fsan

[PATCH] D119296: KCFI sanitizer

nickdesaulniers added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:5419 + typedef typeof(f1) * __attribute__((kcfi_unchecked) unchecked_t; + ((unchecked_t)p2)(); // unchecked + `p2` is already unchecked. Would it be more demonstrative to

[PATCH] D119296: KCFI sanitizer

pcc added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3168 + -1); + llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash); + llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont"); We considered a scheme like this befo

[PATCH] D119296: KCFI sanitizer

joaomoreira added inline comments. Comment at: clang/include/clang/Basic/Attr.td:696 +def KCFIUnchecked : Attr { + let Spellings = [Clang<"kcfi_unchecked">]; + let Subjects = SubjectList<[Var, TypedefName]>; Are you considering that perhaps one could use KCFI a

[PATCH] D119296: KCFI sanitizer

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:695 +def KCFIUnchecked : Attr { + let Spellings = [Clang<"kcfi_unchecked">]; Is there a reason the attribute isn't inheritable? e.g., ``` [[clang::kcfi_unchecked]] extern int i;

[PATCH] D119296: KCFI sanitizer

samitolvanen added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3168 + -1); + llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash); + llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont"); pcc wrote: > We considered a

[PATCH] D119296: KCFI sanitizer

samitolvanen marked an inline comment as not done. samitolvanen added inline comments. Comment at: clang/include/clang/Basic/Attr.td:696 +def KCFIUnchecked : Attr { + let Spellings = [Clang<"kcfi_unchecked">]; + let Subjects = SubjectList<[Var, TypedefName]>; j

[PATCH] D119296: KCFI sanitizer

samitolvanen planned changes to this revision. samitolvanen added a comment. Thanks for the pointers, Aaron. I'll rework the attribute code and also address the issues Nick pointed out in the next revision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D119296: KCFI sanitizer

pcc added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3168 + -1); + llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash); + llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont"); samitolvanen wrote: > pcc wrote: > >

[PATCH] D119296: KCFI sanitizer

pcc added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3168 + -1); + llvm::Value *Test = Builder.CreateICmpEQ(Builder.CreateLoad(HashPtr), Hash); + llvm::BasicBlock *ContBB = createBasicBlock("kcfi.cont"); pcc wrote: > samitolvanen wrote: > >