Closed
Bug 1392105
Opened 7 years ago
Closed 7 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)
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: decoder, Assigned: bbouvier)
References
Details
(6 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main56+])
Crash Data
Attachments
(2 files, 2 obsolete files)
724 bytes,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
14.25 KB,
patch
|
bbouvier
:
review+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bbouvier)
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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.
Comment 3•7 years ago
|
||
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.
Assignee | ||
Comment 4•7 years ago
|
||
(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.
Comment 5•7 years ago
|
||
Agreed, fixing ma_mov_patch to use the correct instruction iterator seems like the right fix.
Assignee | ||
Comment 6•7 years ago
|
||
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).
Comment 8•7 years ago
|
||
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+
Assignee | ||
Comment 9•7 years ago
|
||
(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)
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox-esr52:
--- → ?
tracking-firefox56:
--- → ?
tracking-firefox57:
--- → ?
Assignee | ||
Comment 11•7 years ago
|
||
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+
Assignee | ||
Comment 12•7 years ago
|
||
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?
Comment 13•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8900685 -
Flags: sec-approval?
Assignee | ||
Comment 14•7 years ago
|
||
Flags: needinfo?(bbouvier)
Attachment #8901079 -
Flags: review+
Assignee | ||
Comment 15•7 years ago
|
||
[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+
Assignee | ||
Updated•7 years ago
|
Attachment #8900685 -
Attachment is obsolete: true
Comment 16•7 years ago
|
||
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+
Comment 17•7 years ago
|
||
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)
Comment 18•7 years ago
|
||
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)
Comment 19•7 years ago
|
||
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 20•7 years ago
|
||
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?
Updated•7 years ago
|
Attachment #8901080 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/8190fc1ba510
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 22•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1fd4f4337aae
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Comment 23•7 years ago
|
||
(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-
Updated•7 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main56+]
Assignee | ||
Comment 24•6 years ago
|
||
Is it a good time to check in the test?
Flags: needinfo?(abillings)
Comment 25•6 years ago
|
||
(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)
Assignee | ||
Comment 26•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/db6afcdb09c0e90d0e3b6f83836466f3464c4683
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•