Skip to content

Commit 4a8e242

Browse files
kbeylskbeyls
committed
[GS/StackClash] Recognize stack realignment code sequences.
1 parent 59abd9c commit 4a8e242

File tree

4 files changed

+153
-90
lines changed

4 files changed

+153
-90
lines changed

‎bolt/include/bolt/Core/MCPlusBuilder.h‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -646,7 +646,13 @@ class MCPlusBuilder {
646646
}
647647

648648
virtual bool isRetainOnlyLowerBitsInReg(const MCInst &Inst, MCPhysReg &Reg,
649-
uint64_t &Mask) {
649+
uint64_t &Mask) const {
650+
llvm_unreachable("not implemented");
651+
return false;
652+
}
653+
654+
virtual bool isMaskLowerBitsInReg(const MCInst &Inst, MCPhysReg &FromReg,
655+
MCPhysReg& ToReg, uint64_t &Mask) const {
650656
llvm_unreachable("not implemented");
651657
return false;
652658
}

‎bolt/lib/Passes/StackClashAnalysis.cpp‎

Lines changed: 108 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ raw_ostream &print_state(raw_ostream &OS, const State &S,
206206
} else
207207
OS << "None";
208208
OS << ",";
209-
OS << "LastStackGrowingInsts(" << S.LastStackGrowingInsts.size() << ")>";
209+
OS << "LastStackGrowingInsts(" << S.LastStackGrowingInsts.size() << ")> ";
210210
return OS;
211211
}
212212

@@ -224,6 +224,86 @@ class StackClashStatePrinter {
224224
explicit StackClashStatePrinter(const BinaryContext &BC) : BC(BC) {}
225225
};
226226

227+
bool checkNonConstSPOffsetChange(const BinaryContext &BC, BinaryFunction &BF,
228+
const MCInst &Point, const State &Cur,
229+
State *Next = nullptr) {
230+
const MCPhysReg SP = BC.MIB->getStackPointer();
231+
bool IsNonConstantSPOffsetChange = false;
232+
if (BC.MIB->hasDefOfPhysReg(Point, SP)) {
233+
IsNonConstantSPOffsetChange = true;
234+
235+
// Next, validate that we can track by how much the SP
236+
// value changes. This should be a constant amount.
237+
// Else, if we cannot determine the fixed offset, mark this location as
238+
// needing a report that this potentially changes the SP value by a
239+
// non-constant amount, and hence violates stack-clash properties.
240+
if (Next)
241+
Next->LastStackGrowingInsts.insert(MCInstInBBReference::get(&Point, BF));
242+
if (auto OC = BC.MIB->getOffsetChange(Point, Cur.RegConstValues,
243+
Cur.RegMaxValues);
244+
OC && OC.ToReg == SP) {
245+
if (OC.FromReg == SP) {
246+
IsNonConstantSPOffsetChange = false;
247+
assert(OC.MaxOffsetChange);
248+
if (Next) {
249+
if (*OC.MaxOffsetChange < 0)
250+
Next->MaxOffsetSinceLastProbe =
251+
*Next->MaxOffsetSinceLastProbe - *OC.MaxOffsetChange;
252+
if (OC.OffsetChange && Next->SPFixedOffsetFromOrig)
253+
Next->SPFixedOffsetFromOrig =
254+
*Next->SPFixedOffsetFromOrig + *OC.OffsetChange;
255+
// FIXME: add test case for this if test.
256+
#if 0
257+
if (IsPreIndexOffsetChange)
258+
Next.MaxOffsetSinceLastProbe =
259+
*Next.MaxOffsetSinceLastProbe - StackAccessOffset;
260+
#endif
261+
LLVM_DEBUG({
262+
dbgs() << " Found SP Offset change: ";
263+
BC.printInstruction(dbgs(), Point);
264+
dbgs() << " OffsetChange: " << OC.OffsetChange
265+
<< "; MaxOffsetChange: " << OC.MaxOffsetChange
266+
<< "; new MaxOffsetSinceLastProbe: "
267+
<< Next->MaxOffsetSinceLastProbe
268+
<< "; new SPFixedOffsetFromOrig: "
269+
<< Next->SPFixedOffsetFromOrig
270+
<< "\n";
271+
});
272+
}
273+
// assert(!OC.IsPreIndexOffsetChange || IsStackAccess);
274+
if (Next)
275+
assert(*Next->MaxOffsetSinceLastProbe >= 0);
276+
} else if (Cur.Reg2MaxOffset && Cur.Reg2MaxOffset->contains(OC.FromReg) &&
277+
OC.OffsetChange) {
278+
IsNonConstantSPOffsetChange = false;
279+
const int64_t MaxOffset = Cur.Reg2MaxOffset->find(OC.FromReg)->second;
280+
if (Next) {
281+
Next->MaxOffsetSinceLastProbe = MaxOffset - *OC.OffsetChange;
282+
Next->SPFixedOffsetFromOrig = std::nullopt;
283+
}
284+
}
285+
}
286+
}
287+
uint64_t Mask = 0;
288+
if (MCPhysReg FromReg, ToReg;
289+
BC.MIB->isMaskLowerBitsInReg(Point, FromReg, ToReg, Mask) &&
290+
Cur.Reg2MaxOffset && Cur.Reg2MaxOffset->contains(FromReg)) {
291+
// handle SP-aligning patterns like
292+
// sub x9, sp, #0x1d0
293+
// and sp, x9, #0xffffffffffffff80
294+
uint64_t BitsToZeroMask = ~Mask;
295+
int64_t MaxOffsetChange = BitsToZeroMask + 1;
296+
if (Next) {
297+
Next->MaxOffsetSinceLastProbe =
298+
Cur.Reg2MaxOffset->find(FromReg)->second + MaxOffsetChange;
299+
Next->SPFixedOffsetFromOrig = std::nullopt;
300+
}
301+
IsNonConstantSPOffsetChange = false;
302+
}
303+
304+
return IsNonConstantSPOffsetChange;
305+
}
306+
227307
class StackClashDFAnalysis
228308
: public DataflowAnalysis<StackClashDFAnalysis, State, false /*Forward*/,
229309
StackClashStatePrinter> {
@@ -356,13 +436,15 @@ class StackClashDFAnalysis
356436
MCPhysReg FixedOffsetRegJustSet = BC.MIB->getNoRegister();
357437
if (auto OC = BC.MIB->getOffsetChange(Point, Cur.RegConstValues,
358438
Cur.RegMaxValues))
359-
if (Next.Reg2MaxOffset) {
439+
if (Next.Reg2MaxOffset && OC.OffsetChange) {
440+
int64_t Offset = *OC.OffsetChange;
360441
if (OC.FromReg == SP) {
361-
(*Next.Reg2MaxOffset)[OC.ToReg] = *Cur.MaxOffsetSinceLastProbe;
442+
(*Next.Reg2MaxOffset)[OC.ToReg] =
443+
*Cur.MaxOffsetSinceLastProbe - Offset;
362444
FixedOffsetRegJustSet = OC.ToReg;
363445
} else if (auto I = Cur.Reg2MaxOffset->find(OC.FromReg);
364446
I != Cur.Reg2MaxOffset->end()) {
365-
(*Next.Reg2MaxOffset)[OC.ToReg] = (*I).second;
447+
(*Next.Reg2MaxOffset)[OC.ToReg] = (*I).second - Offset;
366448
FixedOffsetRegJustSet = OC.ToReg;
367449
}
368450
}
@@ -380,60 +462,18 @@ class StackClashDFAnalysis
380462
}
381463
}
382464

383-
if (BC.MIB->hasDefOfPhysReg(Point, SP)) {
384-
// Next, validate that we can track by how much the SP
385-
// value changes. This should be a constant amount.
386-
// Else, if we cannot determine the fixed offset, mark this location as
387-
// needing a report that this potentially changes the SP value by a
388-
// non-constant amount, and hence violates stack-clash properties.
389-
Next.LastStackGrowingInsts.insert(MCInstInBBReference::get(&Point, BF));
390-
if (auto OC = BC.MIB->getOffsetChange(Point, Cur.RegConstValues,
391-
Cur.RegMaxValues);
392-
OC && OC.ToReg == SP) {
393-
if (OC.FromReg == SP) {
394-
assert(OC.MaxOffsetChange);
395-
if (*OC.MaxOffsetChange < 0)
396-
Next.MaxOffsetSinceLastProbe =
397-
*Next.MaxOffsetSinceLastProbe - *OC.MaxOffsetChange;
398-
if (OC.OffsetChange && Next.SPFixedOffsetFromOrig)
399-
Next.SPFixedOffsetFromOrig =
400-
*Next.SPFixedOffsetFromOrig + *OC.OffsetChange;
401-
// FIXME: add test case for this if test.
402-
#if 0
403-
if (IsPreIndexOffsetChange)
404-
Next.MaxOffsetSinceLastProbe =
405-
*Next.MaxOffsetSinceLastProbe - StackAccessOffset;
406-
#endif
407-
LLVM_DEBUG({
408-
dbgs() << " Found SP Offset change: ";
409-
BC.printInstruction(dbgs(), Point);
410-
dbgs() << " OffsetChange: " << OC.OffsetChange
411-
<< "; MaxOffsetChange: " << OC.MaxOffsetChange
412-
<< "; new MaxOffsetSinceLastProbe: "
413-
<< Next.MaxOffsetSinceLastProbe
414-
<< "; new SPFixedOffsetFromOrig: "
415-
<< Next.SPFixedOffsetFromOrig
416-
<< "; IsStackAccess:" << IsStackAccess
417-
<< "; StackAccessOffset: " << StackAccessOffset << "\n";
418-
});
419-
assert(!OC.IsPreIndexOffsetChange || IsStackAccess);
420-
assert(*Next.MaxOffsetSinceLastProbe >= 0);
421-
} else if (
422-
Cur.Reg2MaxOffset && Cur.Reg2MaxOffset->contains(OC.FromReg) &&
423-
OC.OffsetChange ==
424-
0 /* FIXME: also handle other offset changes if needed. */) {
425-
const int64_t MaxOffset = Cur.Reg2MaxOffset->find(OC.FromReg)->second;
426-
Next.MaxOffsetSinceLastProbe = MaxOffset;
427-
}
428-
} else {
429-
Next.MaxOffsetSinceLastProbe.reset();
430-
Next.SPFixedOffsetFromOrig
431-
.reset(); // FIXME - should I make this the empty set?
432-
LLVM_DEBUG({
433-
dbgs() << " Found non-const SP Offset change: ";
434-
BC.printInstruction(dbgs(), Point);
435-
});
436-
}
465+
bool IsNonConstantSPOffsetChange =
466+
checkNonConstSPOffsetChange(BC, BF, Point, Cur, &Next);
467+
if (IsNonConstantSPOffsetChange) {
468+
Next.MaxOffsetSinceLastProbe.reset();
469+
Next.SPFixedOffsetFromOrig
470+
.reset(); // FIXME - should I make this the empty set?
471+
// FIXME - should I make the Reg trackers empty sets
472+
// here?
473+
LLVM_DEBUG({
474+
dbgs() << " Found non-const SP Offset change: ";
475+
BC.printInstruction(dbgs(), Point);
476+
});
437477
}
438478

439479
return Next;
@@ -461,7 +501,6 @@ void StackClashAnalysis::runOnFunction(
461501
StackClashDFAnalysis SCDFA(BF, AllocatorId);
462502
SCDFA.run();
463503
BinaryContext &BC = BF.getBinaryContext();
464-
const MCPhysReg SP = BC.MIB->getStackPointer();
465504

466505
// Now iterate over the basic blocks to indicate where something needs
467506
// to be reported.
@@ -498,31 +537,16 @@ void StackClashAnalysis::runOnFunction(
498537
// Else, if we cannot determine the fixed offset, mark this location
499538
// as needing a report that this potentially changes the SP value by a
500539
// non-constant amount, and hence violates stack-clash properties.
501-
if (auto OC =
502-
BC.MIB->getOffsetChange(Inst, S.RegConstValues, S.RegMaxValues);
503-
BC.MIB->hasDefOfPhysReg(Inst, SP) &&
504-
!(OC && OC.ToReg == SP && OC.FromReg == SP)) {
505-
// If this is a register move from a register that we know contains
506-
// a value that is a fixed offset from the SP at the start of the
507-
// function into the SP, then we know this is probably fine.
508-
// Typical case is moving the frame pointer to the stack pointer
509-
// in the function epilogue.
510-
bool IsFixedRegToSPMove = OC && OC.ToReg == SP && S.Reg2MaxOffset &&
511-
S.Reg2MaxOffset->contains(OC.FromReg);
512-
if (IsFixedRegToSPMove) {
513-
S.MaxOffsetSinceLastProbe =
514-
S.Reg2MaxOffset->find(OC.FromReg)->second;
515-
} else {
516-
// mark to report that this may be an SP change that is not a
517-
// constant amount.
518-
LLVM_DEBUG({
519-
dbgs() << " Found SP Offset change that may not be a constant "
520-
"amount: ";
521-
BC.printInstruction(dbgs(), Inst);
522-
});
523-
SCIAnnotations.push_back(
524-
StackClashIssue::createNonConstantSPChangeData());
525-
}
540+
if (checkNonConstSPOffsetChange(BC, BF, Inst, S)) {
541+
// mark to report that this may be an SP change that is not a
542+
// constant amount.
543+
LLVM_DEBUG({
544+
dbgs() << " Found SP Offset change that may not be a constant "
545+
"amount: ";
546+
BC.printInstruction(dbgs(), Inst);
547+
});
548+
SCIAnnotations.push_back(
549+
StackClashIssue::createNonConstantSPChangeData());
526550
}
527551
// merge and add annotations
528552
if (SCIAnnotations.size() == 0)

‎bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -410,8 +410,8 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
410410
return Inst.getOpcode() == AArch64::ADR;
411411
}
412412

413-
bool isRetainOnlyLowerBitsInReg(const MCInst &Inst, MCPhysReg &Reg,
414-
uint64_t &Mask) override {
413+
bool isRetainOnlyLowerBitsInReg(const MCInst &Inst, MCPhysReg &ToReg,
414+
uint64_t &Mask) const override {
415415
bool Is32Bit = false;
416416
switch (Inst.getOpcode()) {
417417
default:
@@ -426,7 +426,30 @@ class AArch64MCPlusBuilder : public MCPlusBuilder {
426426
Inst.getOperand(2).getImm(), Is32Bit ? 32 : 64);
427427
if (!isPowerOf2_64(MaskValue + 1) && MaskValue != 0)
428428
return false;
429-
Reg = Inst.getOperand(0).getReg();
429+
ToReg = Inst.getOperand(0).getReg();
430+
Mask = MaskValue;
431+
return true;
432+
}
433+
}
434+
435+
bool isMaskLowerBitsInReg(const MCInst &Inst, MCPhysReg &FromReg,
436+
MCPhysReg &ToReg, uint64_t &Mask) const override {
437+
bool Is32Bit = false;
438+
switch (Inst.getOpcode()) {
439+
default:
440+
return false;
441+
case AArch64::ANDSWri:
442+
case AArch64::ANDWri:
443+
Is32Bit = true;
444+
LLVM_FALLTHROUGH;
445+
case AArch64::ANDSXri:
446+
case AArch64::ANDXri:
447+
uint64_t MaskValue = AArch64_AM::decodeLogicalImmediate(
448+
Inst.getOperand(2).getImm(), Is32Bit ? 32 : 64);
449+
if (!isPowerOf2_64(~MaskValue + 1) && MaskValue != 0)
450+
return false;
451+
ToReg = Inst.getOperand(0).getReg();
452+
FromReg = Inst.getOperand(1).getReg();
430453
Mask = MaskValue;
431454
return true;
432455
}

‎bolt/test/gadget-scanner/gs-stackclash.s‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,10 @@ f_variable_large_stack:
4040
stp x29, x30, [sp, #-16]!
4141
add x1, x0, #0xf
4242
and x1, x1, #0xfffffffffffffff0
43-
mov x29, sp
4443
sub sp, sp, x1
4544
mov x1, sp
4645
add x0, x1, x0, lsl #3
4746
ldur w0, [x0, #-24]
48-
mov sp, x29
4947
ldp x29, x30, [sp], #16
5048
ret
5149
.size f_variable_large_stack , .-f_variable_large_stack
@@ -195,6 +193,18 @@ f_alloca_in_loop:
195193

196194
// CHECK-NOT: GS-STACKCLASH:
197195

196+
.global f_align_sp
197+
.type f_align_sp, %function
198+
f_align_sp:
199+
stp x29, x30, [sp, #-0x30]!
200+
mov x29, sp
201+
sub x9, sp, #0x1d0
202+
and sp, x9, #0xffffffffffffff80
203+
ldp x29, x30, [sp], 32
204+
ret
205+
.size f_align_sp, .-f_align_sp
206+
207+
198208
// TODO: also add a test case with nested alloca loops.
199209

200210
// TODO: check access with constant offset from SP through other

0 commit comments

Comments
 (0)