Closed
Bug 1516738
Opened 6 years ago
Closed 6 years ago
Assertion failure: size_t(reg) < mozilla::ArrayLength(names), at js/src/jit/x86-shared/Constants-x86-shared.h:194 with wasm
Categories
(Core :: JavaScript: WebAssembly, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla66
People
(Reporter: gkw, Assigned: lth)
Details
(6 keywords, Whiteboard: [jsbugmon:update][post-cristsmash-triage][adv-main65+][adv-esr60.5+])
Attachments
(4 files)
22.53 KB,
text/plain
|
Details | |
19.01 KB,
application/octet-stream
|
Details | |
353 bytes,
application/x-javascript
|
Details | |
2.14 KB,
patch
|
jseward
:
review+
RyanVM
:
approval-mozilla-beta+
RyanVM
:
approval-mozilla-esr60+
RyanVM
:
sec-approval+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-beta revision 0e5689df129b (build with --32 --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-baseline --
no-ion --test-wasm-await-tier2 w212-out.wrapper w212-out.wasm):
See attachment.
Backtrace:
#0 js::jit::X86Encoding::GPReg8Name (reg=js::jit::X86Encoding::rsi) at js/src/jit/x86-shared/Constants-x86-shared.h:194
#1 js::jit::X86Encoding::BaseAssembler::movsbl_rr (dst=js::jit::X86Encoding::rsi, src=<optimized out>, this=0xf6302228) at js/src/jit/x86-shared/BaseAssembler-x86-shared.h:2314
#2 js::jit::AssemblerX86Shared::movsbl (dest=..., src=..., this=<optimized out>) at js/src/jit/x86-shared/Assembler-x86-shared.h:717
#3 js::jit::MacroAssembler::move8SignExtend (dest=..., src=..., this=<optimized out>) at js/src/jit/x86-shared/MacroAssembler-x86-shared-inl.h:28
#4 js::wasm::BaseCompiler::emitExtendI32_8 (this=0xf6301ad4) at js/src/wasm/WasmBaselineCompile.cpp:7136
#5 0x5717e8e4 in js::wasm::BaseCompiler::emitBody (this=0xf6301ad4) at js/src/wasm/WasmBaselineCompile.cpp:10690
/snip
For detailed crash information, see attachment.
Setting s-s because I don't know how bad this is, being related to the MacroAssembler and wasm base compiler.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
Reporter | ||
Comment 3•6 years ago
•
|
||
autobisectjs shows this is probably related to the following changeset:
The first bad revision is:
changeset: a80521fee641
user: Lars T Hansen
date: Sat Jul 01 11:42:33 2017 -0700
summary: Bug 1377576 - Define atomic ops, add to verifier and test cases, stub out in compilers. r=sunfish
Lars, is bug 1377576 a likely regressor?
Blocks: 1377576
Flags: needinfo?(lhansen)
Assignee | ||
Comment 4•6 years ago
|
||
Yeah, that emitter needs to ensure that the source value is in an 8-bit register on x86-32. Here the source is rsi, which does not have an 8-bit personality. There's a temp available so this should be easy to fix.
It's probably slightly tricky to make the baseline compiler generate predictable code for this bug, so it's hard to say how one can exploit this, but it seems worthy of an expedient fix and uplift, given how many of our Windows users are on 32-bit still.
Assignee | ||
Comment 5•6 years ago
|
||
(Unblocking the atomics bug because this feature is for the really unrelated sign extension feature.)
No longer blocks: 1377576
Flags: needinfo?(lhansen)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Priority: -- → P1
Assignee | ||
Updated•6 years ago
|
Component: JavaScript Engine → Javascript: Web Assembly
Comment 6•6 years ago
|
||
Any idea what the sec rating of this bug should be?
status-firefox64:
--- → wontfix
status-firefox66:
--- → affected
status-firefox-esr60:
--- → affected
Flags: needinfo?(lhansen)
Comment 7•6 years ago
|
||
Sounds like some kind of bounds problem that might be difficult to exploit, so I'll mark it sec-high.
Flags: needinfo?(lhansen)
Keywords: csectype-bounds,
sec-high
Assignee | ||
Comment 8•6 years ago
|
||
Note, compiling with --enable-debug and running with --no-wasm-ion is sufficient to repro with the TC.
Assignee | ||
Comment 9•6 years ago
|
||
Rather simpler test case, compile with --enable-debug and run with --no-wasm-ion.
Assignee | ||
Comment 10•6 years ago
|
||
Patch + TC. This is a simple local fix since I also want to uplift to beta; there are clearly some generalizations possible around how we handle byte registers, but I think that should be followup work.
Attachment #9033921 -
Flags: review?(jseward)
Comment 11•6 years ago
|
||
Comment on attachment 9033921 [details] [diff] [review]
bug1516738-specialize-for-byte-register.patch
Review of attachment 9033921 [details] [diff] [review]:
-----------------------------------------------------------------
Looks OK.
As a random observation .. it would also be possible to generate
xchg reg32, eax; movsbl al, eax; xchg reg32, eax
The exchanges are only one byte long (!) and presumably they disappear at
the register-rename stage of the pipeline. This would have the advantage
of not requiring an extra register.
Attachment #9033921 -
Flags: review?(jseward) → review+
Assignee | ||
Comment 12•6 years ago
|
||
That's nice. We do have ebx as a dedicated temp so there's no particular cost to using it the way I've done, it won't force spills or anything like that. If I were to generalize the code we have to deal with byte registers I would probably end up creating slightly worse code on average because I would end up using a non-temp, and we'd spill/sync more often.
In general I've thought about register exchanges in the past so that we wouldn't have to spill if a specific register is required but busy, but another register in the class is available to hold the value from the specific register. This comes up all the time on x86 (byte registers and eax/ecx/edx for specific purposes), some of the time on x64 (rax/rcx/rdx), rarely on ARM (need even/odd register pairs), and I think never on ARM64. In most settings this would just be a MOV but it could be XCHG when the value to be popped is already in a register. Worth remembering.
Assignee | ||
Comment 13•6 years ago
|
||
Comment on attachment 9033921 [details] [diff] [review]
bug1516738-specialize-for-byte-register.patch
[Security Approval Request]
How easily could an exploit be constructed based on the patch?: Note, I bungled this process again and landed the bug without sec-approval. I'm sorry.
Difficult to exploit, probably, and would require very careful timing to trigger the compiler with the problem. Normally the other compiler - without the problem - is used for most code, and to recompile code that is initially compiled with the affected compiler.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
Which older supported branches are affected by this flaw?: Going back several versions for sure
If not all supported branches, which bug introduced the flaw?: None
Do you have backports for the affected branches?: Yes
If not, how different, hard to create, and risky will they be?: (I expect FF65 will be fixed and this patch should apply, but I will verify)
How likely is this patch to cause regressions; how much testing does it need?: We have test cases and no further testing is needed.
Attachment #9033921 -
Flags: sec-approval?
Assignee | ||
Comment 14•6 years ago
|
||
I have verified that the patch applies to mozilla-beta.
Comment 15•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/00ff999d70b351e37c404ee4e1cab6f8cf9cfe43
https://hg.mozilla.org/mozilla-central/rev/00ff999d70b3
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Assignee | ||
Comment 16•6 years ago
|
||
Comment on attachment 9033921 [details] [diff] [review]
bug1516738-specialize-for-byte-register.patch
[Beta/Release Uplift Approval Request]
Feature/Bug causing the regression: None
User impact if declined: Some wasm programs will fail; possible security implictions in that it may be possible to coerce the compiler into generating malicious code.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce:
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Point fix for clearly-wrong code generation using known-good pattern.
String changes made/needed:
Attachment #9033921 -
Flags: approval-mozilla-beta?
Updated•6 years ago
|
tracking-firefox65:
--- → +
tracking-firefox66:
--- → +
tracking-firefox-esr60:
--- → 65+
Flags: in-testsuite+
Comment 17•6 years ago
|
||
Comment on attachment 9033921 [details] [diff] [review]
bug1516738-specialize-for-byte-register.patch
[Triage Comment]
Would have been good if the test for this would have landed at a later time since this affects all branches. Fixes a bug in the wasm compiler that could cause it to generate malicious code under some circumstances. Approved for 65.0b9 and 60.5.0esr.
I'll go ahead and grant the sec-approval too since that ship's already sailed at this point anyway.
Attachment #9033921 -
Flags: sec-approval?
Attachment #9033921 -
Flags: sec-approval+
Attachment #9033921 -
Flags: approval-mozilla-esr60+
Attachment #9033921 -
Flags: approval-mozilla-beta?
Attachment #9033921 -
Flags: approval-mozilla-beta+
Comment 18•6 years ago
|
||
uplift |
Comment 19•6 years ago
|
||
Do we need the same fix for the emitExtendI32_16 function just below this?
Comment 20•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/6590fd8ab9c3
(In reply to Nathan Froyd [:froydnj] from comment #19)
> Do we need the same fix for the emitExtendI32_16 function just below this?
Please move that to a follow-up bug if the answer is yes.
Assignee | ||
Comment 21•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #19)
> Do we need the same fix for the emitExtendI32_16 function just below this?
Not to my knowledge, and I did check this. On x86 only the byte registers are weird because si and di do not have byte personalities; for 16->32 there are 16-bit personalities of all the source registers, including si and di.
I also glanced at the 8->64 to make sure they were OK and they looked OK because of how we generate code, but I should probably check that again to be completely confident. And it would be good to have test cases for both that and the 16-bit variants. Will handle that elsewhere.
Assignee | ||
Comment 22•6 years ago
|
||
(In reply to Lars T Hansen [:lth] from comment #21)
> (In reply to Nathan Froyd [:froydnj] from comment #19)
> > Do we need the same fix for the emitExtendI32_16 function just below this?
>
> I also glanced at the 8->64 to make sure they were OK and they looked OK
> because of how we generate code, but I should probably check that again to
> be completely confident.
No action needed, though I might push a comment to clarify the additional invariant.
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-cristsmash-triage]
Updated•6 years ago
|
Whiteboard: [jsbugmon:update][post-cristsmash-triage] → [jsbugmon:update][post-cristsmash-triage][adv-main65+][adv-esr60.5+]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•