Last Comment Bug 696748 - [ARM] Assertion failure: (inst & mask) == expected, at ../methodjit/ICChecker.h:56
: [ARM] Assertion failure: (inst & mask) == expected, at ../methodjit/ICChecker...
[sg:critical] js-triage-needed
: assertion, crash, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: ARM Linux
-- critical (vote)
: mozilla11
Assigned To: Marty Rosenberg [:mjrosenb]
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2011-10-24 06:20 PDT by Christian Holler (:decoder)
Modified: 2013-03-20 04:50 PDT (History)
10 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Don't place pools in the middle of a call (2.05 KB, patch)
2011-11-18 17:53 PST, Marty Rosenberg [:mjrosenb]
dmandelin: review+
curtisk: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2011-10-24 06:20:30 PDT
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?)
Comment 1 User image Johnny Stenback (:jst, 2011-11-03 13:53:53 PDT
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.
Comment 2 User image David Mandelin [:dmandelin] 2011-11-16 19:03:25 PST
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.
Comment 3 User image Marty Rosenberg [:mjrosenb] 2011-11-17 18:38:12 PST
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.
Comment 4 User image Marty Rosenberg [:mjrosenb] 2011-11-17 18:41:40 PST
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.
Comment 5 User image Marty Rosenberg [:mjrosenb] 2011-11-18 17:53:22 PST
Created attachment 575610 [details] [diff] [review]
Don't place pools in the middle of a call

Have not pushed to try, nor run jit-tests locally yet.
Comment 6 User image Marty Rosenberg [:mjrosenb] 2011-11-30 18:05:23 PST
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?
Comment 7 User image David Mandelin [:dmandelin] 2011-11-30 18:10:49 PST
(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.
Comment 8 User image David Mandelin [:dmandelin] 2011-12-01 10:41:28 PST
Comment 9 User image Alex Keybl [:akeybl] 2011-12-05 19:46:47 PST
If this is a major security concern for FF9/10 and is considered low risk, please nominate for approval on beta/aurora.
Comment 10 User image Daniel Veditz [:dveditz] 2011-12-08 13:45:58 PST
We should get this fix into Fx10, don't feel too strongly about rushing it into 9.
Comment 11 User image Johnny Stenback (:jst, 2011-12-08 13:46:52 PST
Marty, is this patch appropriate for aurora? If so, please request aurora approval on the patch!
Comment 12 User image Johnny Stenback (:jst, 2012-01-05 13:29:12 PST
Marty, ping?
Comment 13 User image Marty Rosenberg [:mjrosenb] 2012-01-05 13:58:53 PST
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
Comment 14 User image Marty Rosenberg [:mjrosenb] 2012-01-12 16:03:47 PST
Pushed to beta with:
Comment 15 User image Ed Morley [:emorley] 2013-03-20 04:49:24 PDT

Note You need to log in before you can comment on or make changes to this bug.