Closed Bug 1392105 Opened 3 years ago Closed 3 years ago

Crash [@ js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst] or Assertion failure: data >> 28 != 0xf (The instruction does not have condition code), at jit/arm/Assembler-arm.h:1996

Categories

(Core :: JavaScript Engine, defect, critical)

ARM
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

Details

(6 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main56+])

Crash Data

Attachments

(2 files, 2 obsolete files)

The following testcase crashes on mozilla-central revision 5ca5691372cb (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --without-intl-api --enable-optimize --target=i686-pc-linux-gnu --enable-simulator=arm, run with --fuzzing-safe --arm-asm-nop-fill=1 --ion-offthread-compile=off):

var code = "(module ";
for (var i = 0; i < 100; i++)
  code += "(func (param i32) (result i32) (i32.add (i32.const 1) (get_local 0))) ";
code += ")";
var buf = wasmTextToBinary(code);
WebAssembly.compile(buf);



Backtrace:

 received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0xf6d65b40 (LWP 26003)]
js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xf7988af4, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:403
#0  js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst (this=0xf7988af4, off=...) at js/src/jit/shared/IonAssemblerBuffer.h:403
#1  0x083974c8 in js::jit::Assembler::editSrc (bo=..., this=0xf79889f0) at js/src/jit/arm/Assembler-arm.h:1244
#2  js::jit::Assembler::bind (this=0xf79889f0, label=0xf6d64874) at js/src/jit/arm/Assembler-arm.cpp:2880
#3  0x0830f4c9 in js::jit::MacroAssembler::wasmEmitTrapOutOfLineCode (this=0xf79889f0) at js/src/jit/MacroAssembler.cpp:3036
#4  0x0868d51c in js::wasm::BaseCompiler::endFunction (this=0xf6d649f8) at js/src/wasm/WasmBaselineCompile.cpp:2388
#5  js::wasm::BaseCompiler::emitFunction (this=0xf6d649f8) at js/src/wasm/WasmBaselineCompile.cpp:7401
#6  0x0868db4b in js::wasm::BaselineCompileFunction (task=0xf79889b4, unit=0xf516724c, error=0xf6d64e88) at js/src/wasm/WasmBaselineCompile.cpp:7595
#7  0x086c1522 in js::wasm::CompileFunction (task=0xf79889b4, error=0xf6d64e88) at js/src/wasm/WasmGenerator.cpp:1340
#8  0x08583b1b in js::HelperThread::handleWasmWorkload (this=0xf795d1a4, locked=..., mode=js::wasm::CompileMode::Tier1) at js/src/vm/HelperThreads.cpp:1843
#9  0x085849c0 in js::HelperThread::threadLoop (this=0xf795d1a4) at js/src/vm/HelperThreads.cpp:2362
#10 0x08586ef9 in js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::callMain<0u> (this=0xf790b060) at js/src/threading/Thread.h:234
#11 js::detail::ThreadTrampoline<void (&)(void*), js::HelperThread*>::Start (aPack=0xf790b060) at js/src/threading/Thread.h:227
#12 0xf7fb228a in start_thread (arg=0xf6d65b40) at pthread_create.c:333
#13 0xf7cd94ce in clone () from /lib32/libc.so.6
eax	0x17b4	6068
ebx	0xf7988af4	-140997900
ecx	0xe340c000	-482295808
edx	0x1800	6144
esi	0x17b4	6068
edi	0x4c	76
ebp	0x17b4	6068
esp	0xf6d647e4	4141238244
eip	0x83a13ba <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+170>
=> 0x83a13ba <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+170>:	sub    0x8(%ecx),%edx
   0x83a13bd <js::jit::AssemblerBuffer<1024, js::jit::Instruction>::getInst(js::jit::BufferOffset)+173>:	cmp    %edx,%eax


This could be a security problem due to the crash address, marking s-s.
Flags: needinfo?(bbouvier)
This is pretty bad:

- when calling beginFunction(), we call masm.add32ToPtrWithPatch(), which in theory inserts the following: MOVW, MOVT, ADD.
- but we fill with nops here, so we insert (with NOP := jump to next inst): NOP, MOVW, NOP, MOVT, NOP, ADD.
- between the first NOP and the MOVW, a new Slice is allocated because the previous one (that ended with the NOP) is full. There's 4 bytes of padding between the previous slice and the one we just allocated.
- in endFunction(), the patchAdd32ToPtr() assumes the buffer is linear and not split in Slices. The instruction pointer implied from the offset is pointing in the padding between the two slices (!). This padding is converted into the MOVW value.
- then we try to go to the next instruction with Instruction::next(), which uses (this + 1) as the address of the next instruction (!). Again, in a linear assembly buffer without slices, this would work great; but here, it's reading into the Slice header (in particular, it tries to read BufferSlice::prev_), and it appears to have the condition bit unset, if misinterpreted as an ARM instruction.
Flags: needinfo?(bbouvier)
Note this is not exclusively related to wasm: all the uses of Instruction::next() are potentially wrong if they're done on a masm that hasn't been copied out with executableCopy. This could happen with every use of next(), which are hard to evaluate, since we don't have searchfox for ARM code. There are many uses in Assembler-arm.cpp at least. (I'll try renaming next() to see how many compiler errors I get)

So one solution is just to do all the patching after masm::executableCopy has been called (since the destination of executableCopy has to be a linear buffer).

Another would be to do a binary search over the slices in Instruction::next(), to find the Slice that contains the current instruction, and make sure we (maybe) pass over to the next slice before inferring the address of the next instruction. This might have a significant runtime cost, though...

Another solution would be to get rid of Slices entirely, and just use a linear buffer in IonAssemblyBuffer that gets expanded on capacity overflow. I assume the slicing was done because our ARM impl is 32 bits and it'd be easy to OOM by using exponential growth on the assembly buffer.

Another solution would be that all the instructions that can be patched send their actual code offsets for each sub-instruction that's macro-expanded by the masm. This would be a bit invasive though.
I feel like we do a lot of patching and probably we're just missing something from add32ToPtrWithPatch() (which was added just for wasm baseline) that (1) forbids pools/nops, (2) ensures we have at least 3 more instructions in the current pool.  That might be AUtoForbidPools which takes a number of instructions.  It'd be useful to look at how some of the other *WithPatch() masm functions work.
(In reply to Luke Wagner [:luke] from comment #3)
> I feel like we do a lot of patching and probably we're just missing
> something from add32ToPtrWithPatch() (which was added just for wasm
> baseline) that (1) forbids pools/nops, (2) ensures we have at least 3 more
> instructions in the current pool.  That might be AUtoForbidPools which takes
> a number of instructions.  It'd be useful to look at how some of the other
> *WithPatch() masm functions work.

Note the issue is not about the constant pools, which are a different concept from the instruction slices. In fact, there's a masm.flush() call just before the patchAdd32ToPtr() call, to ensure constant pools are empty. I *think* this is a different issue.
Agreed, fixing ma_mov_patch to use the correct instruction iterator seems like the right fix.
See Also: → 1393011
Attached patch fix.patch (obsolete) — Splinter Review
This fixes it and paves the way for the other bug mentioned in "see also".

Note BufferInstructionIterator::skipPool and the variant of InstIsGuard that takes a BufferInstructionIterator have been copied from the existing Instruction::skipPool and InstIsGuard (for Instruction).
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8900276 - Flags: review?(luke)
How far back does this issue go?
Flags: needinfo?(bbouvier)
Comment on attachment 8900276 [details] [diff] [review]
fix.patch

Review of attachment 8900276 [details] [diff] [review]:
-----------------------------------------------------------------

Ideally, I think we'd try to make a more syntactic distinction between the now-known-dangerous InstructionIterator and next() but as a minimal patch to fix the bug, lgtm.

::: js/src/jit/arm/MacroAssembler-arm.h
@@ +130,2 @@
>      static void ma_mov_patch(Imm32 imm, Register dest, Assembler::Condition c,
> +                             RelocStyle rs, Iter& iter);

How about taking the iterator by value?  Client code doesn't seem to get much mileage from being able to reuse an existing iterator (and if it did, we'd want to take an Iter* so it was clear at the callsite that the argument was being mutated).  This would simplify several callers by allowing them to not have to make a separate copy.
Attachment #8900276 - Flags: review?(luke) → review+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> How far back does this issue go?

This precise instance is from bug 1319388. In general, it's hard to tell without a full audit (bug 1393011) if there aren't any other instances of this bug, which could predate from much longer.
Flags: needinfo?(bbouvier)
Track 56+/57+ as regression.
Attached patch fix.patch (obsolete) — Splinter Review
Updated patch with Luke's recommendation and a small fix (iterCopy in TraceDataRelocation has to be placed at the top of the function's body to keep track of the instruction at the initial position, not the updated one).
Attachment #8900276 - Attachment is obsolete: true
Attachment #8900685 - Flags: review+
Comment on attachment 8900685 [details] [diff] [review]
fix.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's hard to tell (see comment 2). In the absolute worst case, somebody could write arbitrary instructions at a code location and have them executed, assuming they perfectly know the code + data layout etc.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test case makes it trivial to reproduce the crash. Then an investigation can lead to the same conclusions. (We could of course land the test case separately)

Which older supported branches are affected by this flaw?
see tracking flags

If not all supported branches, which bug introduced the flaw?
bug 1319388

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
They wouldn't be too hard to make. A bit risky because it involves a small refactoring, but only a few locations were changed.

How likely is this patch to cause regressions; how much testing does it need?
Regressions would be inevitable to have the correct behavior. Local testing has been done on all the JIT tests, and they all pass.
Attachment #8900685 - Flags: sec-approval?
Keywords: sec-high
This can't get sec-approval as is. We need the fix and the test to be separate patches. Then I'll approve the patch. AFTER we ship a release with the fix, THEN you can check in the patch (like a month after). Otherwise, your patch will 0day us because it has a test to demonstrate the security problem.
Flags: needinfo?(bbouvier)
Attachment #8900685 - Flags: sec-approval?
Attached patch test.patchSplinter Review
Flags: needinfo?(bbouvier)
Attachment #8901079 - Flags: review+
Attached patch fix.patchSplinter Review
[Security approval request comment] see comment 12, plus updated answer:

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No (analyzing the code makes it clear, though).
Attachment #8901080 - Flags: sec-approval?
Attachment #8901080 - Flags: review+
Attachment #8900685 - Attachment is obsolete: true
Comment on attachment 8901080 [details] [diff] [review]
fix.patch

Sec-approval+ for trunk.
We need a Beta nominated patch to land after trunk. 
Does this affect ESR52?
Attachment #8901080 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8190fc1ba510c50995b7c90977dc1e120cf12ced

Luke, can you nominate this for Beta approval since Benjamin is away? It grafts cleanly, so no rebasing needed :)
Flags: needinfo?(luke)
Since this bug was found by our own internal fuzzing and it seems unlikely to be causing crashes in the wild and since the patch isn't completely trivial and so carries some non-zero risk, I would suggest just letting the fix ride the trains to get maximal testing.
Flags: needinfo?(luke)
I'd prefer this to go onto the Beta. Our checkin to nightly has exposed the security hole to the public to anyone watching.
Comment on attachment 8901080 [details] [diff] [review]
fix.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1319388
[User impact if declined]: possible crashes
[Is this code covered by automated tests?]: yes, but not checked in
[Has the fix been verified in Nightly?]: test case requires shell, so verified in shell
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: slightly, according to comment 12
[Why is the change risky/not risky?]: slightly refactoring
Attachment #8901080 - Flags: approval-mozilla-beta?
Attachment #8901080 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/8190fc1ba510
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Group: javascript-core-security → core-security-release
Depends on: 1395100
(In reply to Luke Wagner [:luke] from comment #20)
> [Is this code covered by automated tests?]: yes, but not checked in
> [Has the fix been verified in Nightly?]: test case requires shell, so
> verified in shell
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Luke's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main56+]
Is it a good time to check in the test?
Flags: needinfo?(abillings)
(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> Is it a good time to check in the test?

Yes, this should be fine.
Flags: needinfo?(abillings)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.