1516738 - Assertion failure: size_t(reg) < mozilla::ArrayLength(names), at js/src/jit/x86-shared/Constants-x86-shared.h:194 with wasm

Status: RESOLVED FIXED

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

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Detailed Crash Information

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Testcase

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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?

Comment 4

[Lars T Hansen [:lth]]

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.

Comment 5

[Lars T Hansen [:lth]]

(Unblocking the atomics bug because this feature is for the really unrelated sign extension feature.)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Any idea what the sec rating of this bug should be?

Comment 7

[Andrew McCreight [:mccr8]]

Sounds like some kind of bounds problem that might be difficult to exploit, so I'll mark it sec-high.

Comment 8

[Lars T Hansen [:lth]]

Note, compiling with --enable-debug and running with --no-wasm-ion is sufficient to repro with the TC.

Comment 9

[Lars T Hansen [:lth]]

Attachment: extend8.js

Rather simpler test case, compile with --enable-debug and run with --no-wasm-ion.

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1516738-specialize-for-byte-register.patch

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.

Comment 11

[Julian Seward [:jseward]]

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.

Comment 12

[Lars T Hansen [:lth]]

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 13

[Lars T Hansen [:lth]]

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.

Comment 14

[Lars T Hansen [:lth]]

I have verified that the patch applies to mozilla-beta.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/00ff999d70b351e37c404ee4e1cab6f8cf9cfe43
https://hg.mozilla.org/mozilla-central/rev/00ff999d70b3

Comment 16

[Lars T Hansen [:lth]]

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:

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8da001054d07

Comment 19

[Nathan Froyd [:froydnj]]

Do we need the same fix for the emitExtendI32_16 function just below this?

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 21

[Lars T Hansen [:lth]]

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

Comment 22

[Lars T Hansen [:lth]]

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