Closed Bug 1171540 (CVE-2015-4484) Opened 9 years ago Closed 9 years ago

crash in void js::jit::AssemblerX86Shared::lock_addl<js::jit::Imm32>(js::jit::Imm32, js::jit::Operand const&)

Categories

(Core :: JavaScript Engine: JIT, defect)

Unspecified
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox39 + wontfix
firefox40 --- fixed
firefox41 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 40+ fixed
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: jujjyl, Assigned: lth)

Details

(Keywords: crash, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main40+][adv-esr38.2+])

Crash Data

Attachments

(2 files, 2 obsolete files)

Attached file 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?
Is this on an SAB atomic operation? But I thought that was only enabled on Nightly?
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?
Flags: needinfo?(lhansen)
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.
Flags: needinfo?(lhansen)
Fair enough, though that does not explain why the bug is being triggered, so I guess that's job #1 now.
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.
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.
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Attachment #8616665 - Flags: review?(luke)
Attachment #8616665 - Flags: feedback?(jujjyl)
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)" ?
[Tracking Requested - why for this release]:
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.
Attachment #8616665 - Flags: feedback?(jujjyl) → feedback+
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.
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.
Attachment #8617882 - Flags: feedback?(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.
Attachment #8617882 - Flags: feedback?(luke) → feedback+
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.
Attachment #8617882 - Flags: review?(luke)
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.
Attachment #8617882 - Flags: review?(luke)
Minor tidying up of the test cases, code is unchanged.
Attachment #8616665 - Attachment is obsolete: true
Attachment #8617882 - Attachment is obsolete: true
Attachment #8616665 - Flags: review?(luke)
Attachment #8622438 - Flags: review?(luke)
Attachment #8622438 - Flags: review?(luke) → review+
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.
Attachment #8622438 - Flags: approval-mozilla-aurora?
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
https://hg.mozilla.org/mozilla-central/rev/e815b262f9ac
https://hg.mozilla.org/mozilla-central/rev/41142f760914
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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 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.
Attachment #8622438 - Flags: approval-mozilla-beta?
Attachment #8622438 - Flags: approval-mozilla-aurora?
Attachment #8622438 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(sledru)
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!
Flags: needinfo?(sledru)
(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.
Flags: needinfo?(sledru)
Of course, mxr says that SharedArrayBuffer exists all the way back as far as esr31, so...
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.
Attachment #8622438 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

Why not. Taking it in esr then.
Flags: needinfo?(sledru)
Attachment #8622438 - Flags: approval-mozilla-esr38+
Comment on attachment 8622438 [details] [diff] [review]
Add gating to Odin for shared memory + test cases, v3

See comments 11 and 18.
Attachment #8622438 - Flags: approval-mozilla-b2g37?
Attachment #8622438 - Flags: approval-mozilla-b2g34?
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.
Attachment #8622438 - Flags: approval-mozilla-b2g37?
Attachment #8622438 - Flags: approval-mozilla-b2g37+
Attachment #8622438 - Flags: approval-mozilla-b2g34?
Attachment #8622438 - Flags: approval-mozilla-b2g34+
Attachment #8622438 - Flags: approval-mozilla-b2g34+
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main40+][adv-esr38.2+]
Alias: CVE-2015-4484
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.