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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


try {
this.watch("b", "".substring);
} catch(exc1) {}
eval("\
var URI = '';\
test();\
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\"/>';\
}\
test();\
");


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

[Johnny Stenback (:jst)]

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

[David Mandelin [:dmandelin]]

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

[Marty Rosenberg [:mjrosenb]]

oy.
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
...
pool:
...
pool_dest:
.word function_to_call
pool_jaeger_stub_veneer
.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
pool1:
...
pool_dest:
.word function_to_call
after_pool
ldr r8, [pc, pool_jaeger_stub_veneer]
blx r8
...
pool2
pool_jaeger_stub_veneer
.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

[Marty Rosenberg [:mjrosenb]]

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

[Marty Rosenberg [:mjrosenb]]

Attachment: Don't place pools in the middle of a call

Have not pushed to try, nor run jit-tests locally yet.

Comment 6

[Marty Rosenberg [:mjrosenb]]

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?
http://hg.mozilla.org/integration/mozilla-inbound/rev/6dc5acdc2cbe

Comment 7

[David Mandelin [:dmandelin]]

(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?
> http://hg.mozilla.org/integration/mozilla-inbound/rev/6dc5acdc2cbe

No problem, thanks for double-checking.

Comment 8

[David Mandelin [:dmandelin]]

https://hg.mozilla.org/mozilla-central/rev/6dc5acdc2cbe

Comment 9

[Alex Keybl [:akeybl]]

If this is a major security concern for FF9/10 and is considered low risk, please nominate for approval on beta/aurora.

Comment 10

[Daniel Veditz [:dveditz]]

We should get this fix into Fx10, don't feel too strongly about rushing it into 9.

Comment 11

[Johnny Stenback (:jst)]

Marty, is this patch appropriate for aurora? If so, please request aurora approval on the patch!

Comment 12

[Johnny Stenback (:jst)]

Marty, ping?

Comment 13

[Marty Rosenberg [:mjrosenb]]

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

[Marty Rosenberg [:mjrosenb]]

Pushed to beta with: https://hg.mozilla.org/releases/mozilla-beta/rev/2f0d90c7f5c7

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/053bb31882c4