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


(Core :: JavaScript: WebAssembly, defect, P1)




Tracking Status
firefox-esr60 65+ fixed
firefox64 --- wontfix
firefox65 + fixed
firefox66 + fixed


(Reporter: gkw, Assigned: lth)


(6 keywords, Whiteboard: [jsbugmon:update][post-cristsmash-triage][adv-main65+][adv-esr60.5+])


(4 files)

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.


#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

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.
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)
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.
(Unblocking the atomics bug because this feature is for the really unrelated sign extension feature.)
No longer blocks: 1377576
Flags: needinfo?(lhansen)
Assignee: nobody → lhansen
Priority: -- → P1
Component: JavaScript Engine → Javascript: Web Assembly
Any idea what the sec rating of this bug should be?
Flags: needinfo?(lhansen)
Sounds like some kind of bounds problem that might be difficult to exploit, so I'll mark it sec-high.
Flags: needinfo?(lhansen)
Note, compiling with --enable-debug and running with --no-wasm-ion is sufficient to repro with the TC.
Attached file extend8.js
Rather simpler test case, compile with --enable-debug and run with --no-wasm-ion.
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 on attachment 9033921 [details] [diff] [review]

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+
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.
Comment on attachment 9033921 [details] [diff] [review]

[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?
I have verified that the patch applies to mozilla-beta.
Group: javascript-core-security → core-security-release
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Comment on attachment 9033921 [details] [diff] [review]

[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?
Comment on attachment 9033921 [details] [diff] [review]

[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+
Do we need the same fix for the emitExtendI32_16 function just below this?

(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.
(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.
(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.
Flags: qe-verify-
Whiteboard: [jsbugmon:update] → [jsbugmon:update][post-cristsmash-triage]
Whiteboard: [jsbugmon:update][post-cristsmash-triage] → [jsbugmon:update][post-cristsmash-triage][adv-main65+][adv-esr60.5+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.