[LFI][AArch64] Add rewrites for memory accesses#195167
Conversation
|
@llvm/pr-subscribers-backend-aarch64 Author: Zachary Yedidia (zyedidia) ChangesThis PR adds LFI rewrites for loads and stores, and guards for stack pointer modifications. In this implementation, I've taken the approach of using functions with switch tables to track all the addressing mode information we need: base/offset indices and whether the access uses pre/post-indexing. I've tried to use macros to shrink the size of these switch statements and avoid duplication. In a previous implementation I used TSFlags bits set from TableGen to record this information, but the approach in this PR is more contained to just the LFI rewriter. This also introduces the With these changes the rewriter now supports making fully software-sandboxed programs. I've tested the changes on SPEC 2017 and the LLVM test suite. There are some minor changes needed in libunwind to avoid emitting SVE/MTE instructions that cause verification failure, which I'll address in a future patch. After this patch I'll add the guard elimination optimization, which will improve performance. After that the initial version of AArch64 LFI is complete. I also plan to make a patch to Clang to control the LFI subtarget features from a frontend option, and that can also set a macro that communicates the feature level (currently we just have Patch is 101.66 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/195167.diff 18 Files Affected:
diff --git a/llvm/docs/LFI.rst b/llvm/docs/LFI.rst
index a173cd57e0ee0..fb28195fa23d4 100644
--- a/llvm/docs/LFI.rst
+++ b/llvm/docs/LFI.rst
@@ -115,8 +115,6 @@ Example:
Compiler Options
++++++++++++++++
-**Note**: these options are not yet implemented.
-
The LFI target has several configuration options, specified via ``-mattr=``:
* ``+no-lfi-loads``: Disable sandboxing for load instructions (stores-only mode).
@@ -197,8 +195,6 @@ require any rewrite.
Memory accesses
~~~~~~~~~~~~~~~
-**Note**: not yet implemented.
-
Memory accesses are rewritten to use the ``[x27, wM, uxtw]`` addressing mode if
it is available, which is automatically safe. Otherwise, rewrites fall back to
using ``x28`` along with an instruction to safely load it with the target
@@ -279,8 +275,6 @@ address.
Stack pointer modification
~~~~~~~~~~~~~~~~~~~~~~~~~~
-**Note**: not yet implemented.
-
When the stack pointer is modified, we write the modified value to a temporary,
before moving it back into ``sp`` with a safe ``add``.
diff --git a/llvm/lib/Target/AArch64/AArch64Features.td b/llvm/lib/Target/AArch64/AArch64Features.td
index 8cd2278a05747..ab77106eaa1bc 100644
--- a/llvm/lib/Target/AArch64/AArch64Features.td
+++ b/llvm/lib/Target/AArch64/AArch64Features.td
@@ -1075,6 +1075,11 @@ def FeatureHardenSlsNoComdat : SubtargetFeature<"harden-sls-nocomdat",
"Generate thunk code for SLS mitigation in the normal text section">;
+def FeatureNoLFILoads : SubtargetFeature<"no-lfi-loads", "NoLFILoads", "true",
+ "Disable LFI sandboxing for load instructions">;
+def FeatureNoLFIStores : SubtargetFeature<"no-lfi-stores", "NoLFIStores", "true",
+ "Disable LFI sandboxing for store instructions">;
+
// Only intended to be used by disassemblers.
def FeatureAll
: SubtargetFeature<"all", "IsAll", "true", "Enable all instructions">;
diff --git a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
index 844adc3eb54c4..decb3572c6638 100644
--- a/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
@@ -16,6 +16,7 @@
#include "AArch64PointerAuth.h"
#include "AArch64Subtarget.h"
#include "MCTargetDesc/AArch64AddressingModes.h"
+#include "MCTargetDesc/AArch64MCLFIRewriter.h"
#include "MCTargetDesc/AArch64MCTargetDesc.h"
#include "Utils/AArch64BaseInfo.h"
#include "llvm/ADT/ArrayRef.h"
@@ -129,14 +130,39 @@ static std::optional<unsigned> getLFIInstSizeInBytes(const MachineInstr &MI) {
if (MI.getOperand(0).getReg() != AArch64::LR)
return 8;
return 4;
+ case AArch64::SYSxt:
+ // VA-based DC/IC ops (op1=3, Cn=7, op2=1) expand to 2 instructions.
+ if (MI.getOperand(0).getImm() == 3 && MI.getOperand(1).getImm() == 7 &&
+ MI.getOperand(3).getImm() == 1)
+ return 8;
+ return std::nullopt;
default:
break;
}
- // Instructions that explicitly modify LR expand to 2 instructions.
+ // Instructions that explicitly modify LR get an extra LR mask.
+ bool ModifiesLR = false;
for (const MachineOperand &MO : MI.explicit_operands())
- if (MO.isReg() && MO.isDef() && MO.getReg() == AArch64::LR)
- return 8;
+ if (MO.isReg() && MO.isDef() && MO.getReg() == AArch64::LR) {
+ ModifiesLR = true;
+ break;
+ }
+
+ // Memory accesses expand to a base-register guard plus the rewritten access
+ // (8 bytes), with an extra base-register update for pre/post-index forms (12
+ // bytes total). If the access also defines LR, an LR mask is appended (+4
+ // bytes). Depending on additional optimizations that the rewriter performs,
+ // this may be an overestimate.
+ if (MI.mayLoadOrStore()) {
+ unsigned Size = isLFIPrePostMemAccess(MI.getOpcode()) ? 12 : 8;
+ if (ModifiesLR)
+ Size += 4;
+ return Size;
+ }
+
+ // Non-memory instructions that modify LR expand to 2 instructions.
+ if (ModifiesLR)
+ return 8;
// Default case: instructions that don't cause expansion.
// - TP accesses in LFI are a single load/store, so no expansion.
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCLFIRewriter.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCLFIRewriter.cpp
index 6066ddb7e59a9..6a81a59341c0a 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCLFIRewriter.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCLFIRewriter.cpp
@@ -17,6 +17,8 @@
#include "Utils/AArch64BaseInfo.h"
#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstrDesc.h"
+#include "llvm/MC/MCInstrInfo.h"
#include "llvm/MC/MCStreamer.h"
#include "llvm/MC/MCSubtargetInfo.h"
@@ -64,6 +66,97 @@ static bool isPrivilegedTPAccess(const MCInst &Inst) {
return false;
}
+// Instructions that have mayLoad/mayStore set in TableGen but don't actually
+// perform memory accesses.
+static bool isNotMemAccess(const MCInst &Inst) {
+ switch (Inst.getOpcode()) {
+ case AArch64::DMB:
+ case AArch64::DSB:
+ case AArch64::ISB:
+ case AArch64::HINT:
+ return true;
+ default:
+ return false;
+ }
+}
+
+static bool mayPrefetch(const MCInst &Inst) {
+ switch (Inst.getOpcode()) {
+ case AArch64::PRFMl:
+ case AArch64::PRFMroW:
+ case AArch64::PRFMroX:
+ case AArch64::PRFMui:
+ case AArch64::PRFUMi:
+ return true;
+ default:
+ return false;
+ }
+}
+
+// User-mode DC/IC instructions that take a virtual address operand. Encoded as
+// SYSxt with op1=3, Cn=7, op2=1 where the Cm field selects the operation.
+static bool isVASysOp(const MCInst &Inst) {
+ if (Inst.getOpcode() != AArch64::SYSxt)
+ return false;
+ if (Inst.getOperand(0).getImm() != 3 || Inst.getOperand(1).getImm() != 7 ||
+ Inst.getOperand(3).getImm() != 1)
+ return false;
+ switch (Inst.getOperand(2).getImm()) {
+ case 4: // DC ZVA
+ case 5: // IC IVAU
+ case 10: // DC CVAC
+ case 11: // DC CVAU
+ case 12: // DC CVAP
+ case 13: // DC CVADP
+ case 14: // DC CIVAC
+ return true;
+ default:
+ return false;
+ }
+}
+
+static MCInst replaceRegAt(const MCInst &Inst, unsigned Idx,
+ MCRegister NewReg) {
+ MCInst New;
+ New.setOpcode(Inst.getOpcode());
+ New.setLoc(Inst.getLoc());
+ for (unsigned I = 0, E = Inst.getNumOperands(); I != E; ++I) {
+ if (I == Idx)
+ New.addOperand(MCOperand::createReg(NewReg));
+ else
+ New.addOperand(Inst.getOperand(I));
+ }
+ return New;
+}
+
+// Memory instruction information for the basic load/store rewriting path.
+// Provides operand indices for the base register and offset, and whether
+// the instruction uses pre/post-indexed addressing.
+struct MemInstInfo {
+ int BaseRegIdx;
+ int OffsetIdx; // -1 if no offset operand.
+ bool IsPrePost;
+ bool IsLiteral;
+};
+
+// Returns memory instruction info for a given opcode, or std::nullopt if
+// the opcode is not a recognized memory instruction.
+static std::optional<MemInstInfo> getMemInstInfo(unsigned Op);
+
+static unsigned convertUiToRoW(unsigned Op);
+static unsigned convertPreToRoW(unsigned Op);
+static unsigned convertPostToRoW(unsigned Op);
+static unsigned convertRoXToRoW(unsigned Op, unsigned &Shift);
+static bool getRoWShift(unsigned Op, unsigned &Shift);
+static unsigned getPrePostScale(unsigned Op);
+static unsigned convertPrePostToBase(unsigned Op, bool &IsPre,
+ bool &IsNoOffset);
+static int getSIMDNaturalOffset(unsigned Op);
+
+bool AArch64MCLFIRewriter::mayModifyStack(const MCInst &Inst) const {
+ return mayModifyRegister(Inst, AArch64::SP);
+}
+
bool AArch64MCLFIRewriter::mayModifyReserved(const MCInst &Inst) const {
return mayModifyRegister(Inst, LFIAddrReg) ||
mayModifyRegister(Inst, LFIBaseReg) ||
@@ -111,6 +204,75 @@ void AArch64MCLFIRewriter::emitMov(MCRegister Dest, MCRegister Src,
emitInst(Inst, Out, STI);
}
+void AArch64MCLFIRewriter::emitAddImm(MCRegister Dest, MCRegister Src,
+ int64_t Imm, MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ assert(std::abs(Imm) <= 4095);
+ MCInst Inst;
+ if (Imm >= 0) {
+ Inst.setOpcode(AArch64::ADDXri);
+ Inst.addOperand(MCOperand::createReg(Dest));
+ Inst.addOperand(MCOperand::createReg(Src));
+ Inst.addOperand(MCOperand::createImm(Imm));
+ Inst.addOperand(MCOperand::createImm(0)); // shift
+ } else {
+ Inst.setOpcode(AArch64::SUBXri);
+ Inst.addOperand(MCOperand::createReg(Dest));
+ Inst.addOperand(MCOperand::createReg(Src));
+ Inst.addOperand(MCOperand::createImm(-Imm));
+ Inst.addOperand(MCOperand::createImm(0)); // shift
+ }
+ emitInst(Inst, Out, STI);
+}
+
+void AArch64MCLFIRewriter::emitAddReg(MCRegister Dest, MCRegister Src1,
+ MCRegister Src2, unsigned Shift,
+ MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ // add Dest, Src1, Src2, lsl #Shift
+ MCInst Inst;
+ Inst.setOpcode(AArch64::ADDXrs);
+ Inst.addOperand(MCOperand::createReg(Dest));
+ Inst.addOperand(MCOperand::createReg(Src1));
+ Inst.addOperand(MCOperand::createReg(Src2));
+ Inst.addOperand(
+ MCOperand::createImm(AArch64_AM::getShifterImm(AArch64_AM::LSL, Shift)));
+ emitInst(Inst, Out, STI);
+}
+
+void AArch64MCLFIRewriter::emitAddRegExtend(MCRegister Dest, MCRegister Src1,
+ MCRegister Src2,
+ AArch64_AM::ShiftExtendType ExtType,
+ unsigned Shift, MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ // add Dest, Src1, Src2, ExtType #Shift
+ MCInst Inst;
+ if (ExtType == AArch64_AM::SXTX || ExtType == AArch64_AM::UXTX)
+ Inst.setOpcode(AArch64::ADDXrx64);
+ else
+ Inst.setOpcode(AArch64::ADDXrx);
+ Inst.addOperand(MCOperand::createReg(Dest));
+ Inst.addOperand(MCOperand::createReg(Src1));
+ Inst.addOperand(MCOperand::createReg(Src2));
+ Inst.addOperand(
+ MCOperand::createImm(AArch64_AM::getArithExtendImm(ExtType, Shift)));
+ emitInst(Inst, Out, STI);
+}
+
+void AArch64MCLFIRewriter::emitMemRoW(unsigned Opcode, const MCOperand &DataOp,
+ MCRegister BaseReg, MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ // Op DataOp, [LFIBaseReg, W(BaseReg), uxtw]
+ MCInst Inst;
+ Inst.setOpcode(Opcode);
+ Inst.addOperand(DataOp);
+ Inst.addOperand(MCOperand::createReg(LFIBaseReg));
+ Inst.addOperand(MCOperand::createReg(getWRegFromXReg(BaseReg)));
+ Inst.addOperand(MCOperand::createImm(0)); // S bit = 0 (UXTW).
+ Inst.addOperand(MCOperand::createImm(0)); // Shift amount = 0 (unscaled).
+ emitInst(Inst, Out, STI);
+}
+
// {br,blr} xN
// ->
// add x28, x27, wN, uxtw
@@ -151,7 +313,11 @@ void AArch64MCLFIRewriter::rewriteReturn(const MCInst &Inst, MCStreamer &Out,
void AArch64MCLFIRewriter::rewriteLRModification(const MCInst &Inst,
MCStreamer &Out,
const MCSubtargetInfo &STI) {
- emitInst(Inst, Out, STI);
+ if (!isNotMemAccess(Inst) &&
+ (mayLoad(Inst) || mayStore(Inst) || mayPrefetch(Inst)))
+ rewriteLoadStore(Inst, Out, STI);
+ else
+ emitInst(Inst, Out, STI);
emitAddMask(AArch64::LR, AArch64::LR, Out, STI);
}
@@ -211,6 +377,254 @@ void AArch64MCLFIRewriter::rewriteTPWrite(const MCInst &Inst, MCStreamer &Out,
emitInst(Store, Out, STI);
}
+bool AArch64MCLFIRewriter::rewriteLoadStoreRoW(const MCInst &Inst,
+ MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ unsigned Op = Inst.getOpcode();
+ unsigned MemOp;
+
+ // Case 1: Indexed load/store with zero immediate offset.
+ // ldr xN, [xM, #0] -> ldr xN, [x27, wM, uxtw]
+ if ((MemOp = convertUiToRoW(Op)) != AArch64::INSTRUCTION_LIST_END) {
+ MCRegister BaseReg = Inst.getOperand(1).getReg();
+ if (BaseReg == AArch64::SP)
+ return false;
+ const MCOperand &OffsetOp = Inst.getOperand(2);
+ if (OffsetOp.isImm() && OffsetOp.getImm() == 0) {
+ emitMemRoW(MemOp, Inst.getOperand(0), BaseReg, Out, STI);
+ return true;
+ }
+ return false;
+ }
+
+ // Case 2: Pre-index load/store.
+ // ldr xN, [xM, #imm]! -> add xM, xM, #imm; ldr xN, [x27, wM, uxtw]
+ if ((MemOp = convertPreToRoW(Op)) != AArch64::INSTRUCTION_LIST_END) {
+ MCRegister BaseReg = Inst.getOperand(2).getReg();
+ if (BaseReg == AArch64::SP)
+ return false;
+ int64_t Imm = Inst.getOperand(3).getImm();
+ emitAddImm(BaseReg, BaseReg, Imm, Out, STI);
+ emitMemRoW(MemOp, Inst.getOperand(1), BaseReg, Out, STI);
+ return true;
+ }
+
+ // Case 3: Post-index load/store.
+ // ldr xN, [xM], #imm -> ldr xN, [x27, wM, uxtw]; add xM, xM, #imm
+ if ((MemOp = convertPostToRoW(Op)) != AArch64::INSTRUCTION_LIST_END) {
+ MCRegister BaseReg = Inst.getOperand(2).getReg();
+ if (BaseReg == AArch64::SP)
+ return false;
+ int64_t Imm = Inst.getOperand(3).getImm();
+ emitMemRoW(MemOp, Inst.getOperand(1), BaseReg, Out, STI);
+ emitAddImm(BaseReg, BaseReg, Imm, Out, STI);
+ return true;
+ }
+
+ // Case 4: Register-offset-X load/store.
+ // ldr xN, [xM1, xM2] -> add x26, xM1, xM2; ldr xN, [x27, w26, uxtw]
+ unsigned Shift;
+ if ((MemOp = convertRoXToRoW(Op, Shift)) != AArch64::INSTRUCTION_LIST_END) {
+ MCRegister Reg1 = Inst.getOperand(1).getReg();
+ MCRegister Reg2 = Inst.getOperand(2).getReg();
+ int64_t Extend = Inst.getOperand(3).getImm();
+ int64_t IsShift = Inst.getOperand(4).getImm();
+
+ if (!IsShift)
+ Shift = 0;
+
+ if (Extend)
+ emitAddRegExtend(LFIScratchReg, Reg1, Reg2, AArch64_AM::SXTX, Shift, Out,
+ STI);
+ else
+ emitAddReg(LFIScratchReg, Reg1, Reg2, Shift, Out, STI);
+ emitMemRoW(MemOp, Inst.getOperand(0), LFIScratchReg, Out, STI);
+ return true;
+ }
+
+ // Case 5: Register-offset-W load/store.
+ // ldr xN, [xM1, wM2, uxtw] -> add x26, xM1, wM2, uxtw;
+ // ldr xN, [x27, w26, uxtw]
+ if (getRoWShift(Op, Shift)) {
+ MCRegister Reg1 = Inst.getOperand(1).getReg();
+ MCRegister Reg2 = Inst.getOperand(2).getReg();
+ int64_t S = Inst.getOperand(3).getImm();
+ int64_t IsShift = Inst.getOperand(4).getImm();
+
+ if (!IsShift)
+ Shift = 0;
+
+ if (S)
+ emitAddRegExtend(LFIScratchReg, Reg1, Reg2, AArch64_AM::SXTW, Shift, Out,
+ STI);
+ else
+ emitAddRegExtend(LFIScratchReg, Reg1, Reg2, AArch64_AM::UXTW, Shift, Out,
+ STI);
+ emitMemRoW(Op, Inst.getOperand(0), LFIScratchReg, Out, STI);
+ return true;
+ }
+
+ return false;
+}
+
+void AArch64MCLFIRewriter::rewriteLoadStoreBasic(const MCInst &Inst,
+ MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ unsigned Opcode = Inst.getOpcode();
+ auto Info = getMemInstInfo(Opcode);
+
+ if (!Info)
+ return error(Inst, "unknown addressing mode for memory instruction in LFI");
+
+ if (Info->IsLiteral)
+ return error(Inst, "PC-relative literal loads are not supported in LFI");
+
+ MCRegister BaseReg = Inst.getOperand(Info->BaseRegIdx).getReg();
+
+ // Stack accesses don't need address sandboxing, except when sp is modified
+ // with a register post-index operand.
+ bool BaseIsSP = BaseReg == AArch64::SP;
+ if (BaseIsSP) {
+ if (Info->OffsetIdx < 0 || !Inst.getOperand(Info->OffsetIdx).isReg())
+ return emitInst(Inst, Out, STI);
+ MCRegister OffReg = Inst.getOperand(Info->OffsetIdx).getReg();
+ if (OffReg == AArch64::XZR || OffReg == AArch64::WZR)
+ return emitInst(Inst, Out, STI);
+ }
+
+ // Guard the base register, unless it is SP.
+ MCRegister AccessBase = BaseIsSP ? AArch64::SP : LFIAddrReg;
+ if (!BaseIsSP)
+ emitAddMask(LFIAddrReg, BaseReg, Out, STI);
+
+ if (Info->IsPrePost) {
+ bool IsPre = false;
+ bool IsNoOffset = false;
+ unsigned BaseOpcode = convertPrePostToBase(Opcode, IsPre, IsNoOffset);
+
+ if (BaseOpcode != AArch64::INSTRUCTION_LIST_END) {
+ // Demote pre/post-index to base indexed form.
+ MCInst NewInst;
+ NewInst.setOpcode(BaseOpcode);
+ NewInst.setLoc(Inst.getLoc());
+
+ // Skip writeback operand (operand 0) and copy data operands up to base.
+ for (int I = 1; I < Info->BaseRegIdx; ++I)
+ NewInst.addOperand(Inst.getOperand(I));
+
+ // Add the access base register (LFIAddrReg or SP).
+ NewInst.addOperand(MCOperand::createReg(AccessBase));
+
+ // For pre-index, include the offset; for post-index, use zero.
+ if (IsPre && Info->OffsetIdx >= 0)
+ NewInst.addOperand(Inst.getOperand(Info->OffsetIdx));
+ else if (!IsNoOffset)
+ NewInst.addOperand(MCOperand::createImm(0));
+
+ emitInst(NewInst, Out, STI);
+
+ // Update the base register with the offset. If the base is SP, a
+ // register offset must be sandboxed (the result is otherwise
+ // unbounded), and ADDXrs cannot take SP, so the extended-register form
+ // via the scratch register is used.
+ if (Info->OffsetIdx >= 0) {
+ const MCOperand &OffsetOp = Inst.getOperand(Info->OffsetIdx);
+ if (OffsetOp.isImm()) {
+ int64_t Scale = getPrePostScale(Opcode);
+ int64_t Offset = OffsetOp.getImm() * Scale;
+ emitAddImm(BaseReg, BaseReg, Offset, Out, STI);
+ } else if (OffsetOp.isReg()) {
+ // SIMD post-index uses a register offset (XZR for natural offset).
+ MCRegister OffReg = OffsetOp.getReg();
+ if (OffReg == AArch64::XZR) {
+ int NaturalOffset = getSIMDNaturalOffset(Opcode);
+ if (NaturalOffset > 0)
+ emitAddImm(BaseReg, BaseReg, NaturalOffset, Out, STI);
+ } else if (OffReg != AArch64::WZR) {
+ if (BaseIsSP) {
+ emitAddRegExtend(LFIScratchReg, AArch64::SP, OffReg,
+ AArch64_AM::UXTX, 0, Out, STI);
+ emitAddMask(AArch64::SP, LFIScratchReg, Out, STI);
+ } else {
+ emitAddReg(BaseReg, BaseReg, OffReg, 0, Out, STI);
+ }
+ }
+ }
+ }
+ } else {
+ error(Inst, "unhandled pre/post-index instruction in LFI rewriter");
+ }
+ } else {
+ // Non-pre/post instruction: replace the base register operand.
+ MCInst NewInst = replaceRegAt(Inst, Info->BaseRegIdx, LFIAddrReg);
+ emitInst(NewInst, Out, STI);
+ }
+}
+
+void AArch64MCLFIRewriter::rewriteLoadStore(const MCInst &Inst, MCStreamer &Out,
+ const MCSubtargetInfo &STI) {
+ bool IsStore = mayStore(Inst);
+ bool IsLoad = mayLoad(Inst) || mayPrefetch(Inst);
+
+ bool SkipLoads = STI.hasFeature(AArch64::FeatureNoLFILoads);
+ bool SkipStores = STI.hasFeature(AArch64::FeatureNoLFIStores);
+
+ if ((!IsLoad || SkipLoads) && (!IsStore || SkipStores))
+ return emitInst(Inst, Out, STI);
+
+ if (rewriteLoadStoreRoW(Inst, Out, STI))
+ return;
+
+ rewriteLoadStoreBasic(Inst, Out, STI);
+}
+
+// modify sp
+// ->
+// modify x26
+// add sp, x27, w26, uxtw
+void AArch64MCLFIRewriter::rewriteStackModification(
+ const MCInst &Inst, MCStreamer &Out, const MCSubtargetInfo &STI) {
+ // Route through rewriteLRModification or rewriteLoadStore for memory
+ // accesses. Those helpers automatically handle dangerous stack modifications
+ // that can happen via register post-index.
+ if (mayLoad(Inst) || mayStore(Inst)) {
+ if (mayModifyRegister(Inst, AArch64::LR))
+ return rewriteLRModification(Inst, Out, STI);
+ return rewriteLoadStore(Inst, Out, STI);
+ }
+
+ // No stack sandboxing if sandboxing is disabled for both loads and stores.
+ bool SkipLoads = STI.hasFeature(AArch64::FeatureNoLFILoads);
+ bool SkipStores = STI.hasFeature(AArch64::FeatureNoLFIStores);
+ if (SkipLoads && SkipStores)
+ return emitInst(Inst, Out, STI);
+
+ // Redirect SP modification destination to scratch, then sandbox.
+ MCInst ModInst = replaceRegAt(Inst, 0, LFIScratchReg);
+ emitInst(ModInst, Out, STI);
+ emitAddMask(AArch64::SP, LFIScratchReg, Out, STI);
+}
+
+// {dc,ic} <op>, xN
+// ->
+// add x28, x27, wN, uxtw
+// {dc,ic} <op>, x28
+void AArch64MCLFIRewriter::rewriteVASysOp(const MCInst &Inst, MCStreamer &Out,
+ ...
[truncated]
|
zyedidia
requested review from
Sharjeel-Khan,
efriedma-quic,
nickdesaulniers and
smithp35
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
I've made a start, have left a marker where I stopped. Will try and make some progress through it over the next few days.
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
GitHub is giving me an internal error when I try to leave more comments. It doesn't seem to affect my colleagues so there may be some odd state in its system.
A couple more comments that I can express here (hopefully these won't end up as duplicates):
Line 400
I suggest adding writeback
// Case 2: Pre-index load/store, with writeback.
Line 432
Is it possible that XM1 could be SP? All the other code paths return false in this situation.
|
Thanks for the review so far!
In this case since a general-purpose register is being added to SP (rather than a static immediate), we need a rewrite (see line 171 in |
Thanks for the explanation. For the benefit of a new reader of the code, could be worth a comment. Will take a look through the remainder of the file next week. |
zyedidia
force-pushed
the
llvm-pr/lfi/aarch64-rewriter-3
branch
from
22badd1 to
117d9a0
Compare
| X(STRD, 3) \ | ||
| X(STRX, 3) \ | ||
| X(LDRQ, 4) \ | ||
| X(STRQ, 4) |
There was a problem hiding this comment.
I really despise the x macros pattern; it quickly can become difficult to follow. Is there nothing smarter that can be done in tablegen to maintain these tables inside the instruction definitions themselves?
There was a problem hiding this comment.
In the original more monolithic PR for the AArch64 rewriter (#184277) I used a TSFlags-based approach to encode some of the necessary info in TableGen. See this for the relevant TableGen updates: https://github.com/llvm/llvm-project/pull/184277/changes#diff-b7c2a6d4b6cc2fac3fd43b4877e6ccd05c395aeb38c605d2e1ff862753f9ce3f. Even with that info, some large switch tables were necessary (see the bottom of https://github.com/llvm/llvm-project/pull/184277/changes#diff-026171a3feab4865a9a843b323382856be51e14125b69301a370bdde28e068dc). In general, I am seeking guidance on whether to go with the TSFlags/TableGen approach, or the switch table approach. When refactoring for this PR, I did manage to use macros to significantly reduce the switch table source code size, so I went with that since it is more self-contained vs. touching TSFlags/TableGen, but I am still unsure of which approach is better.
There was a problem hiding this comment.
I had two contradictory reactions to seeing the original tablegen modifications and seeing the macros. On seeing the Tablegen my initial thought was that LFI, for better or worse, would be put in front of a lot more developers. For example let's assume that there is some new load in a future architecture extension, the developer may need to interact and understand the LFI flags when making modifications. Whereas with the representation here, LFI can take its time to catch up.
On seeing the macros I had the same reaction as Nick, and thought can we put this in Tablegen!
I don't have a strong opinion one way or the other as there are trade-offs. If there were a new TableGen backend that could auto-generate the switch tables then this would likely swing it favour of TableGen. I guess that could be a refactoring afterwards.
I'll see if I can get any reaction from my colleagues internally, you may also want to ask the specific question in Discourse.
There was a problem hiding this comment.
I realized after this discussion that I could use TableGen in a self-contained way, independent of the existing TableGen instruction definitions, to avoid these macros. I've updated the implementation to do that. Let me know what you think of this approach. The latest commit shows the diff: 64ae66f.
There was a problem hiding this comment.
For example let's assume that there is some new load in a future architecture extension
Yeah, I guess this is the largest risk (IMO) that isn't mitigated by using tablegen or x-macros.
I was wondering if perhaps instead of hard coding values for every single aarch64 instruction we would need to rewrite, can we make the code significantly shorter and less brittle to new arch extensions by making the code more generic? Is it possible to use MCInstrDesc::{mayLoad,mayStore} to figure out if a given instruction is a load or store, then calculate the necessary values the x-macros-switch/tablegen would, such that we're not having to maintain tables at all?
There was a problem hiding this comment.
I can take a look, although thanks to a couple of short weeks (company and country holidays) I'm a bit behind, please do give the occasional ping. I'll try and summarise the two approaches and will post them internally to see if anyone has any strong opinions.
There was a problem hiding this comment.
Worst case, we do have a verifier IIUC? Perhaps we could delegate erroring on newly added-to-the-ISA loads/stores there, and proceed to land this?
There was a problem hiding this comment.
Yes that sounds good to me, the verifier is the final source of truth on whether an instruction has been rewritten properly, so even if they pass through the rewriter unmodified (a warning will be produced by the current code), the verifier will notify the user if the generated addressing mode is unsafe.
There was a problem hiding this comment.
I've had a chance to look through Nick's proposed TableGen alternatives.
I agree with Nick that lfi-project#6 is the better of the two alternatives. I don't think the additional changes in AArch64InstrFormats.td are significantly more complex than lfi-project#5
I found it difficult to compare 3 implementations across the size of the diffs. I think it would likely be simpler for a non LFI expert to have a [NFC] refactoring and put that to a wider audience to see if there are any objections. The feeling I get is that an overall simpler more robust solution is preferred to keeping LFI isolated.
One small comment on the TableGen, there were some classes with parameters that didn't look like they were used. There could be some inheritance that I'm missing though.
class LFILoadStoreRO<bits<2> sz, bit V> : LFIInstruction
let LFIInst = !cast<Instruction>(NAME);
let IsLFIMem = 1;
let BaseIdx = 1;
let OffsetIdx = 2;
let HasOffset = 1;
}
| // multiple switch tables without repetition. Each macro takes a callback X and | ||
| // invokes X(NAME, VALUE) for each entry. | ||
|
|
||
| // Scalar memory ops that have ui, pre, post, roX, and roW variants. |
There was a problem hiding this comment.
Can you expand on what "ui, pre, post, roX, and roW variants" are (in comments)?
There was a problem hiding this comment.
There is a comment that explains the meaning of Ui, RoW, and RoX earlier in the file (line 147) when these functions are forward-declared. I can also add explanation of pre/post there.
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
I've got to the end of the file. I must confess to not going over all of the cases in the switch-statements.
As mentioned in the inline comment, I don't have a particularly strong opinion vs the question of whether to use tablegen, although I suspect others might. Possibly worth widening out to Discourse.
There's also the possibility of refactoring it afterwards, which may be easier to review.
zyedidia
force-pushed
the
llvm-pr/lfi/aarch64-rewriter-3
branch
from
b665ebc to
64ae66f
Compare
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
Thanks for adding the tablegen. I think that helps a lot, definitely easier to follow than the macros.
I don't have any more significant comments at the moment. This is looking good on my side.
nickdesaulniers
left a comment
nickdesaulniers
left a comment
There was a problem hiding this comment.
Looks like this needs to be rebased before landing. Please leave this open for another week to collect any other feedback.
|
I owe some feedback on the TableGen proposals mentioned in #195167 (comment) I don't think that should prevent this from landing, I think it could be handled as a NFC refactoring change afterwards. Hope to be able to comment on the tablegen this week. |
zyedidia
force-pushed
the
llvm-pr/lfi/aarch64-rewriter-3
branch
from
eb6e583 to
f2b56f1
Compare
smithp35
left a comment
smithp35
left a comment
There was a problem hiding this comment.
I've marked this LGTM on my side.
|
SGTM, I will merge this tomorrow morning. |
This patch adds LFI rewrites for loads and stores, and guards for stack pointer modifications. This also introduces the `+no-lfi-stores` and `+no-lfi-loads` features to control the granularity of memory sandboxing. With these changes the rewriter now supports making fully software-sandboxed programs. There are some minor changes needed in libunwind to avoid emitting SVE/MTE instructions that cause verification failure, which will be addressed in a future patch. Future work includes the guard elimination optimization, which will improve performance, Clang flags to control the LFI subtarget features from the frontend, and that can also set a macro that communicates the LFI feature level.
This patch adds LFI rewrites for loads and stores, and guards for stack pointer modifications. This also introduces the `+no-lfi-stores` and `+no-lfi-loads` features to control the granularity of memory sandboxing. With these changes the rewriter now supports making fully software-sandboxed programs. There are some minor changes needed in libunwind to avoid emitting SVE/MTE instructions that cause verification failure, which will be addressed in a future patch. Future work includes the guard elimination optimization, which will improve performance, Clang flags to control the LFI subtarget features from the frontend, and that can also set a macro that communicates the LFI feature level.
4 participants
This PR adds LFI rewrites for loads and stores, and guards for stack pointer modifications. In this implementation, I've taken the approach of using functions with switch tables to track all the addressing mode information we need: base/offset indices and whether the access uses pre/post-indexing. I've tried to use macros to shrink the size of these switch statements and avoid duplication. In a previous implementation I used TSFlags bits set from TableGen to record this information, but the approach in this PR is more contained to just the LFI rewriter.
This also introduces the
+no-lfi-storesand+no-lfi-loadsfeatures to control the granularity of memory sandboxing.With these changes the rewriter now supports making fully software-sandboxed programs. I've tested the changes on SPEC 2017 and the LLVM test suite. There are some minor changes needed in libunwind to avoid emitting SVE/MTE instructions that cause verification failure, which I'll address in a future patch.
After this patch I'll add the guard elimination optimization, which will improve performance. After that the initial version of AArch64 LFI is complete. I also plan to make a patch to Clang to control the LFI subtarget features from a frontend option, and that can also set a macro that communicates the feature level (currently we just have
__LFI__).