x86: decode 16-bit addressing modes correctly

[zardus]

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Problem

cmp dx, word ptr [di] reads %bp. This is 3b 15 at 0xf9c98 in tests/i386/bios.bin.elf, the SeaBIOS image angr/binaries tracks, lifted in the real mode the BIOS runs in:

------ IMark(0xf9c98, 2, 0) ------
t4 = GET:I16(offset=28)      <- guest_EBP.  The instruction names %di, offset 36.
t5 = 16Uto32(t4)
t3 = t5
t2 = GET:I16(offset=16)
t1 = LDle:I16(t3)
t0 = Sub16(t2,t1)

It does not fault, does not decline, and returns Ijk_Boring with the correct instruction length. Of the 24 memory forms of a 16-bit ModRM byte, one is right; eleven decode and read the wrong base register like this, and twelve refuse outright with a vpanic. Put a segment override in front and all 24 refuse.

The bytes reported in 2017 at angr/pyvex#80 still show it: 67 20 74 6f is and %dh, 0x6f(%si), and on master it lifts cleanly reading offset 24, which is %sp. Only the sanity-check failure that issue was closed on is gone.

Both routes into this code are live. One is the 0x67 address-size override. The other has no prefix at all: disInstr_X86_WRK sets current_sz_addr = 2 for every instruction when archinfo->x86_cr0 & 1 is clear, so in real mode every memory ModRM takes this path. Over the 4,323 instructions of that BIOS image's .text that do, 3,521 refuse, 687 compute the address from the wrong registers, 66 come out the wrong length, 2 emit no load or store to check, and 47 show no defect under those checks -- one of which takes the wrong immediate anyway.

Root cause

disAMode16 squeezes mod and r/m into a five-bit key and hands the r/m half straight to getIReg:

case 0x04: case 0x05: case 0x07:
   { UChar rm = mod_reg_rm;
     *len = 1;
     return disAMode_copy2tmp(
            handleSegOverride(sorb, getIReg(2,rm)));
   }

getIReg numbers registers ax cx dx bx sp bp si di, but a 16-bit r/m field means (%bx,%si) (%bx,%di) (%bp,%si) (%bp,%di) (%si) (%di) (%bp) (%bx). So (%si) reads %sp, (%di) reads %bp, (%bp) reads %si and (%bx) reads %di.

The four base+index forms have no implementation: r/m 0-3 hit vpanic("TODO disAMode16 1") at mod 0 and vpanic("TODO disAMode16 2") at mod 1, and mod 2 falls into the default. A negative 8-bit displacement fails mkU16's vassert(i < 65536), since getSDisp8 sign-extends to 32 bits, and a segment override reaches handleSegOverride as an Ity_I16 where a 32-bit address is wanted.

lengthAMode dispatches on protected_mode while disAMode dispatches on current_sz_addr, and those differ whenever a 0x67 prefix is in play, so such an amode is decoded 16-bit and measured 32-bit. Everything that finds its immediate through lengthAMode -- Grp1/2/3/5/8, SHLD/SHRD, the FPU escapes -- then reads it from the wrong byte: 67 83 06 34 12 05 is addl $5, 0x1234 and lifts with t1 = 0x00000034, the low half of the displacement. lengthAMode16 is separately wrong for 17 of the 32 mod/rm combinations, and that is not latent either: in real mode protected_mode is false, so it runs with no prefix at all, and 83 3e 20 df 00 -- cmpw $0, 0xdf20, at 0xfe05d in the same BIOS image -- takes 0xffdf for its immediate instead of 0.

Fix

Rewrite disAMode16 and lengthAMode16 around mod and r/m, with the 16-bit r/m table in one place, and dispatch lengthAMode on current_sz_addr so it agrees with disAMode. The displacement is masked back to 16 bits and the address is widened to 32 before the segment base is added, which is where the wrap belongs; disAMode_copy2tmp becomes a plain copy again. The same instruction at the same address:

------ IMark(0xf9c98, 2, 0) ------
t5 = GET:I16(offset=36)      <- guest_EDI
t4 = 16Uto32(t5)
t3 = t4
t2 = GET:I16(offset=16)
t1 = LDle:I16(t3)
t0 = Sub16(t2,t1)

What a 16-bit address size means for the instructions that take no ModRM byte is deliberately not touched. MOVS, CMPS, STOS, LODS and SCAS still address off %esi/%edi rather than %si/%di, LOOP and JCXZ still count in %ecx, XLAT still indexes off %ebx, the A0-A3 moffs forms still take a 32-bit offset, and in real mode PUSH and POP still address the stack through 32-bit %esp. guest_amd64_toIR.c handles the prefixed cases through haveASO; this front end has no equivalent, before or after.

Testing

The regression lands with the consumer in angr/pyvex, which pins its vex submodule to this head: it walks all 24 memory forms of the 16-bit ModRM byte and asserts each reads the registers the r/m table names, then covers the negative displacement, the segment override and the immediate. Five of its seven tests fail on the merge base.

An exhaustive differential over every (0x67?, 0x0F?, opcode, ModRM) combination, 262,144 lifts per side, run in both processor modes: in protected mode, where only the prefix reaches this code, all 130,050 cases carrying no 0x67 byte anywhere are byte-identical on both sides, and 42,962 of the 132,094 that do carry one differ. In real mode, where every memory ModRM reaches it, 32,897 of those same 130,050 differ. On the BIOS image above, refusals fall from 3,521 to 470 and instructions with no detectable defect rise from 47 to 3,719 out of 4,323, with nothing that decoded before refusing now. Validation: #97 (comment)

🤖 Generated with Claude Code

session: sharpen

[zardus]

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Validation record for head 5e255516e977171c07fb0bd45dcaf5c08b80596c against baseline e3062871112afded52ae155757113a5873e9819c.

Rebased onto vex master after the Valgrind 3.27.1 import (#98) and the AVX512 series (#99) landed. git range-diff reports the one commit unchanged and priv/guest_x86_toIR.c is at blob 0c061974996a977d06594590f66782ab3bd99fa7 on the old head and the new one, so the patch text did not move -- but the file underneath it did, by 708 insertions and 530 deletions in the import, so the figures below are measurements at e3062871/5e25551, not the ones this record carried at 875f7c9a/fb71d75.

• Regression: pytest -q tests/test_x86_addr16.py in the consumer at its pinned head — 5 of 7 fail on the baseline (test_addr16_modrm_reads_the_right_registers, test_addr16_negative_disp8, test_addr16_segment_override, test_addr16_immediate_follows_the_16_bit_amode, test_addr16_in_real_mode_needs_no_prefix), 7 pass on the head
• Full suite: consumer pytest --import-mode=append -q tests — 88 passed and 5 failed on the baseline, 93 passed on the head. pyvex master's own tree is 86 passed; the seven extra are the consumer branch's new file
• Consumer of the consumer: two nix-built angr environments that differ in libVEX and nothing else — diff -rq -x __pycache__ over each one's whole site-packages reports four files: pyvex/lib/libpyvex.so, angr/unicornlib.so, which links against it, and the two RECORD files that hash them. Every .py file and angr/rustylib.abi3.so are byte-identical. The -x is load-bearing and not a convenience: without it the same command reports 3,284 differences, because a .pyc embeds the store path it was compiled under. Both arms give pytest --import-mode=append -q tests/analyses/cfg -n 4 = 238 passed, 9 skipped, 28 subtests passed, from angr at 23b470d9f, and both pass test_cfgemulated.py -k test_cfg_6, the CFGEmulated run over the BIOS image this change is about
• Build: make -f Makefile-gcc -j 6 all — exit 0; the only two warnings it emits are master's own, in guest_amd64_toIR_AVX512.c, and neither names guest_x86_toIR.c. gcc -c -DPYVEX -Ipub -Ipriv -Wall -Werror=implicit-function-declaration on the changed file alone — no output, exit 0, and exit 0 on the base too, with a file calling an undeclared function rejected by the same command as the control that the flag speaks. That flag is here because a base that moved by an import is where a clean replay can still stop compiling
• Lint/type: not applicable, C only
• Workspace gate, run on the consumer at its pinned head: the complete local gate for this exact head, all fourteen suites selected. Thirteen passed, among them pyvex, cle, angr, archinfo, pypcode, pysoot, the Rust suite, the headless GUI suite, the full pre-commit hook set over every checkout, and the test-input and packaging checks. The fourteenth is this workspace's own harness suite, and it failed on six tests belonging to an unrelated internal tool that another job was editing while the run was in flight; the gate's worktree check named those same files and nothing else. Each of the six passes at that tool's committed revision, run one at a time, so neither failure is reachable from this change -- which touches one test file and a submodule pointer

Exhaustive opcode differential. Every combination of the 0x67 prefix, the 0x0F escape, all 256 opcode bytes and all 256 ModRM bytes, followed by the fixed 12-byte tail 11 22 33 44 55 66 77 88 99 AA BB CC — 262,144 lifts per side with ArchX86, opt_level=0, max_inst=1, compared on jumpkind, size and a hash of the statement list. Run in both processor modes, because which of them is in force decides whether this code is reached at all: the default x86_cr0 is protected mode, where only a 0x67 prefix reaches it, and x86_cr0 = 0xFFFFFFFE is real mode, where every memory ModRM does:

	Cases	Differing, protected	Differing, real
The byte 0x67 appears nowhere in the instruction	130,050	0	32,897
0x67 present, in any byte position	132,094	42,962	1,722

The top-left cell is the no-regression control: in the mode where this change is not supposed to be reachable without the prefix, nothing without the prefix moves. The cell beside it, 32,897, is the no-prefix half of the change and is the same population the real-mode census below sweeps. In protected mode all 42,962 differences carry a 0x67; the 84 that are not in the prefixed half have one in the opcode or ModRM slot. The three cells that are not the control depend on the tail, which is why it is written out above — a tail of twelve 90 bytes gives 32,861, 42,892 and 166, in the same order as the table, and moves the 84 to 67. The 130,050 and 0 is the same under both tails. Against the record this replaces those three cells read 32,129, 42,190 and 1,718, and the 90-tail protected figure 42,120; the control cell was 0 there too.

Real-mode census over tests/i386/bios.bin.elf at angr/binaries fc07821c89535534979b02760e7e1bfc35faf690, where it is blob 4bbc57934f4e9c103585b3c32bb4072e0f9e708a — the same bytes as at the 003e82a2 this record used to cite and as at 0166109eb1fa0aec5baefb12403a487268f0e9ca. That file is an EM_386 ET_EXEC SeaBIOS image with one SHF_EXECINSTR section, .text at 0xf9c10, 25,584 bytes, and angr/tests/analyses/cfg/test_cfgemulated.py already runs CFGEmulated over it. Linear sweep in capstone CS_MODE_16, restarting one byte past each stall: 10,461 instructions, of which 4,323 carry a memory operand and no 0x67 address-size prefix, so they reach disAMode16 through current_sz_addr alone. The prefix is read from capstone's decoded prefix slots; scanning the instruction's bytes for 0x67 instead gives 4,311, because twelve of them carry that byte as a displacement or an immediate and still reach this code. Each lifted at its real address with vex_archinfo["x86_cr0"] = 0xFFFFFFFE, opt_level=0, max_inst=1. The rows are a partition, in this order:

Result	Baseline	Head
Ijk_NoDecode, size 0	3,521	470
Decoded, instruction length disagrees with capstone	66	66
Decoded, right length, no load or store in the IR to check	2	11
Decoded, right length, address computed from the wrong registers	687	57
Decoded, right length, address computed from the right registers	47	3,719

• The register rows are a dataflow check, not a text match: the guest registers reaching the address temp of a load or store, followed back through the temp definitions, must be exactly the base and index capstone names. A register the instruction reads as an operand does not reach that temp and is not counted. Only the eight general-purpose registers are counted; a segment override also pulls the selector and the descriptor-table pointers into that expression, and counting those too moves the head's last two rows to 102 and 3,674 while leaving the baseline's five rows unchanged
• Deterministic deltas: 3,051 instructions go from Ijk_NoDecode to decoding and 0 go the other way; 641 of the baseline's 687 address-register errors are corrected, so the head's 57 are the 46 that carry over plus 11 that the baseline refused outright and that now decode into that row; 0 instructions leave the last row
• The last row does not cover immediates, and one of the baseline's 47 has a wrong one: 83 3e 20 df 00 at 0xfe05d, cmpw $0, 0xdf20, takes 0xffdf where the head takes 0. So 46 of the 47 are right on the baseline. That instruction carries no 0x67, which is the lengthAMode16 half of this change
• The 470 remaining refusals are not addressing-mode refusals. Re-lifting each with a 32-bit address and operand size: 334 refuse there too, so the opcode is not implemented by this front end at all (insb 49, sldt 39, str 30, verr 27, arpl 26, lcall 22, outsw 20, verw 19, bound 17, insw 17, outsb 17, test 15 — a Grp3 sub-opcode gap that refuses in 32-bit mode too — ljmp 13, ltr 5, and 18 more — lldt 4, mov 4, les 3, and 2, lss 2, lds 1, outsd 1, ror 1); the other 136 decode there, so the guard is on the operand size (case 0x8D LEA 84 and case 0x0F 0xB7 MOVZX 24 both goto decode_failure when sz != 4, plus Grp5 near call 16 and jmp 11, and pcmpgtd 1)
• The 66 length disagreements are identical on both sides and every one is an A0-A3 moffs form: 23 a1, 17 a3, 14 a0, 12 a2, all mov, same length on both. That is in the not-done list in the description
• The 57 remaining register rows are both measurement-side and defect-side: 38 are MOVS/CMPS forms, where capstone reports one of the instruction's two memory operands and the address check is comparing against half of what it uses — those really do address off %esi/%edi rather than %si/%di, which is the not-done list again; the other 19 are PUSH/POP, whose memory address is right and whose extra register is the 32-bit %esp they push through, which the not-done list also names
• The 2 to 11 rise in the third row is LIDT, LGDT and FSTP becoming decodable and emitting no load or store whose address can be inspected: lidt goes 1 to 6, lgdt and fstp 0 to 2 each. The single LEA in that row is on both sides and does not move

Prior art. The reproducer in angr/pyvex#80, closed in 2017, is 67 20 74 6f, and %dh, 0x6f(%si). The sanityCheckFail it was filed for is gone; on the baseline the instruction lifts Ijk_Boring size 4 reading offset 24, which is %sp, and on the head it reads offset 32, which is %si.

Upstream Valgrind has none of this machinery: disAMode16, lengthAMode16, current_sz_addr, protected_mode and x86_cr0 each appear 0 times in its priv/guest_x86_toIR.c, and its prefix loop has no case 0x67. All of it entered this fork in 4b83714. There is nothing to backport.

0x67 in the public corpus, stated so it is not overread: 124 tracked i386 objects at angr/binaries fc07821c (80 ELF EM_386, 44 PE 0x14c), 25,025,691 executable bytes swept in CS_MODE_32 — every SHF_EXECINSTR ELF section and every IMAGE_SCN_MEM_EXECUTE PE section, a PE section counted at the smaller of its raw and virtual size — 1,872 0x67-prefixed memory-ModRM instructions across 17 objects, the prefix read from capstone's decoded prefix slots. 1,074 of those are in tests/x86_64/windows/Project1.vmp.exe, a VMProtect-packed PE where linear sweep is corroborated by nothing. An earlier revision of this record gave the byte total as 25,037,366; the object, instruction and object-with-hits counts re-derive exactly and only the byte total moved, on how a PE section's executable extent is measured. The real-mode census above is the evidence for this change; the prefixed count is not.

Caveats: the consumer's tests are the only executable regression, since this repository's CI builds and does not lift. No corpus decompilation sweep was run — this is a decode change on an architecture and mode nothing in the sweep corpora is loaded as.

[zardus]

THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS

Three instructions from the .text of tests/i386/bios.bin.elf, a SeaBIOS image, lifted at their real addresses with ArchX86, opt_level=0, max_inst=1 and vex_archinfo["x86_cr0"] = 0xFFFFFFFE. That is the mode the code runs in on hardware, and the mode angr reaches through a guest mov cr0: angr/tests/analyses/cfg/test_cfgemulated.py's CFGEmulated run over this file makes 4 of its lift_vex calls that way, all at 0xfe29c; the rest are in protected mode, where only a 0x67 prefix reaches this code. That run makes 6,405 calls in total against angr 23b470d9f -- the total moves with angr master, the 4 does not. Guest offsets: 8 EAX, 16 EDX, 17 DH, 20 EBX, 28 EBP, 32 ESI, 36 EDI. Each block is str(stmt) over irsb.statements; the NEXT expression is not a statement, so it is not shown.

Before — the first two decode and read the wrong base register; the third does not decode:

angr/vex master `e3062871`

; 3b15  cmp dx, word ptr [di]   at 0xf9c98
; jumpkind Ijk_Boring, size 2 (the instruction is 2 bytes)
    ------ IMark(0xf9c98, 2, 0) ------
    t4 = GET:I16(offset=28)
    t5 = 16Uto32(t4)
    t3 = t5
    t2 = GET:I16(offset=16)
    t1 = LDle:I16(t3)
    t0 = Sub16(t2,t1)
    PUT(offset=40) = 0x00000005
    t6 = 16Uto32(t2)
    PUT(offset=44) = t6
    t7 = 16Uto32(t1)
    PUT(offset=48) = t7
    PUT(offset=52) = 0x00000000
    PUT(offset=68) = 0x000f9c9a

; 00b70500  add byte ptr [bx + 5], dh   at 0xf9c13
; jumpkind Ijk_Boring, size 4 (the instruction is 4 bytes)
    ------ IMark(0xf9c13, 4, 0) ------
    t6 = GET:I16(offset=36)
    t5 = Add16(t6,0x0005)
    t4 = t5
    t7 = 16Uto32(t4)
    t3 = t7
    t2 = LDle:I8(t3)
    t1 = GET:I8(offset=17)
    t0 = Add8(t2,t1)
    STle(t3) = t0
    PUT(offset=40) = 0x00000001
    t8 = 8Uto32(t2)
    PUT(offset=44) = t8
    t9 = 8Uto32(t1)
    PUT(offset=48) = t9
    PUT(offset=52) = 0x00000000
    PUT(offset=68) = 0x000f9c17

; 0300  add ax, word ptr [bx + si]   at 0xf9c11
; jumpkind Ijk_NoDecode, size 0 (the instruction is 2 bytes)

After — [di] reads EDI, [bx+5] reads EBX, and [bx+si] decodes as the sum of EBX and ESI:

with this change

; 3b15  cmp dx, word ptr [di]   at 0xf9c98
; jumpkind Ijk_Boring, size 2 (the instruction is 2 bytes)
    ------ IMark(0xf9c98, 2, 0) ------
    t5 = GET:I16(offset=36)
    t4 = 16Uto32(t5)
    t3 = t4
    t2 = GET:I16(offset=16)
    t1 = LDle:I16(t3)
    t0 = Sub16(t2,t1)
    PUT(offset=40) = 0x00000005
    t6 = 16Uto32(t2)
    PUT(offset=44) = t6
    t7 = 16Uto32(t1)
    PUT(offset=48) = t7
    PUT(offset=52) = 0x00000000
    PUT(offset=68) = 0x000f9c9a

; 00b70500  add byte ptr [bx + 5], dh   at 0xf9c13
; jumpkind Ijk_Boring, size 4 (the instruction is 4 bytes)
    ------ IMark(0xf9c13, 4, 0) ------
    t6 = GET:I16(offset=20)
    t5 = Add16(t6,0x0005)
    t4 = 16Uto32(t5)
    t3 = t4
    t2 = LDle:I8(t3)
    t1 = GET:I8(offset=17)
    t0 = Add8(t2,t1)
    STle(t3) = t0
    PUT(offset=40) = 0x00000001
    t7 = 8Uto32(t2)
    PUT(offset=44) = t7
    t8 = 8Uto32(t1)
    PUT(offset=48) = t8
    PUT(offset=52) = 0x00000000
    PUT(offset=68) = 0x000f9c17

; 0300  add ax, word ptr [bx + si]   at 0xf9c11
; jumpkind Ijk_Boring, size 2 (the instruction is 2 bytes)
    ------ IMark(0xf9c11, 2, 0) ------
    t6 = GET:I16(offset=32)
    t7 = GET:I16(offset=20)
    t5 = Add16(t7,t6)
    t4 = 16Uto32(t5)
    t3 = t4
    t2 = GET:I16(offset=8)
    t1 = LDle:I16(t3)
    t0 = Add16(t2,t1)
    PUT(offset=40) = 0x00000002
    t8 = 16Uto32(t2)
    PUT(offset=44) = t8
    t9 = 16Uto32(t1)
    PUT(offset=48) = t9
    PUT(offset=52) = 0x00000000
    PUT(offset=8) = t0
    PUT(offset=68) = 0x000f9c13