Closed
Bug 973874
Opened 11 years ago
Closed 11 years ago
Crash [@ js::jit::Simulator::instructionDecode] with poisoned pointer (0xdeadbeef)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla30
Tracking | Status | |
---|---|---|
firefox30 | --- | verified |
firefox-esr24 | - | unaffected |
seamonkey2.26 | --- | fixed |
People
(Reporter: decoder, Assigned: mjrosenb)
References
Details
(Keywords: crash, sec-high, testcase, Whiteboard: [adv-main30+])
Crash Data
Attachments
(1 file)
2.90 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 339f0d450d46 (x86 ARM simulator build, run with --fuzzing-safe --ion-eager --ion-check-range-analysis):
function test() {
eval("var M4x4 = {};\
M4x4.mul = function M4x4_mul(a, r) {\
a11 = a[0];\
a21 = a[1];\
a23 = a[9];\
a33 = M4x4_mul[10];\
return r;\
};\
M4x4.makeLookAt = function() {\
tm1 = Float32Array(16);\
tm2 = Float32Array(16);\
r = new Float32Array(16);\
return M4x4.mul(tm1, tm2, r);\
};\
for(j = 0; j < 3; j++)\
M4x4.mul(M4x4.makeLookAt(), M4x4.makeLookAt());\
");
} test();
Reporter | ||
Comment 1•11 years ago
|
||
Backtrace:
Program received signal SIGSEGV, Segmentation fault.
js::jit::Simulator::instructionDecode (this=0x948a068, instr=0xdeadbeef) at js/src/jit/arm/Simulator-arm.cpp:3980
3980 if (instr->conditionField() == kSpecialCondition) {
(gdb) bt 32
#0 js::jit::Simulator::instructionDecode (this=0x948a068, instr=0xdeadbeef) at js/src/jit/arm/Simulator-arm.cpp:3980
#1 0x083bd2a8 in execute<false> (this=0x948a068) at js/src/jit/arm/Simulator-arm.cpp:4036
#2 js::jit::Simulator::callInternal (this=0x948a068, entry=0xf78bc720 "\361O-\351\020\213-\355\r\200\240\341h\220\235\345\r\260\240\341t\240\235", <incomplete sequence \345>)
at js/src/jit/arm/Simulator-arm.cpp:4118
#3 0x083bd51c in js::jit::Simulator::call (this=0x948a068, entry=0xf78bc720 "\361O-\351\020\213-\355\r\200\240\341h\220\235\345\r\260\240\341t\240\235", <incomplete sequence \345>, argument_count=8)
at js/src/jit/arm/Simulator-arm.cpp:4198
#4 0x081902a2 in EnterBaseline (cx=0x948a9e8, data=...) at js/src/jit/BaselineJIT.cpp:122
#5 0x081cd3da in js::jit::EnterBaselineMethod (cx=0x948a9e8, state=...) at js/src/jit/BaselineJIT.cpp:154
#6 0x08565fb5 in js::RunScript (cx=0x948a9e8, state=...) at js/src/vm/Interpreter.cpp:412
#7 0x085661b2 in RunScript (state=..., cx=0x948a9e8) at js/src/vm/Interpreter.cpp:390
#8 js::ExecuteKernel (cx=0x948a9e8, script=0xf75331a8, scopeChainArg=(JSObject &) @0xf753b790 [object Call] delegate, thisv=..., type=js::EXECUTE_DIRECT_EVAL, evalInFrame=..., result=0xf7acbd88)
at js/src/vm/Interpreter.cpp:631
#9 0x08118394 in EvalKernel (cx=0x948a9e8, args=..., evalType=DIRECT_EVAL, caller=..., scopeobj=(JSObject * const) 0xf753b790 [object Call] delegate, pc=0x9518f3b "{")
at js/src/builtin/Eval.cpp:336
#10 0x081189e7 in js::DirectEval (cx=0x948a9e8, args=...) at js/src/builtin/Eval.cpp:453
#11 0x0820eaf0 in js::jit::DoCallFallback (cx=0x948a9e8, frame=0xf7acbdd0, stub=0x9519ef8, argc=1, vp=0xf7acbd88, res=$jsval(-nan(0xfff8200000000))) at js/src/jit/BaselineIC.cpp:8098
#12 0x083bca8b in js::jit::Simulator::softwareInterrupt (this=0x948a068, instr=0x9519674) at js/src/jit/arm/Simulator-arm.cpp:2155
#13 0x083b9b95 in decodeType7 (instr=0x9519674, this=0x948a068) at js/src/jit/arm/Simulator-arm.cpp:3147
#14 js::jit::Simulator::instructionDecode (this=0x948a068, instr=0x9519674) at js/src/jit/arm/Simulator-arm.cpp:4004
#15 0x083bd2a8 in execute<false> (this=0x948a068) at js/src/jit/arm/Simulator-arm.cpp:4036
#16 js::jit::Simulator::callInternal (this=0x948a068, entry=0xf78bc698 "\361O-\351\020\213-\355\r\200\240\341h\220\235\345t\240\235", <incomplete sequence \345>)
at js/src/jit/arm/Simulator-arm.cpp:4118
#17 0x083bd51c in js::jit::Simulator::call (this=0x948a068, entry=0xf78bc698 "\361O-\351\020\213-\355\r\200\240\341h\220\235\345t\240\235", <incomplete sequence \345>, argument_count=8)
at js/src/jit/arm/Simulator-arm.cpp:4198
#18 0x082a4e39 in EnterIon (data=..., cx=0x948a9e8) at js/src/jit/Ion.cpp:2235
[...]
(gdb) x /i $pc
=> 0x83b975d <js::jit::Simulator::instructionDecode(js::jit::SimInstruction*)+45>: mov (%edi),%edx
(gdb) info reg edi edx
edi 0xdeadbeef -559038737
edx 0xdeadbeef -559038737
Marked s-s because this involves a poisoned pointer. If this is a bug in the simulator only, feel free to remove the s-s.
status-firefox30:
--- → affected
Comment 2•11 years ago
|
||
I can reproduce this on OS X. If you use a threadsafe build, you need --ion-parallel-compile=off as well.
Marty, Doug: can one of you take a look? Are we executing a pool instruction?
Flags: needinfo?(mrosenberg)
Flags: needinfo?(dtc-moz)
Comment 3•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2)
> I can reproduce this on OS X. If you use a threadsafe build, you need
> --ion-parallel-compile=off as well.
>
> Marty, Doug: can one of you take a look? Are we executing a pool instruction?
This is reproducible on hardware so may not be a problem with the simulator.
The simulator gives the following trace leading up to the crash.
Baseline code:
0xf76d540c ldr r7, [r4, #72]
0xf76d5410 vldr s0, [r7, #36]
0xf76d5414 vcmp.f32 s0, s0
0xf76d5418 vmrs APSR_nzcv, fpscr
0xf76d541c bvc #0
0xf76d5420 vldr s0, [pc, #32]
0xf76d5424 vcvt.f64.f32 d0, s0
0xf76d5428 b #9176 < branch to ioncache code at 0xf76d7808
Ioncache code:
0xf76d7808 ldr lr, [r2]
0xf76d780c movw r12, #23016
0xf76d7810 movt r12, #62980
0xf76d7814 cmp lr, r12
0xf76d7818 bne #32
0xf76d781c ldr lr, [r2, #4]
0xf76d7820 movw r12, #45472
0xf76d7824 movt r12, #62978
0xf76d7828 cmp lr, r12
0xf76d782c bne #12
0xf76d7830 ldr r2, [r2, #8]
0xf76d7834 add r12, r2, #2048
0xf76d7838 vstr d0, [r12, #16]
0xf76d783c b #-9164 < branch back to baseline code at 0xf76d5478
Baseline code:
0xf76d5478 ldr pc, [pc, #-20] < crash !!!
0xf76d547c str r0, [sp, #-4]!
0xf76d5480 movw r0, #40960
0xf76d5484 movt r0, #63257
0xf76d5488 ldr r12, [r0, #128]
0xf76d548c cmp r12, #0
0xf76d5490 bne #72
0xf76d5494 push {r0, r1, r2, r3}
0xf76d5498 vpush {d0, d1, d2, d3, d4, d5, d6, d7}
Memory dump around [pc, #-20]:
0xf76d5450: 0xdeadbeef -559038737
0xf76d5454: 0xdeadbeef -559038737
0xf76d5458: 0xf76dc314 -143801580
0xf76d545c: 0xdeadbeef -559038737
0xf76d5460: 0xdeadbeef -559038737
0xf76d5464: 0xf76dc318 -143801576
0xf76d5468: 0xdeadbeef -559038737
0xf76d546c: 0xdeadbeef -559038737
0xf76d5470: 0xdeadbeef -559038737
0xf76d5474: 0xdeadbeef -559038737
Shall try to narrow down the source of the 'ldr' instruction at 0xf76d5478 to check if it is emitted as intended.
Flags: needinfo?(dtc-moz)
Comment 4•11 years ago
|
||
Looks like a problem with the constant pools. The 'ldr' instruction is emitted by patchConstantPoolLoad when finishing the pools. The instruction seems to be encoded correctly and the problem seems to in the larger constant pools code.
Depends on: 760642
Comment 5•11 years ago
|
||
While bug 760642 will probably fix this, that bug is not ready yet and won't be backported. Can we fix the old code in the meantime?
Executing data in the constant pool as code seems scary. An attacker can probably control what's in there and execute arbitrary code, right?
Comment 6•11 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5)
> While bug 760642 will probably fix this, that bug is not ready yet and won't
> be backported. Can we fix the old code in the meantime?
>
> Executing data in the constant pool as code seems scary. An attacker can
> probably control what's in there and execute arbitrary code, right?
0xdeadbeef is not aligned, I don't know if this matters for ARM cpus, but I would assume this would cause a SIGBUS. Still it sounds safer if all these constants could be made to default to the first page, such as 0x00000bad to crash instead badly of potentially executing shell-code.
Reporter | ||
Comment 7•11 years ago
|
||
Is the 0xdeadbeef stuff also used in optimized builds? I thought that kind of poisoning is only done in debug builds.
Comment 8•11 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> 0xdeadbeef is not aligned, I don't know if this matters for ARM cpus, but I
> would assume this would cause a SIGBUS. Still it sounds safer if all these
> constants could be made to default to the first page, such as 0x00000bad to
> crash instead badly of potentially executing shell-code.
What if we can jump to some int32-constant stored in the middle of the pool? Or can we only jump to 0xdeadbeef?
Comment 9•11 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #7)
> Is the 0xdeadbeef stuff also used in optimized builds? I thought that kind
> of poisoning is only done in debug builds.
This one is a dummy placeholder waiting to be patched. So this one is generated all the time as we do not have the data at the time of the code generation.
(In reply to Jan de Mooij [:jandem] from comment #8)
> (In reply to Nicolas B. Pierron [:nbp] from comment #6)
> > 0xdeadbeef is not aligned, I don't know if this matters for ARM cpus, but I
> > would assume this would cause a SIGBUS. Still it sounds safer if all these
> > constants could be made to default to the first page, such as 0x00000bad to
> > crash instead badly of potentially executing shell-code.
>
> What if we can jump to some int32-constant stored in the middle of the pool?
> Or can we only jump to 0xdeadbeef?
We set the PC (ldr pc, …) based on a relative-pc read (…, [pc, #-0x20]). This is how we are supposed to use the constant pool, by reading addresses or constant out of it.
If we generate code correctly we should not be able to execute anything in the constant pool. In addition, these constant pools are placed after unconditional jumps. So, unless you have a jump into the constant pool, there is no way you are going to execute it.
Comment 10•11 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> If we generate code correctly we should not be able to execute anything in
> the constant pool.
I know, but we are *not* generating code correctly or we wouldn't be crashing. If we can emit a bogus "ldr pc, [pc, #-20]" instruction, we could probably also emit an ldr instruction that jumps to some more dangerous location inside or outside the constant pool.
Marty, can you take this bug?
Comment 11•11 years ago
|
||
fwiw the ldr instruction is generate via jumpWithPatch and this is called from RepatchIonCache::emitInitialJump.
The CodeLocationJump::repoint looks plausible and points to the instruction and into the constant pool at the location loaded by the ldr instruction.
However I am not seeing a call to PatchJump to actually fill the entry in the constant pool before the ldr instruction is reached?
Sorry, it appears possible that it is not a constant pool issue.
Comment 12•11 years ago
|
||
It would appear that jumpWithPatch is expected to encode an initial jump target, and it does appear to do this in most cases. Perhaps the constant pool entry is just not being initialized with the branch target and this causes this problem when the ldr encoding is used and PatchJump has not been called. Looking like a constant pool issue again.
Assignee | ||
Comment 13•11 years ago
|
||
This fixed a similar sounding bug that was reported on IRC. Haven't tested it on the testcase in this bug yet.
Attachment #8378764 -
Flags: review?(dtc-moz)
Flags: needinfo?(mrosenberg)
Comment 14•11 years ago
|
||
(In reply to Marty Rosenberg [:mjrosenb] from comment #13)
> Created attachment 8378764 [details] [diff] [review]
> fix_crazy_crash-r0.patch
>
> This fixed a similar sounding bug that was reported on IRC. Haven't tested
> it on the testcase in this bug yet.
Thank you. The bug does not occur with the changes to as_BranchPool applied.
However, the failing instruction has been replaced by a branch rather than a ldr, so I would need more time to review this to be sure it was not just avoiding the problematic case. An explanation would help?
Flags: needinfo?(mrosenberg)
Assignee | ||
Comment 15•11 years ago
|
||
Yeah. Assembler::as_BranchPool is supposed fill the repatch label with the offset of the branch/load instruction. This should always be the returned instruction, but since we initially got the offset with nextOffset() rather than just taking the address generated by insert_entry, when a pool was inserted immediately before this instruction, it resulted in the repatch label getting the address of the branch over the pool, rather than the load immediately after the pool. Then when we go to repatch it later, it is already a branch, so it looks legit enough, but it leaves the original load in place, along with the poisoned address.
Flags: needinfo?(mrosenberg)
Comment 16•11 years ago
|
||
Comment on attachment 8378764 [details] [diff] [review]
fix_crazy_crash-r0.patch
Review of attachment 8378764 [details] [diff] [review]:
-----------------------------------------------------------------
Thank you for the quick patch, and the explanation. It looks like part of the problem is that the jump was not being marked as a branch, and thus executableCopy was not calling patchBranch on the errant load. The patch does appear to work around this.
::: js/src/jit/arm/Assembler-arm.cpp
@@ +1703,5 @@
> BufferOffset
> Assembler::as_BranchPool(uint32_t value, RepatchLabel *label, ARMBuffer::PoolEntry *pe, Condition c)
> {
> PoolHintPun php;
> + //BufferOffset next = nextOffset();
Remove.
::: js/src/jit/arm/MacroAssembler-arm.cpp
@@ +4120,5 @@
> + static int count = 0;
> + BufferOffset bo = as_BranchPool(0xdeadbeef - ((count << 2) & 0xffff), label, &pe, cond);
> + if (((count << 2) & 0xffff) == 0)
> + fprintf(stderr, "rolling over, now 0x%x\n", count);
> + count++;
Remove this debug code.
::: js/src/jit/arm/Simulator-arm.cpp
@@ +3812,5 @@
> void
> Simulator::instructionDecode(SimInstruction *instr)
> {
> + if (getenv("DIS_EVERY_INST"))
> + DisassembleInstruction((uint32_t)instr);
Remove this debug code.
Attachment #8378764 -
Flags: review?(dtc-moz) → review+
Updated•11 years ago
|
Group: javascript-core-security
Comment 17•11 years ago
|
||
Is this ready for sec-approval and landing, Marty? Thanks.
Assignee: nobody → mrosenberg
Flags: needinfo?(mrosenberg)
Comment 18•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/21879e298728
Looks like this landed without sec-approval. Hope it doesn't affect too much else.
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 19•11 years ago
|
||
err, Oops, I was pinged to land it, and forgot to ask for sec-approval. We haven't seen this on real hardware, so the chances of it affecting real hardware seem low.
Flags: needinfo?(mrosenberg)
Comment 20•11 years ago
|
||
Doesn't comment 3 say it is reproducible on hardware?
Updated•11 years ago
|
Group: javascript-core-security
Comment 21•11 years ago
|
||
(In reply to Marty Rosenberg [:mjrosenb] from comment #19)
> err, Oops, I was pinged to land it, and forgot to ask for sec-approval. We
> haven't seen this on real hardware, so the chances of it affecting real
> hardware seem low.
So we don't know how far back this goes? Do we need to port this anywhere, like ESR?
Comment 22•11 years ago
|
||
This is an ARM bug, and I think we don't ship ESR on ARM.
Comment 23•11 years ago
|
||
Deja Vu. We had this discussion in another bug and I thought one of our downstream partners (Redhat?) did.
Comment 24•11 years ago
|
||
Comment 2 says that this is reproducible on OSX, although I'm not clear if that means a desktop or B2G desktop build. Andrew/Jan, can you confirm that this is ARM specific and that this doesn't impact desktop users?
Flags: needinfo?(jdemooij)
Flags: needinfo?(continuation)
Comment 25•11 years ago
|
||
Yeah, the patch only touches ARM code, so I think it is ARM only. This is reproducible on OSX in the ARM simulator, which should not affect desktop users.
Flags: needinfo?(jdemooij)
Flags: needinfo?(continuation)
Comment 26•11 years ago
|
||
Thanks Andrew. Marking as esr24-.
status-firefox-esr24:
--- → unaffected
tracking-firefox-esr24:
--- → -
Updated•11 years ago
|
Whiteboard: [adv-main30+]
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 28•11 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 30•10 years ago
|
||
status-seamonkey2.26:
--- → fixed
Updated•10 years ago
|
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•