From 0398e40b07ae9c926457ae9841ed8623830f134f Mon Sep 17 00:00:00 2001 From: caheckman <48068198+caheckman@users.noreply.github.com> Date: Tue, 16 May 2023 18:37:19 -0400 Subject: [PATCH] GP-3441 Protect INDIRECT path to switch variable --- .../Decompiler/certification.manifest | 1 + .../src/decompile/cpp/coreaction.cc | 54 +++++++++++++++++++ .../src/decompile/cpp/coreaction.hh | 2 + .../Decompiler/src/decompile/cpp/op.hh | 5 +- .../src/decompile/cpp/ruleaction.cc | 2 +- .../src/decompile/datatests/switchind.xml | 54 +++++++++++++++++++ 6 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 Ghidra/Features/Decompiler/src/decompile/datatests/switchind.xml diff --git a/Ghidra/Features/Decompiler/certification.manifest b/Ghidra/Features/Decompiler/certification.manifest index 317d26f55a..eb6a97953d 100644 --- a/Ghidra/Features/Decompiler/certification.manifest +++ b/Ghidra/Features/Decompiler/certification.manifest @@ -56,6 +56,7 @@ src/decompile/datatests/retstruct.xml||GHIDRA||||END| src/decompile/datatests/sbyte.xml||GHIDRA||||END| src/decompile/datatests/skipnext2.xml||GHIDRA||||END| src/decompile/datatests/statuscmp.xml||GHIDRA||||END| +src/decompile/datatests/switchind.xml||GHIDRA||||END| src/decompile/datatests/threedim.xml||GHIDRA||||END| src/decompile/datatests/twodim.xml||GHIDRA||||END| src/decompile/datatests/union_datatype.xml||GHIDRA||||END| diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc b/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc index 7efbd3e01a..5e3bfd1054 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc +++ b/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.cc @@ -2122,6 +2122,57 @@ int4 ActionLikelyTrash::apply(Funcdata &data) return 0; } +/// Test if the path to the given BRANCHIND originates from a constant but passes through INDIRECT operations. +/// This indicates that the switch value is produced indirectly, so we mark these INDIRECT +/// operations as \e not \e collapsible, to guarantee that the indirect value is not lost during analysis. +/// \param op is the given BRANCHIND op +void ActionRestructureVarnode::protectSwitchPathIndirects(PcodeOp *op) + +{ + vector indirects; + Varnode *curVn = op->getIn(0); + while(curVn->isWritten()) { + PcodeOp *curOp = curVn->getDef(); + uint4 evalType = curOp->getEvalType(); + if ((evalType & (PcodeOp::binary | PcodeOp::ternary)) != 0) { + if (curOp->numInput() > 1) { + if (!curOp->getIn(1)->isConstant()) return; // Multiple paths + } + curVn = curOp->getIn(0); + } + else if ((evalType & PcodeOp::unary) != 0) + curVn = curOp->getIn(0); + else if (curOp->code() == CPUI_INDIRECT) { + indirects.push_back(curOp); + curVn = curOp->getIn(0); + } + else if (curOp->code() == CPUI_LOAD) { + curVn = curOp->getIn(1); + } + else + return; + } + if (!curVn->isConstant()) return; + // If we reach here, there is exactly one path, from a constant to a switch + for(int4 i=0;isetNoIndirectCollapse(); + } +} + +/// Run through BRANCHIND ops, treat them as switches and protect the data-flow path to the destination variable +/// \param data is the function to examine +void ActionRestructureVarnode::protectSwitchPaths(Funcdata &data) + +{ + const BlockGraph &bblocks(data.getBasicBlocks()); + for(int4 i=0;ilastOp(); + if (op == (PcodeOp *)0) continue; + if (op->code() != CPUI_BRANCHIND) continue; + protectSwitchPathIndirects(op); + } +} + int4 ActionRestructureVarnode::apply(Funcdata &data) { @@ -2132,6 +2183,9 @@ int4 ActionRestructureVarnode::apply(Funcdata &data) if (data.syncVarnodesWithSymbols(l1,false,aliasyes)) count += 1; + if (data.isJumptableRecoveryOn()) + protectSwitchPaths(data); + numpass += 1; #ifdef OPACTION_DEBUG if ((flags&rule_debug)==0) return 0; diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.hh b/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.hh index 11d520d772..7cdff96df2 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.hh +++ b/Ghidra/Features/Decompiler/src/decompile/cpp/coreaction.hh @@ -831,6 +831,8 @@ public: /// This produces on intermediate view of symbols on the stack. class ActionRestructureVarnode : public Action { int4 numpass; ///< Number of passes performed for this function + static void protectSwitchPathIndirects(PcodeOp *op); ///< Protect path to the given switch from INDIRECT collapse + static void protectSwitchPaths(Funcdata &data); ///< Look for switches and protect path of switch variable public: ActionRestructureVarnode(const string &g) : Action(0,"restructure_varnode",g) {} ///< Constructor virtual void reset(Funcdata &data) { numpass = 0; } diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/op.hh b/Ghidra/Features/Decompiler/src/decompile/cpp/op.hh index 02e6906f24..8ae074c13e 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/op.hh +++ b/Ghidra/Features/Decompiler/src/decompile/cpp/op.hh @@ -114,7 +114,8 @@ public: is_cpool_transformed = 0x20, ///< Have we checked for cpool transforms stop_type_propagation = 0x40, ///< Stop data-type propagation into output from descendants hold_output = 0x80, ///< Output varnode (of call) should not be removed if it is unread - concat_root = 0x100 ///< Output of \b this is root of a CONCAT tree + concat_root = 0x100, ///< Output of \b this is root of a CONCAT tree + no_indirect_collapse = 0x200 ///< Do not collapse \b this INDIRECT (via RuleIndirectCollapse) }; private: TypeOp *opcode; ///< Pointer to class providing behavioral details of the operation @@ -219,6 +220,8 @@ public: void setPartialRoot(void) { addlflags |= concat_root; } ///< Mark \b this as root of CONCAT tree bool stopsCopyPropagation(void) const { return ((flags&no_copy_propagation)!=0); } ///< Does \b this allow COPY propagation void setStopCopyPropagation(void) { flags |= no_copy_propagation; } ///< Stop COPY propagation through inputs + bool noIndirectCollapse(void) const { return ((addlflags & no_indirect_collapse)!=0); } ///< Check if INDIRECT collapse is possible + void setNoIndirectCollapse(void) { addlflags |= no_indirect_collapse; } ///< Prevent collapse of INDIRECT /// \brief Return \b true if this LOADs or STOREs from a dynamic \e spacebase pointer bool usesSpacebasePtr(void) const { return ((flags&PcodeOp::spacebase_ptr)!=0); } uintm getCseHash(void) const; ///< Return hash indicating possibility of common subexpression elimination diff --git a/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc b/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc index e4961f9849..2704fe4afd 100644 --- a/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc +++ b/Ghidra/Features/Decompiler/src/decompile/cpp/ruleaction.cc @@ -2926,7 +2926,7 @@ int4 RuleIndirectCollapse::applyOp(PcodeOp *op,Funcdata &data) } } else if (indop->isCall()) { - if (op->isIndirectCreation()) + if (op->isIndirectCreation() || op->noIndirectCollapse()) return 0; // If there are no aliases to a local variable, collapse if (!op->getOut()->hasNoLocalAlias()) diff --git a/Ghidra/Features/Decompiler/src/decompile/datatests/switchind.xml b/Ghidra/Features/Decompiler/src/decompile/datatests/switchind.xml new file mode 100644 index 0000000000..407f96a4b1 --- /dev/null +++ b/Ghidra/Features/Decompiler/src/decompile/datatests/switchind.xml @@ -0,0 +1,54 @@ + + + +4883ec18488d7c240cc744240c000000 +00e8da0f0000837c240c0a776b8b4424 +0c488d1580000000486304824801d0ff +e000000000000000e8bb0f00004883c4 +18c3000000000000e8b30f00004883c4 +18c3000000000000e8ab0f00004883c4 +18c3000000000000e8a30f00004883c4 +18c3000000000000e89b0f00004883c4 +18c3000000000000e8930f00004883c4 +18c3 + + + a0ffffffb0ffffff +c0ffffffd0ffffff90ffffff90ffffff +e0ffffffe0ffffffe0ffffffe0ffffff +90ffffff + + + + +case 0: +case 1: +case 2: +case 3: +case 4: +case 5: +case 10: +default: +casefunc0\(\); +casefunc1\(\); +casefunc2\(\); +casefunc3\(\); +casefuncmany\(\); +casefuncdefault\(\); +get_value_byref\(&val\); +switch\(val\) +