GP-3441 Protect INDIRECT path to switch variable

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|

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<PcodeOp *> 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;i<indirects.size();++i) {
+    indirects[i]->setNoIndirectCollapse();
+  }
+}
+
+/// 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;i<bblocks.getSize();++i) {
+    PcodeOp *op = bblocks.getBlock(i)->lastOp();
+    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;

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; }

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

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())

Ghidra/Features/Decompiler/src/decompile/datatests/switchind.xml

@@ -0,0 +1,54 @@
+<decompilertest>
+<binaryimage arch="x86:LE:64:default:gcc">
+<bytechunk space="ram" offset="0x100010" readonly="true">
+4883ec18488d7c240cc744240c000000
+00e8da0f0000837c240c0a776b8b4424
+0c488d1580000000486304824801d0ff
+e000000000000000e8bb0f00004883c4
+18c3000000000000e8b30f00004883c4
+18c3000000000000e8ab0f00004883c4
+18c3000000000000e8a30f00004883c4
+18c3000000000000e89b0f00004883c4
+18c3000000000000e8930f00004883c4
+18c3
+</bytechunk>
+<bytechunk space="ram" offset="0x1000b8" readonly="true">
+                a0ffffffb0ffffff
+c0ffffffd0ffffff90ffffff90ffffff
+e0ffffffe0ffffffe0ffffffe0ffffff
+90ffffff
+</bytechunk>
+<symbol space="ram" offset="0x100010" name="switchind"/>
+</binaryimage>
+<script>
+  <com>option readonly on</com>
+  <com>map fun r0x101000 get_value_byref</com>
+  <com>map fun r0x101010 casefunc0</com>
+  <com>map fun r0x101018 casefunc1</com>
+  <com>map fun r0x101020 casefunc2</com>
+  <com>map fun r0x101028 casefunc3</com>
+  <com>map fun r0x101008 casefuncmany</com>
+  <com>map fun r0x101030 casefuncdefault</com>
+  <com>lo fu switchind</com>
+  <com>map addr s0xfffffffffffffff4 int4 val</com>
+  <com>decompile</com>
+  <com>print C</com>
+  <com>quit</com>
+</script>
+<stringmatch name="Switch Indirect #1" min="1" max="1">case 0:</stringmatch>
+<stringmatch name="Switch Indirect #2" min="1" max="1">case 1:</stringmatch>
+<stringmatch name="Switch Indirect #3" min="1" max="1">case 2:</stringmatch>
+<stringmatch name="Switch Indirect #4" min="1" max="1">case 3:</stringmatch>
+<stringmatch name="Switch Indirect #5" min="1" max="1">case 4:</stringmatch>
+<stringmatch name="Switch Indirect #6" min="1" max="1">case 5:</stringmatch>
+<stringmatch name="Switch Indirect #7" min="1" max="1">case 10:</stringmatch>
+<stringmatch name="Switch Indirect #8" min="1" max="1">default:</stringmatch>
+<stringmatch name="Switch Indirect #9" min="1" max="1">casefunc0\(\);</stringmatch>
+<stringmatch name="Switch Indirect #10" min="1" max="1">casefunc1\(\);</stringmatch>
+<stringmatch name="Switch Indirect #11" min="1" max="1">casefunc2\(\);</stringmatch>
+<stringmatch name="Switch Indirect #12" min="1" max="1">casefunc3\(\);</stringmatch>
+<stringmatch name="Switch Indirect #13" min="1" max="1">casefuncmany\(\);</stringmatch>
+<stringmatch name="Switch Indirect #14" min="1" max="1">casefuncdefault\(\);</stringmatch>
+<stringmatch name="Switch Indirect #15" min="1" max="1">get_value_byref\(&amp;val\);</stringmatch>
+<stringmatch name="Switch Indirect #16" min="1" max="1">switch\(val\)</stringmatch>
+</decompilertest>