Skip to content

Commit 6418012

Browse files
CouleeAppsCouleeApps
committed
Checking flags and temp registers better
1 parent 1123529 commit 6418012

File tree

3 files changed

+160
-17
lines changed

3 files changed

+160
-17
lines changed

‎plugins/lift_check/lifted_il_lift_check.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -239,7 +239,7 @@ void LiftedILVerifier::Verify()
239239
{
240240
if ((found->second & ValidAsParent) == 0 && expr.flags == 0)
241241
{
242-
m_diagnostics.push_back(Diagnostic::Error(this, expr, "Expression is not expected to be parent instruction"));
242+
m_diagnostics.push_back(Diagnostic::Diag(WarningSeverity, this, expr, "Expression is not expected to be parent instruction without setting flags"));
243243
}
244244
}
245245
else

‎plugins/lift_check/llil_lift_check.cpp‎

Lines changed: 150 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,25 @@ void LowLevelILVerifier::CheckExprSize(const LowLevelILInstruction& expr, std::o
232232
CHECK(expr.size != 0, "op should have a size");
233233

234234
auto reg = expr.GetSourceRegister<LLIL_REG>();
235-
if (!LLIL_REG_IS_TEMP(reg))
235+
if (LLIL_REG_IS_TEMP(reg))
236+
{
237+
if (auto found = m_tempRegSizes.find(reg); found != m_tempRegSizes.end())
238+
{
239+
CHECK(
240+
found->second.width == expr.size,
241+
"attempting to load {:#x} bytes out of {:#x} byte temporary register temp{} (first seen at {:?})",
242+
expr.size,
243+
found->second.width,
244+
LLIL_GET_TEMP_REG_INDEX(reg),
245+
m_il->GetExpr(found->second.seenExpr)
246+
);
247+
}
248+
else
249+
{
250+
m_diagnostics.push_back(Diagnostic::Error(this, expr, fmt::format("Using unknown/yet unseen temporary register temp{}", LLIL_GET_TEMP_REG_INDEX(reg))));
251+
}
252+
}
253+
else
236254
{
237255
auto info = m_arch->GetRegisterInfo(reg);
238256

@@ -260,13 +278,49 @@ void LowLevelILVerifier::CheckExprSize(const LowLevelILInstruction& expr, std::o
260278
CHECK(expr.size != 0, "op should have a size");
261279

262280
auto hi = expr.GetHighRegister<LLIL_REG_SPLIT>();
263-
if (!LLIL_REG_IS_TEMP(hi))
281+
if (LLIL_REG_IS_TEMP(hi))
282+
{
283+
if (auto found = m_tempRegSizes.find(hi); found != m_tempRegSizes.end())
284+
{
285+
CHECK(
286+
found->second.width == expr.size,
287+
"attempting to load {:#x} bytes out of {:#x} byte temporary register temp{} (first seen at {:?})",
288+
expr.size,
289+
found->second.width,
290+
LLIL_GET_TEMP_REG_INDEX(hi),
291+
m_il->GetExpr(found->second.seenExpr)
292+
);
293+
}
294+
else
295+
{
296+
m_diagnostics.push_back(Diagnostic::Error(this, expr, fmt::format("Using unknown/yet unseen temporary register temp{}", LLIL_GET_TEMP_REG_INDEX(hi))));
297+
}
298+
}
299+
else
264300
{
265301
auto info = m_arch->GetRegisterInfo(hi);
266302
CHECK(expr.size == info.size, "attempting to load {:#x} bytes out of a {:#x} byte hi register", expr.size, info.size);
267303
}
268304
auto lo = expr.GetLowRegister<LLIL_REG_SPLIT>();
269-
if (!LLIL_REG_IS_TEMP(lo))
305+
if (LLIL_REG_IS_TEMP(lo))
306+
{
307+
if (auto found = m_tempRegSizes.find(lo); found != m_tempRegSizes.end())
308+
{
309+
CHECK(
310+
found->second.width == expr.size,
311+
"attempting to load {:#x} bytes out of {:#x} byte temporary register temp{} (first seen at {:?})",
312+
expr.size,
313+
found->second.width,
314+
LLIL_GET_TEMP_REG_INDEX(lo),
315+
m_il->GetExpr(found->second.seenExpr)
316+
);
317+
}
318+
else
319+
{
320+
m_diagnostics.push_back(Diagnostic::Error(this, expr, fmt::format("Using unknown/yet unseen temporary register temp{}", LLIL_GET_TEMP_REG_INDEX(lo))));
321+
}
322+
}
323+
else
270324
{
271325
auto info = m_arch->GetRegisterInfo(lo);
272326
CHECK(expr.size == info.size, "attempting to load {:#x} bytes out of a {:#x} byte lo register", expr.size, info.size);
@@ -347,18 +401,18 @@ void LowLevelILVerifier::CheckExprSize(const LowLevelILInstruction& expr, std::o
347401
}
348402
case LLIL_FLAG_COND:
349403
{
350-
if (requiredSize.has_value() && *requiredSize != 0)
404+
if (requiredSize.has_value())
351405
{
352-
CHECK(expr.size == *requiredSize, "op producing boolean (0 size) value where {:#x} bytes are expected", *requiredSize);
406+
CHECK(*requiredSize == 0, "op expecting to produce {:#x} byte value but should be producing boolean (0 size)", *requiredSize);
353407
}
354408
CHECK(expr.size == 0, "op should be boolean (0 size) but is {:#x} size", expr.size);
355409
break;
356410
}
357411
case LLIL_FLAG_GROUP:
358412
{
359-
if (requiredSize.has_value() && *requiredSize != 0)
413+
if (requiredSize.has_value())
360414
{
361-
CHECK(expr.size == *requiredSize, "op producing boolean (0 size) value where {:#x} bytes are expected", *requiredSize);
415+
CHECK(*requiredSize == 0, "op expecting to produce {:#x} byte value but should be producing boolean (0 size)", *requiredSize);
362416
}
363417
CHECK(expr.size == 0, "op should be boolean (0 size) but is {:#x} size", expr.size);
364418
break;
@@ -656,9 +710,32 @@ void LowLevelILVerifier::CheckInstrSize(const LowLevelILInstruction& instr)
656710
break;
657711
case LLIL_SET_REG:
658712
{
659-
// TODO: how to do sanity checking for temp registers?
713+
CHECK(instr.size != 0, "op should have a size");
714+
660715
auto reg = instr.GetDestRegister<LLIL_SET_REG>();
661-
if (!LLIL_REG_IS_TEMP(reg))
716+
if (LLIL_REG_IS_TEMP(reg))
717+
{
718+
if (auto found = m_tempRegSizes.find(reg); found != m_tempRegSizes.end())
719+
{
720+
CHECK(
721+
found->second.width == instr.size,
722+
"setting {:#x} byte temporary register temp{} to {:#x} bytes (first seen at {:?})",
723+
found->second.width,
724+
LLIL_GET_TEMP_REG_INDEX(reg),
725+
instr.size,
726+
m_il->GetExpr(found->second.seenExpr)
727+
);
728+
}
729+
else
730+
{
731+
TempRegisterInfo info;
732+
info.reg = reg;
733+
info.width = instr.size;
734+
info.seenExpr = instr.exprIndex;
735+
m_tempRegSizes.insert({ reg, info });
736+
}
737+
}
738+
else
662739
{
663740
auto info = m_arch->GetRegisterInfo(reg);
664741

@@ -681,15 +758,60 @@ void LowLevelILVerifier::CheckInstrSize(const LowLevelILInstruction& instr)
681758
}
682759
case LLIL_SET_REG_SPLIT:
683760
{
684-
// TODO: how to do sanity checking for temp registers?
761+
CHECK(instr.size != 0, "op should have a size");
685762
auto hi = instr.GetHighRegister<LLIL_SET_REG_SPLIT>();
686-
if (!LLIL_REG_IS_TEMP(hi))
763+
if (LLIL_REG_IS_TEMP(hi))
764+
{
765+
if (auto found = m_tempRegSizes.find(hi); found != m_tempRegSizes.end())
766+
{
767+
CHECK(
768+
found->second.width == instr.size,
769+
"setting {:#x} byte temporary register temp{} to {:#x} bytes (first seen at {:?})",
770+
found->second.width,
771+
LLIL_GET_TEMP_REG_INDEX(hi),
772+
instr.size,
773+
m_il->GetExpr(found->second.seenExpr)
774+
);
775+
}
776+
else
777+
{
778+
TempRegisterInfo info;
779+
info.reg = hi;
780+
info.width = instr.size;
781+
info.seenExpr = instr.exprIndex;
782+
m_tempRegSizes.insert({ hi, info });
783+
}
784+
}
785+
else
687786
{
688787
auto info = m_arch->GetRegisterInfo(hi);
689788
CHECK(info.size == instr.size, "setting {:#x} byte hi register {} to {:#x} byte value", info.size, m_arch->GetRegisterName(hi), instr.size);
690789
}
691790
auto lo = instr.GetHighRegister<LLIL_SET_REG_SPLIT>();
692-
if (!LLIL_REG_IS_TEMP(lo))
791+
if (LLIL_REG_IS_TEMP(lo))
792+
{
793+
if (auto found = m_tempRegSizes.find(lo); found != m_tempRegSizes.end())
794+
{
795+
CHECK(
796+
found->second.width == instr.size,
797+
"setting {:#x} byte temporary register temp{} to {:#x} bytes (first seen at {:?})",
798+
found->second.width,
799+
LLIL_GET_TEMP_REG_INDEX(lo),
800+
instr.size,
801+
m_il->GetExpr(found->second.seenExpr)
802+
);
803+
}
804+
else
805+
{
806+
TempRegisterInfo info;
807+
info.reg = lo;
808+
info.width = instr.size;
809+
info.seenExpr = instr.exprIndex;
810+
m_tempRegSizes.insert({ lo, info });
811+
}
812+
813+
}
814+
else
693815
{
694816
auto info = m_arch->GetRegisterInfo(lo);
695817
CHECK(info.size == instr.size, "setting {:#x} byte lo register {} to {:#x} byte value", info.size, m_arch->GetRegisterName(lo), instr.size);
@@ -706,6 +828,7 @@ void LowLevelILVerifier::CheckInstrSize(const LowLevelILInstruction& instr)
706828
}
707829
case LLIL_SET_REG_STACK_REL:
708830
{
831+
CHECK(instr.size != 0, "op should have a size");
709832
auto regStack = instr.GetDestRegisterStack<LLIL_SET_REG_STACK_REL>();
710833
auto info = m_arch->GetRegisterStackInfo(regStack);
711834
auto firstReg = info.firstStorageReg;
@@ -719,6 +842,7 @@ void LowLevelILVerifier::CheckInstrSize(const LowLevelILInstruction& instr)
719842
}
720843
case LLIL_REG_STACK_PUSH:
721844
{
845+
CHECK(instr.size != 0, "op should have a size");
722846
auto regStack = instr.GetDestRegisterStack<LLIL_REG_STACK_PUSH>();
723847
auto info = m_arch->GetRegisterStackInfo(regStack);
724848
auto firstReg = info.firstStorageReg;
@@ -1113,8 +1237,11 @@ void LowLevelILVerifier::CheckExprOperands(const BinaryNinja::LowLevelILInstruct
11131237
auto condition = expr.GetFlagCondition<LLIL_FLAG_COND>();
11141238
CHECK(condition <= LLFC_FUO, "unknown flag condition {}", condition);
11151239
auto semClass = expr.GetSemanticFlagClass<LLIL_FLAG_COND>();
1116-
auto name = m_arch->GetSemanticFlagClassName(semClass);
1117-
CHECK(!name.empty(), "unknown semantic flag class {}", semClass);
1240+
if (semClass != 0)
1241+
{
1242+
auto name = m_arch->GetSemanticFlagClassName(semClass);
1243+
CHECK(!name.empty(), "unknown semantic flag class {}", semClass);
1244+
}
11181245
break;
11191246
}
11201247
case LLIL_FLAG_GROUP:
@@ -1216,6 +1343,7 @@ void LowLevelILVerifier::Verify()
12161343
(n/i for now)
12171344
-[-] No jumping to entry block
12181345
(currently allowed)
1346+
-[x] All blocks have source blocks
12191347
-[x] Sizes of expressions are consistent
12201348
-[-] Base level instructions are of a limited subset of operations (setreg, call, etc)
12211349
(they can technically be value expressions, e.g. subtractions, for setting flags)
@@ -1256,10 +1384,16 @@ void LowLevelILVerifier::Verify()
12561384
{
12571385
for (auto& outgoing: bb->GetOutgoingEdges())
12581386
{
1387+
auto source = bb->GetSourceBlock();
1388+
if (!source)
1389+
{
1390+
m_diagnostics.push_back(Diagnostic::Error(this, fmt::format("block {}->{} has no source block? (probably need to call Finalize again or something)", bb->GetStart(), bb->GetEnd())));
1391+
source = bb;
1392+
}
12591393
// TODO: This is currently valid but we want this to eventually be lifted as a tailcall
12601394
if (outgoing.target == entryBlock)
12611395
{
1262-
m_diagnostics.push_back(Diagnostic::Diag(WarningSeverity, this, fmt::format("block {:#x} jumps to entry block", bb->GetStart())));
1396+
m_diagnostics.push_back(Diagnostic::Diag(WarningSeverity, this, fmt::format("block {:#x}->{:#x} jumps to entry block (probably a bug in core's Finalize)", source->GetStart(), source->GetEnd())));
12631397
}
12641398
}
12651399
}
@@ -1338,7 +1472,7 @@ void LowLevelILVerifier::Verify()
13381472
{
13391473
// TODO: This is sometimes due to a bug in FlagsResolver
13401474
// where it doesn't duplicate the expressions used in the condition
1341-
m_diagnostics.push_back(Diagnostic::Diag(NoteSeverity, this, expr, "Expression used more than once"));
1475+
m_diagnostics.push_back(Diagnostic::Diag(NoteSeverity, this, expr, "Expression used more than once (probably a bug in core's FlagsResolver)"));
13421476
}
13431477
return true;
13441478
});

‎plugins/lift_check/llil_lift_check.h‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ class LowLevelILVerifier: public ILVerifier
1111
BinaryNinja::Ref<BinaryNinja::LowLevelILFunction> m_il;
1212
BinaryNinja::Ref<BinaryNinja::Architecture> m_arch;
1313

14+
struct TempRegisterInfo
15+
{
16+
size_t reg;
17+
size_t width;
18+
size_t seenExpr;
19+
};
20+
21+
std::unordered_map<size_t, TempRegisterInfo> m_tempRegSizes;
22+
1423
LowLevelILVerifier(BNFunctionGraphType graphType, BinaryNinja::Ref<BinaryNinja::LowLevelILFunction> function);
1524

1625
void CheckExprSize(const BinaryNinja::LowLevelILInstruction& expr, std::optional<size_t> requiredSize);

0 commit comments

Comments
 (0)