[PATCH] D155485: Retain all jump table range checks when using BTI.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added a comment.
This revision is now accepted and ready to land.

I agree with the hardening side argument. I have checked `FallthroughUnreachable` uses in `CC_BitTests` and `CC_Range` for some CodeGen tests (mostly in X86/) and confirmed that they don't need the `"branch-target-enforcement"` special case.

> A table-based branch of any kind is at risk of being a JOP gadget, if it doesn't range-check the offset into the table. ...

Consider adding `CC_JumpTable` to the paragraph. Its case label is many lines above and with the default context of git log, it's difficult to see how this change is relevant to `CC_JumpTable` ...

> ... many of these table branch idioms use branch instructions that do not set the BTI flag, so they can target instructions without BTI landing pads.

Q: what does "use branch instructions that do not set the BTI flag" mean?



================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:11325
+        if (FallthroughUnreachable &&
+            !CurMBB->getBasicBlock()->getModule()->getModuleFlag(
+                "branch-target-enforcement"))
----------------
`CurMF->getMMI().getModule()->getModuleFlag(` is more common to get `Module`.


================
Comment at: llvm/test/CodeGen/Thumb2/jump-table-bti.ll:1
+; RUN: sed s/SPACE/4/ %s | llc -mtriple=thumbv8.1m.main-linux-gnu -mattr=+pacbti -o - | FileCheck %s --check-prefix=BTI-TBB
+
----------------
There is a bti-jump-table.mir, but I think `jump-table-*` as this patch adds is a better name. It groups jump table tests together and matches `ARM/jump-table-*.ll`.


================
Comment at: llvm/test/CodeGen/Thumb2/jump-table-bti.ll:1
+; RUN: sed s/SPACE/4/ %s | llc -mtriple=thumbv8.1m.main-linux-gnu -mattr=+pacbti -o - | FileCheck %s --check-prefix=BTI-TBB
+
----------------
MaskRay wrote:
> There is a bti-jump-table.mir, but I think `jump-table-*` as this patch adds is a better name. It groups jump table tests together and matches `ARM/jump-table-*.ll`.
This test file needs a file-level comment describing what it tests, perhaps:

`;; When BTI is enabled, keep the range check for a jump table for hardening, even with a unreachable default`


================
Comment at: llvm/test/CodeGen/Thumb2/jump-table-bti.ll:3
+
+; RUN: sed s/SPACE/4/ %s | sed /^..for-non-bti-build-sed-will-delete-everything-after-this-line/q | llc -mtriple=thumbv8.1m.main-linux-gnu -mattr=+pacbti -o - | FileCheck %s --check-prefix=NOBTI-TBB
+
----------------
Add quotes, otherwise this seems to trigger some substitutions in zsh: `sed '/^..for-non-bti-build-sed-will-delete-everything-after-this-line/q'`




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155485/new/

https://reviews.llvm.org/D155485