1171540 - (CVE-2015-4484) crash in void js::jit::AssemblerX86Shared::lock_addl<js::jit::Imm32>(js::jit::Imm32, js::jit::Operand const&)

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jukka Jylänki]

Attachment: atomics_crash.tar.gz

This bug was filed from the Socorro interface and is 
report bp-3cd5bd7d-33e2-45a2-afce-39cb22150604.
=============================================================

There is a systematically reproducible crash in current Firefox Beta channel at least, which affects Linux and OS X. To reproduce, open the .html page from the attached file.

The crash occurs in current Emscripten unit test build servers (logs don't really show much except timeout, but linking here just as a record):

OSX: http://clb.demon.fi:8112/builders/osx-incoming/builds/891/steps/Tests-browser-firefox-beta/logs/stdio
Linux: http://clb.demon.fi:8112/builders/ubuntu-incoming/builds/888/steps/Tests-browser-firefox-beta/logs/stdio

The issue does not occur in the Emscripten unit test servers in Firefox stable channel, current FF Dev Edition or FF Nightly, and it does not occur on Windows either.

I'm initially assuming this might be a crash that has been already fixed in FF Dev Edition, but that the crash slipped through to FF Beta channel release? Or alternative possibility is that the issue is still present, but somehow masked in FF Dev Edition and newer?

Comment 1

[Alon Zakai (:azakai)]

Is this on an SAB atomic operation? But I thought that was only enabled on Nightly?

Comment 2

[Jukka Jylänki]

I think this occurs immediately when parsing the page, during asm.js compilation? Also, I believe the enablement is only done by gating on whether the SharedArrayBuffer and Atomics objects are exposed to JS or not. If those are not exposed, then effectively the feature should be fully disabled. Lars knows this, that's only my current hunch. I think that is a feasible way to do it, as long as we don't crash when parsing.

Hopefully this is fixable just by cherry-picking a fix from a later date?

Comment 3

[Lars T Hansen [:lth]]

SAB, STA, and Atomics are enabled only on Nightly and when they are disabled it is by removing the names from the JS environment.

Most likely there is an uninitialized bit in some MIR node that causes a barrier to be emitted after a store; the barrier is implemented as lock_addl on non-sse2 systems.  We should not be on a non-sse2 system, but if one bit is off another might be.  I smell broader memory corruption.

Also, I suddenly wonder if SAB/STA/Atomics are properly gated when compiling asm.js...

I will look into this in the morning.

Comment 4

[Jukka Jylänki]

Bisected this to have broken first time at https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0c7cb48aed86&tochange=7cf3406ac1cd , and then later fixed by https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6f8e9bf83767&tochange=e213e4546d9a .

Comment 5

[Lars T Hansen [:lth]]

Fair enough, though that does not explain why the bug is being triggered, so I guess that's job #1 now.

Comment 6

[Lars T Hansen [:lth]]

OK, so the crash will happen because:

- Odin compiles a program eagerly; the presence of the program causes Odin to run

- Odin does not properly gate access to the Atomics or the SharedArrayBuffer views

- Odin gets a program that contains Atomics and generates code for it, and tickles the assembler bug

Since SharedArrayBuffer is not available on the JS side the compiled program could never be run (this would be the current situation in Dev Edition, where Odin does not crash because the assembler bug has been fixed), so I doubt this is a security sensitive problem.

The appropriate fix is to add proper gating in Odin.  I'm not sure if either this bug or bug 1147916 (the assembler fix) is beta-worthy; I'm open to opinions.

Comment 7

[Lars T Hansen [:lth]]

Attachment: Add gating to Odin for shared memory + test cases

This adds gating by name to Odin (shared view types plus "Atomics").  I could disable more code than what I do but based on control flow I think this is sufficient.  I could probably also disable less code than what I do, but I opted for a little defense in depth.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8616665 [details] [diff] [review]
Add gating to Odin for shared memory + test cases

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

Instead of these #ifdefs, could we have just one #ifdef (after the module declarations, before the function bodies) that fails if shared=true and !defined(ENABLE_SHARED_ARRAY_BUFFER)?

::: js/src/asmjs/AsmJSValidate.cpp
@@ +4129,5 @@
>          if (mathOrAtomicsOrSimd == m.cx()->names().SIMD)
>              return CheckGlobalSimdImport(m, initNode, varName, field);
> +#ifdef ENABLE_SHARED_ARRAY_BUFFER
> +        return m.failName(base, "expecting %s.{Math|Atomics|SIMD}", globalName);
> +#else

Seems harmless to leave off the #ifdef for this one.

::: js/src/jit-test/tests/asm.js/gating.js
@@ +18,5 @@
> +	return v|0;
> +    }
> +
> +    return { load: do_load };
> +}

Could you add any conditional "if (SharedArrayBuffer) assertEq(isAsmJSModule(loadModule_a), true)" ?

Comment 9

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:

Comment 10

[Jukka Jylänki]

Looks good. I agree also that no need to go conditionally excluding every individual bit, but only the parts where the SAB and Atomics are exposed.

Comment 11

[Andrew McCreight [:mccr8]]

I think it would be good to backport this patch to every branch that SharedArrayBuffer is in, so we don't have to worry about whether any other random security issues in this code (past and future) might affect a non-Nightly branch. The patch looks simple enough.

Comment 12

[Lars T Hansen [:lth]]

Attachment: Add gating to Odin for shared memory + test cases, alternate solution

You're thinking something like this?  It effectively works, though there is at least one hole - an operation such as Atomics.isLockFree, which does not touch any views, will not be flagged as illegal.  I don't anticipate there being more functions like isLockFree.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8617882 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, alternate solution

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

Yes, exactly, thank you.

Comment 14

[Lars T Hansen [:lth]]

Comment on attachment 8617882 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, alternate solution

Well, unless you're concerned about Atomics.isLockFree (which isn't even in the tree currently, so I guess we can defer that issue in any case), perhaps this is the patch then.

As you'll see one of the test cases is commented out because of bug 1172517.  I'd prefer not to block this bug on bug 1172517, but I'll fix that next.

Comment 15

[Lars T Hansen [:lth]]

Comment on attachment 8617882 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, alternate solution

Withdrawing r?, I need to rethink the test cases, which won't work properly in a non-Nightly build.

Comment 16

[Lars T Hansen [:lth]]

Attachment: Add gating to Odin for shared memory + test cases, v3

Minor tidying up of the test cases, code is unchanged.

Comment 17

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e815b262f9ac

Comment 18

[Lars T Hansen [:lth]]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

Approval Request Comment
[Feature/regressing bug #]: Feature is experimental shared memory for JS, bug 1054841, available only on Nightly, this is a gating corner case that was missed
[User impact if declined]: Possible to construct content that touches this feature on non-Nightly, may tickle other bugs or cause inconsistencies, as in the present case
[Describe test coverage new/current, TreeHerder]: Added test case to jit-tests to ensure that gating is correct
[Risks and why]: Low risk, no live code on the web should be affected
[String/UUID change made/needed]: None

The eventual target for this fix is beta, before beta moves to release; my understanding is that it should go via Aurora.  Please advise.

Comment 19

[Lars T Hansen [:lth]]

Addendum, to handle a corner case when asm.js is disabled at runtime due to no FPU support (--no-fpu):
https://hg.mozilla.org/integration/mozilla-inbound/rev/41142f760914

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e815b262f9ac
https://hg.mozilla.org/mozilla-central/rev/41142f760914

Comment 21

[Lars T Hansen [:lth]]

Ryan asked on irc whether we should backport to 38.  It wouldn't hurt to backport it, but the only reason we discovered it was because of that assembler bug, which seems not to be present in 38, if i interpreted the error report correctly.  The risk of such a backport is low, however.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

Approval Request Comment:
See comment #18.

Requesting the uplift myself to bring this patch to Liz attention.

Anyway, taking it for aurora
As it is a sec moderate on an experimental feature, I don't think we should care about ESR.

Comment 23

[Lars T Hansen [:lth]]

Sylvestre, what happens now - do you land the patch on Aurora or do I?  (I've never done this before.)  And if me, anything special I need to do to push it?

Comment 24

[Sylvestre Ledru [:Sylvestre]]

You don't have to do anything. The sheriffs are scanning for the uplift approval. They will take care of that for both of us!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Anyway, taking it for aurora
> As it is a sec moderate on an experimental feature, I don't think we should
> care about ESR.

My concern was over comment 11.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Of course, mxr says that SharedArrayBuffer exists all the way back as far as esr31, so...

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/106cbc77cc87

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

I can see the argument for doing this especially if we were earlier in the beta cycle, but beta 7 (last chance for beta) is going to build tomorrow so I don't want to risk changes that aren't for sec-critical or some sec-high bugs.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

Why not. Taking it in esr then.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/d5d5e1ab76e8

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

See comments 11 and 18.

Comment 32

[Josh Cheng [:josh]]

Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

Approving for increasing stability. Please NI if causing any side effect.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/05fb211d4c6c

Turns out that this depends on bug 1073096, which landed on Gecko 36. So older versions aren't affected.