973874 - Crash [@ js::jit::Simulator::instructionDecode] with poisoned pointer (0xdeadbeef)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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();

Comment 1

[Christian Holler (:decoder)]

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.

Comment 2

[Jan de Mooij [:jandem]]

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?

Comment 3

[Douglas Crosher [:dougc]]

(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.

Comment 4

[Douglas Crosher [:dougc]]

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.

Comment 5

[Jan de Mooij [:jandem]]

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

[Nicolas B. Pierron [:nbp]]

(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.

Comment 7

[Christian Holler (:decoder)]

Is the 0xdeadbeef stuff also used in optimized builds? I thought that kind of poisoning is only done in debug builds.

Comment 8

[Jan de Mooij [:jandem]]

(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

[Nicolas B. Pierron [:nbp]]

(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

[Jan de Mooij [:jandem]]

(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

[Douglas Crosher [:dougc]]

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

[Douglas Crosher [:dougc]]

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.

Comment 13

[Marty Rosenberg [:mjrosenb]]

Attachment: 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.

Comment 14

[Douglas Crosher [:dougc]]

(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?

Comment 15

[Marty Rosenberg [:mjrosenb]]

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.

Comment 16

[Douglas Crosher [:dougc]]

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.

Comment 17

[Andrew McCreight [:mccr8]]

Is this ready for sec-approval and landing, Marty?  Thanks.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21879e298728

Looks like this landed without sec-approval. Hope it doesn't affect too much else.

Comment 19

[Marty Rosenberg [:mjrosenb]]

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.

Comment 20

[Andrew McCreight [:mccr8]]

Doesn't comment 3 say it is reproducible on hardware?

Comment 21

[Al Billings [:abillings - ex-MoCo]]

(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

[Andrew McCreight [:mccr8]]

This is an ARM bug, and I think we don't ship ESR on ARM.

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Deja Vu. We had this discussion in another bug and I thought one of our downstream partners (Redhat?) did.

Comment 24

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 25

[Andrew McCreight [:mccr8]]

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.

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks Andrew. Marking as esr24-.

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi decoder - can you verify this? Thanks.

Comment 28

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 29

[Christian Holler (:decoder)]

Verified by the bot :)

Comment 30

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/7fba42f5f555