Fix heap corruption on instructions that outgrow the parse tree - #289
Fix heap corruption on instructions that outgrow the parse tree#289zardus wants to merge 1 commit into
Conversation
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Validation record for head
Reproducers, one instruction per process,
Which glibc check fires, and whether the process aborts or faults, depends on heap layout and varies between builds; the PowerPC and ARM deaths happen after Tree sizes, from a build carrying a counter in
The operand counts come from the shipped specs:
Caveats:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #289 +/- ##
=======================================
Coverage 86.82% 86.82%
=======================================
Files 5 5
Lines 516 516
Branches 82 82
=======================================
Hits 448 448
Misses 26 26
Partials 42 42 ☔ View full report in Codecov by Harness. |
ParserContext holds the parse tree of the instruction being decoded in three
fixed arrays: a node array and a per-node operand array, both sized once by
ParserContext::initialize, and the walker's breadcrumb trail. allocateOperand
indexed all three without checking any of them, so an instruction whose tree
did not fit wrote past the end of two heap blocks and into the walker's own
context member. Translation returned normally with correct p-code and the
process died later, in an unrelated malloc or free, with a glibc abort or a
segfault.
Ordinary encodings reach every one of the three. PowerPC AltiVec spells
arithmetic out one operand per vector lane, so vaddubm declares 24 operands and
vadduhm and vsubuhm declare 27, against an allotment of 20. A register list
takes one Constructor per register, so ARM vldmia r0,{s0-s19} needs 76 nodes
against 75, and vldmia r0,{s0-s30} needs 109 and nests 36 deep against a
32-entry breadcrumb trail. NDS32 register-list instructions reach 120 nodes.
Give the node array room for 512 nodes and the breadcrumb trail room for 128,
have setConstructor size a node's operand array to the Constructor it is
attached to, and have allocateOperand raise BadDataError rather than allocate a
node the arrays cannot hold. BadDataError is already how oneInstruction reports
undecodable input, so a tree that genuinely has no bound of its own -- a JVM
lookupswitch, which nests one level per table entry and takes the entry count
from the instruction stream -- now reaches the caller as a catchable exception.
Refusing abandons a half-built tree in a cached ParserContext, so the tests
cover that the Context stays usable afterwards.
Over 20,000 random inputs against each of the 187 shipped languages, no
instruction outside JVM reaches either limit, and the largest tree any of them
builds is 120 nodes at 36 levels.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
478e663 to
54d3ad5
Compare
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS A note on an interaction with #288, found while building both. This branch makes a crash in Nothing here is wrong — this branch does what it says, and it fixes JVM, NDS32 and the PowerPC case in #292, none of which #288 touches. It is only that the two overlap in a way that could hide a regression: with this merged, a delay-slot use-after-return will often not fault, so #288 should be evaluated on its own tree rather than on top of this one. |
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS Corpus frequency for this, from a sweep that has since completed — and it cuts both ways, so both halves are here. Of 746,966 objects analysed, 479 died on a signal. This defect accounts for 115 of them, 24% of every native crash and the second largest cause. It also covers the PowerPC The tempering half: all 115 are 32-to-128-byte synthetic blobs from a generated p-code collection. Not one is a real-world binary. So the rank measures the fuzz corpus, not exposure — by real-world objects the same run puts a delay-slot use-after-return (#288) at 220 and an out-of-memory family at 40 ahead of it. That does not make it less of a defect. One interaction worth knowing, from building both branches separately: this change makes #288's symptom disappear without fixing it. Detail is in a separate comment there and on #288; the short version is that growing |
|
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS
Before — five encodings return correct p-code and then abort in an unrelated allocator call, and two fault outright: pypcode master, 559aacdAfter — six decode and exit cleanly, and the unbounded JVM with this change, 54d3ad5 |
1 participant
THIS MESSAGE WAS GENERATED BY AN AUTOMATED PROCESS
Problem
Context.translatecorrupts the heap on ordinary encodings.0x10000000is PowerPCvaddubm v0,v0,v0: translation returns correct p-code, and the process dies afterwards in an unrelated allocator call.Six more encodings across three shipped languages do the same —
0x10000040and0x10000440(vadduhm,vsubuhm), ARM14 0a 90 ecand1f 0a 90 ec(vldmiawith 20 and with 31 registers), NDS323b c5 37 45(lmwa.bim) — dying as SIGABRT or SIGSEGV depending on heap layout. The write happens inside a translation that then succeeds, so the death has no relation in time or place to the instruction that caused it.Root cause
Every cached parse tree gets a fixed allotment, 75 nodes of 20 operands each in
pypcode/sleigh/sleigh.cc, and a fixed path from the root inpypcode/sleigh/context.hh:ParserContext::allocateOperandindexes all three and checks none of them:ConstructState *opstate = &state[alloc++]; ... walker.point->resolve[i] = opstate; walker.breadcrumb[walker.depth++] += 1;Measured on an instrumented build, one instruction per process:
10 00 00 00vaddubm10 00 00 40vadduhm14 0a 90 ecvldmia, 20 registers1f 0a 90 ecvldmia, 31 registers3b c5 37 45lmwa.bimAltiVec arithmetic names one operand per vector lane, so
vaddubmruns off the 20-entry operand array. A register list is one Constructor per register, sovldmiaandlmwa.bimrun off the 75-node array, and at 31 registers off the 32-entry breadcrumb trail as well. A JVMlookupswitch,ab, recurses once per table entry with the count read from the instruction stream, so its tree has no bound at all.Fix
Size a node's operand array to the Constructor attached to it, in
ParserWalkerChange::setConstructor; raise the node allotment to 512 and the breadcrumb trail to 128, several times the largest tree measured above; and haveallocateOperandraiseBadDataErrorrather than overrun either. That is already howtranslateanddisassemblereport undecodable input (pypcode/pypcode_native.cpp:309), and angr maps it toIjk_NoDecode. All seven encodings then decode or refuse cleanly, the JVM one as:lookupswitchstill does not decode; refusing it is containment, not support.These sources are vendored from Ghidra 12.1.3 unchanged, so every released Ghidra carries the same defect. Upstream master rewrote this region in
5328fa2c6dbf(GP-6554) with equivalent bounds, which pypcode has not picked up.Testing
ParseTreeTestsdecodes each encoding in a child interpreter, because master returns fromtranslatebefore dying and the process doing it cannot observe the damage;test_context_is_reusable_after_a_refusalcovers that a refused instruction leaves the cachedParserContextusable. Against a baseline build,-k ParseTreeover this branch's tests reports8 failed, 2 passed, every failure a child killed by a signal (assert -11 == 0,assert -6 == 0); on this branch, 4 passed and 6 subtests passed.Growing
ParserWalkeralso moves every stack frame holding a walker, which makes a separate delay-slot use-after-return in sparc and MIPS stop faulting without repairing it. pypcode 288 is that fix, and it has to be judged on its own tree rather than on top of this one.Validation: #289 (comment)
session: sharpen