Closed Bug 696748 Opened 8 years ago Closed 8 years ago

[ARM] Assertion failure: (inst & mask) == expected, at ../methodjit/ICChecker.h:56


(Core :: JavaScript Engine, defect, critical)

Not set



Tracking Status
firefox8 - wontfix
firefox9 - wontfix
firefox10 + fixed
firefox11 + fixed
firefox12 + fixed
status1.9.2 --- unaffected


(Reporter: decoder, Assigned: mjrosenb)


(Blocks 1 open bug)


(Keywords: assertion, crash, testcase, Whiteboard: [sg:critical] js-triage-needed)


(1 file)

The following test crashes on mozilla-central revision e79245e249c4 (options -m -n -a), armv7-a arch only:

try {"b", "".substring);
} catch(exc1) {}
var URI = '';\
function test() {\
  URI += '<zcti application=\"xxxx_demo\">';\
  URI += '<pstn_data>';\
  URI += '<dnis>877-485-xxxx</dnis>';\
  URI += '</pstn_data>';\
  URI >>=  '<keyvalue key=\"name\" value=\"xxx\"/>';\
  URI += '<keyvalue key=\"phone\" value=\"6509309000\"/>';\

The optimized build does not crash, but stepping through the assertion causes a crash (but the assert also differs here). It might be that the testcase requires additional tweaking to reveal the issue on opt. Marking this as S-s due to the crash. If the crash is debug-only, feel free unhide.

Backtrace of crash:

Program received signal SIGABRT, Aborted.
Assertion failure: (*insn & 0x012fff30) == 0x012fff30
(../assembler/assembler/ARMAssembler.h:994 static JSC::ARMWord* JSC::ARMAssembler::getLdrImmAddress(JSC::ARMWord*))

Program received signal SIGSEGV, Segmentation fault.
0x00282b98 in JSC::ARMAssembler::getLdrImmAddress (insn=0x4009a188) at ../assembler/assembler/ARMAssembler.h:994
994                     ASSERT((*insn & 0x012fff30) == 0x012fff30);
(gdb) bt
#0  0x00282b98 in JSC::ARMAssembler::getLdrImmAddress (insn=0x4009a188) at ../assembler/assembler/ARMAssembler.h:994
#1  0x00282c5c in JSC::ARMAssembler::patchPointerInternal (from=1074373000, to=0x2f6045) at ../assembler/assembler/ARMAssembler.h:1020
#2  0x002f6772 in JSC::ARMAssembler::relinkCall (from=0x4009a188, to=0x2f6045) at ../assembler/assembler/ARMAssembler.h:1139
#3  0x002f687c in JSC::MacroAssemblerARM::repatchCall (call=..., destination=...) at ../assembler/assembler/MacroAssemblerARM.h:1523
#4  0x002f6ec8 in JSC::RepatchBuffer::relink (this=0xbe8ad664, call=..., destination=...) at ../assembler/assembler/RepatchBuffer.h:95
#5  0x002f6f9e in js::mjit::ic::Repatcher::relink (this=0xbe8ad664, call=..., stub=...) at ../methodjit/ICRepatcher.h:90
#6  0x002f3c78 in PatchSetFallback (f=..., ic=0x456800) at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/MonoIC.cpp:150
#7  0x002f46ce in UpdateSetGlobalName (f=..., ic=0x456800, obj=0x40b00040, shape=0x40b0efb0) at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/MonoIC.cpp:280
#8  0x002f4886 in js::mjit::ic::SetGlobalName (f=..., ic=0x456800) at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/MonoIC.cpp:329
#9  0x00281352 in JaegerStubVeneer () at /home/decoder/LangFuzz/mozilla-central/js/src/methodjit/MethodJIT.cpp:164
#10 0x4009a194 in ?? ()
Cannot access memory at address 0x0
#11 0x4009a194 in ?? ()
Cannot access memory at address 0x0
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Given that this is in the guts of the assembler I'm guessing at sg:critical here, but if that turns out to not be the case we can re-label this one.
Whiteboard: js-triage-needed → [sg:critical] js-triage-needed
Marty, can you take a look at this--see if it is in fact exploitable? It looks to me like it may not be, but it seems like a codegen bug at least.
Assignee: general → mrosenberg
Not horribly unexpected, the issue is with pools.  More specifically, with pools and the repatcher.

For repatchable stub calls (well all calls, but specifically, repatchable calls) we generate the sequence
ldr r12, [pc, pool_dest]
ldr r8, [pc, pool_jaeger_stub_veneer]
blx r8
.word function_to_call
.word jaeger_stub_veneer

which will load the address of a function out of pool_dest, into register 12, then load the address of the jaeger stub veneer into r8, and call the jaeger stub veneer. The repatcher gets the address of |ldr r8, ...|, then looks back at the previous instruction, decodes enough of the instruction to find the address of pool_dest, and finally writes over the |.word function_to_call| with the new value.

In this case, however, immediately after emitting |ldr r12, [pc, pool_dest]|, we determined the pool needed to be flushed, and flushed the pool, thus the sequence we actually got was:
ldr r12, [pc, pool_dest]
b after_pool
.word function_to_call
ldr r8, [pc, pool_jaeger_stub_veneer]
blx r8
.word jaeger_stub_veneer

so when the instruction that the re-patcher gets is still |ldr r8, [pc,pool_jaeger_stub_veneer]|. However, the "instruction" before it is now .word function_to_call rather than |ldr r12, [pc, pool_dest]|.  This is why the assertion is thrown.  The result of calling the patcher on this is: we read the bottom 12 bits of the function that we're attempting to patch from, add it to the address of the patch (two before the actuall call to jaeger_stub_veneer), get a memory location, and write over that memory location with a single word of data that any possible attackers most likely won't get to control (We are writing over the value with the address of a C function).  More likely than not, the address that we write over will *not* be word aligned, whereas any instructions (or other data) that we generate will likely be word aligned, so it is likely going to write over parts of two different values.  I would say that this is in fact exploitable, but would be hard pressed to generate a reasonable exploit.
I can think of two possible solutions:
1) rather than storing the address of the second instruction, store the address of the *first* instruction, so we'll be able to walk forwards and get to the other two, possibly following pool guards.
2) mark those three instructions as a no-pool zone.  no-pool zones have always felt like a hack.
I'll need to find out why we carry the address of the second instruction to begin with.
Have not pushed to try, nor run jit-tests locally yet.
Attachment #575610 - Flags: review?(dmandelin)
Attachment #575610 - Flags: review?(dmandelin) → review+
Sorry for the slow update, I thought the fix for this landed on m-i on monday, but evidently the push failed, and I didn't notice?
(In reply to Marty Rosenberg [:Marty] from comment #6)
> Sorry for the slow update, I thought the fix for this landed on m-i on
> monday, but evidently the push failed, and I didn't notice?

No problem, thanks for double-checking.
Closed: 8 years ago
Resolution: --- → FIXED
If this is a major security concern for FF9/10 and is considered low risk, please nominate for approval on beta/aurora.
We should get this fix into Fx10, don't feel too strongly about rushing it into 9.
Summary: Assertion failure: (inst & mask) == expected, at ../methodjit/ICChecker.h:56 → [ARM] Assertion failure: (inst & mask) == expected, at ../methodjit/ICChecker.h:56
Target Milestone: --- → mozilla11
Marty, is this patch appropriate for aurora? If so, please request aurora approval on the patch!
Marty, ping?
Comment on attachment 575610 [details] [diff] [review]
Don't place pools in the middle of a call

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
This bug has been in the assembler since arm support for JM landed
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
It has been pretty thoroughly tested on aurora, and the fix is pretty foolproof
Attachment #575610 - Flags: approval-mozilla-beta?
Attachment #575610 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: core-security
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.