LukeSerne
commented
1 week ago This bug occurs because the address of the jump table is not determined correctly.
This happens because there is a mismatch between how the segment pcode op is implemented, and the pcode for the JMP instruction. The segment(a, b) pcode op returns the address (a << 4) + b. However, the pcode for the JMP word ptr CS:[BX + 0xb69] instruction is:
4b0a:09e5 2e ff a7 69 0b JMP word ptr CS:[BX + 0xb69]
$U3e00:4 = INT_RIGHT 0x4ba8a:4, 4:4
$U3f00:4 = INT_AND $U3e00:4, 0xf000:4
CS = SUBPIECE $U3f00:4, 0:4
$U2e80:2 = INT_ADD BX, 0xb69:2
$U3e00:4 = INT_RIGHT 0x4ba8a:4, 4:4
$U3f00:4 = INT_AND $U3e00:4, 0xf000:4
CS = SUBPIECE $U3f00:4, 0:4
$U4a80:4 = CALLOTHER "segment", CS, $U2e80:2
$U9200:2 = LOAD ram($U4a80:4)
$U29a80:4 = CALLOTHER "segment", CS, $U9200:2
BRANCHIND $U29a80:4We would expect CS to have the value 0x4b0a, but here, it's updated (twice?!). The root cause seems to be the definition of currentCS in ia.sinc. This macro uses the 32-bit address of the next instruction to calculate the current value of CS. Since inst_next == (CS << 4) + offset, this macro sets CS to (inst_next >> 4) & 0xF000 == (((CS << 4) + offset) >> 4) & 0xF000. However, without knowing offset, it's impossible to determine what the real value of CS is. However, CS is set to the correct value at the start of the function. As such, it seems best to not change it, and have the currentCS macro just export the CS varnode.
The only other location the CS varnode is changed, is in the ptr1616 and ptr1632 macros. I don't really understand how real-mode 16-bit x86 works, so I assumed these updates to CS are necessary. These macros are used for the CALLF and JMPF instruction. In the CALLF instruction, the control flow will return from the call, so we should restore the CS value afterwards. The JMPF will not return, and as such, we don't need to restore the CS value.
As said before, I barely have any experience with real-mode 16-bit x86, so I'm not sure how well these changes actually model reality, or if it's just an ugly hack that happens to work decently well for this one sample. Anyway, I pasted the patch I applied below.
diff --git a/Ghidra/Processors/x86/data/languages/ia.sinc b/Ghidra/Processors/x86/data/languages/ia.sinc
index dd8c841131..1bf5513a0c 100644
--- a/Ghidra/Processors/x86/data/languages/ia.sinc
+++ b/Ghidra/Processors/x86/data/languages/ia.sinc
@@ -1046,8 +1046,8 @@ addr64: [Base64 + Index64*ss + simm32_64] is mod=2 & r_m=4; Index64 & Base64 & s
addr64: [Base64 + Index64*ss] is mod=2 & r_m=4; Index64 & Base64 & ss; imm32=0 { local tmp=Base64+Index64*ss; export tmp; }
@endif
-currentCS: CS is protectedMode=0 & CS { tmp:4 = (inst_next >> 4) & 0xf000; CS = tmp:2; export CS; }
-currentCS: CS is protectedMode=1 & CS { tmp:4 = (inst_next >> 16) & 0xffff; CS = tmp:2; export CS; }
+currentCS: CS is protectedMode=0 & CS { export CS; }
+currentCS: CS is protectedMode=1 & CS { export CS; }
segWide: is segover=0 { export 0:$(SIZE); }
segWide: CS: is segover=1 & CS { export 0:$(SIZE); }
@@ -2526,10 +2526,10 @@ with : lockprefx=0 {
@endif
# direct far calls generate an opcode undefined exception in x86-64
-:CALLF ptr1616 is vexMode=0 & addrsize=0 & opsize=0 & byte=0x9a; ptr1616 { push22(CS); build ptr1616; push22(&:2 inst_next); call ptr1616; }
-:CALLF ptr1616 is vexMode=0 & addrsize=1 & opsize=0 & byte=0x9a; ptr1616 { push42(CS); build ptr1616; push42(&:2 inst_next); call ptr1616; }
-:CALLF ptr1632 is vexMode=0 & addrsize=0 & opsize=1 & byte=0x9a; ptr1632 { push22(CS); build ptr1632; push24(&:4 inst_next); call ptr1632; }
-:CALLF ptr1632 is vexMode=0 & addrsize=1 & opsize=1 & byte=0x9a; ptr1632 { pushseg44(CS); build ptr1632; push44(&:4 inst_next); call ptr1632; }
+:CALLF ptr1616 is vexMode=0 & addrsize=0 & opsize=0 & byte=0x9a; ptr1616 { tmp:2 = CS; push22(CS); build ptr1616; push22(&:2 inst_next); call ptr1616; CS = tmp; }
+:CALLF ptr1616 is vexMode=0 & addrsize=1 & opsize=0 & byte=0x9a; ptr1616 { tmp:2 = CS; push42(CS); build ptr1616; push42(&:2 inst_next); call ptr1616; CS = tmp; }
+:CALLF ptr1632 is vexMode=0 & addrsize=0 & opsize=1 & byte=0x9a; ptr1632 { tmp:2 = CS; push22(CS); build ptr1632; push24(&:4 inst_next); call ptr1632; CS = tmp; }
+:CALLF ptr1632 is vexMode=0 & addrsize=1 & opsize=1 & byte=0x9a; ptr1632 { tmp:2 = CS; pushseg44(CS); build ptr1632; push44(&:4 inst_next); call ptr1632; CS = tmp; }
:CALLF addr16 is vexMode=0 & addrsize=0 & opsize=0 & byte=0xff; addr16 & reg_opcode=3 ... { local ptr:$(SIZE) = segment(DS,addr16); local addrptr:$(SIZE) = segment(*:2 (ptr+2),*:2 ptr);
push22(CS); push22(&:2 inst_next); call [addrptr]; }
:CALLF addr32 is vexMode=0 & addrsize=1 & opsize=0 & byte=0xff; addr32 & reg_opcode=3 ... { local dest:4 = addr32; push42(CS); push42(&:2 inst_next); call [dest]; }With this patch applied, the pcode for the JMP instruction looks much more like I expected.
4b0a:09e5 2e ff a7 69 0b JMP word ptr CS:[BX + 0xb69]
$U2e80:2 = INT_ADD BX, 0xb69:2
$U4680:4 = CALLOTHER "segment", CS, $U2e80:2
$U8e00:2 = LOAD ram($U4680:4)
$U29880:4 = CALLOTHER "segment", CS, $U8e00:2
BRANCHIND $U29880:4Additionally, Ghidra is now able to correctly identify the jumptable, and it also correctly identifies the code for the switch cases. As such, the listing view also looks better:
And the decompiler is also able to correctly recover the switch cases. The decompiled code looks correct at a first glance.
NancyAurum
Describe the bug Borland C++ generates switches in form of
JMP word ptr CS:[BX + CaseTable], where CaseTable is placed at the end of a function, containing the switch, and holds CS relative offsets (1 for each case) inside the function. Ghidra ignores that table and generates completely unrelated references far away from the function.To Reproduce Steps to reproduce the behavior:
Other switches there follow the same scheme and are incorrectly analyzed too.
Expected behavior Ghidra uses the table properly to generate navigate around the switch.
Attachments st.zip
Environment (please complete the following information):
Additional context The work around is clearing existing references by the jump's address, and then manually adding a set of COMPUTED_JUMP references. The one runs SwitchOverride.java at the jump's address.
It would be nice if Ghidra did some sanity checks, that the jump target lies inside the function or anywhere near it. And if anything is suspicious, just asking user for further input, mentioning that SwitchOverride.java script.