1204700 - Assertion failure: !has(reg), at jit/RegisterSets.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

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

var x = [];
var y = [];
for (var i = 0; i < 3; i++) {
    y.push(x);
}

asserts js 32-bit debug ARM-simulator shell on m-c changeset fba4b0cd3823 with --fuzzing-safe --no-threads --ion-eager --unboxed-arrays at Assertion failure: !has(reg), at jit/RegisterSets.h

Configure options:

LD=ld CROSS_COMPILE=1 CC="clang -Qunused-arguments -msse2 -mfpmath=sse -arch i386" RANLIB=ranlib CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments -msse2 -mfpmath=sse" AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 HOST_CXX="clang++ -Qunused-arguments -msse2 -mfpmath=sse" sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-arm-simulator --enable-simulator=arm --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

python -u ~/funfuzz/js/compileShell.py -b "--32 --enable-debug --enable-nspr-build --enable-more-deterministic --enable-simulator=arm" -r fba4b0cd3823

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8b68a9f000b3
user:        Nicolas B. Pierron
date:        Wed Aug 26 20:48:05 2015 +0200
summary:     Bug 1190446 - Update Coverage information in Baseline. r=jandem

Nicolas, is bug 1190446 a likely regressor?

Comment 1

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

Attachment: stack

(lldb) bt 5
* thread #1: tid = 0x5882e3, 0x00764ecd js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register) + 64 at RegisterSets.h:860, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00764ecd js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(js::jit::MacroAssembler&, js::jit::Register) + 64 at RegisterSets.h:860
    frame #1: 0x00764e8d js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::AutoGenericRegisterScope<js::jit::Register>::AutoGenericRegisterScope(this=0xbfffe500, masm=<unavailable>, reg=(reg_ = r12)) + 61 at MacroAssembler.cpp:2898
    frame #2: 0x00829e28 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(js::jit::Register, js::jit::BaseIndex const&) [inlined] js::jit::ScratchRegisterScope::ScratchRegisterScope(this=<unavailable>, masm=<unavailable>) + 72 at Assembler-arm.h:55
    frame #3: 0x00829e09 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(js::jit::Register, js::jit::BaseIndex const&) [inlined] js::jit::ScratchRegisterScope::ScratchRegisterScope(masm=<unavailable>) at Assembler-arm.h:55
    frame #4: 0x00829e09 js-dbg-32-dm-nsprBuild-armSim-darwin-fba4b0cd3823`js::jit::MacroAssemblerARMCompat::store32(this=<unavailable>, src=(reg_ = r12), dest=0xbfffe578) + 41 at MacroAssembler-arm.cpp:2343
(lldb)

Comment 2

[Nicolas B. Pierron [:nbp — off until 29-09]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #0)
> Nicolas, is bug 1190446 a likely regressor?

No.

This particular test case should fail since Bug 1165392 & Bug 1165463, but the underlying issue is present in the JS Engine since Bug 976446.

This issue does the following: when storing a constant value in an array, instead of pushing the value of the constant, we push the address of the element in the Array element.

Thus, this issue leak a pointer to a JS Array, which can later be used to map the entire memory in conjunction with another issue which allow to update the length of the Array.

Comment 3

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: part 0 - Annotate the assertion with a recommended security rating.

Comment 4

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: part 1 - ARM: Use a different scratch register for store32.

This patch change store32(Register, BaseIndex) to use the second scratch
register, such that this does not conflict with the scratch register used in
storePtr(ImmWord, T).

This patch also change the store32(Imm32, BaseIndex) to use the first
scratch register instead of the second, such that this does not introduce
any conflict.

Comment 5

[Sean Stangl [:sstangl]]

Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

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

::: js/src/jit/RegisterSets.h
@@ +748,5 @@
>      using Parent::has;
>  
>      using Parent::addUnchecked;
>      void add(RegType reg) {
> +        MOZ_ASSERT(!has(reg), "sec-citical: Potentially overwritten register");

I realize this isn't my review, but it's spelled "citical" instead of "critical" everywhere.

Comment 6

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

Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Moving on to :sstangl for review. As he pointed out, "citical" should be spelled as "critical" instead.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

Not strongly opposing, but I wonder if it's a good thing to sound a five-alarm blaze so obviously for anyone who can fuzz us.  I would rest slightly easier if this were maintained not in the tree, although I acknolwedge that has its own problems of the connection going stale, etc.

Comment 8

[Sean Stangl [:sstangl]]

Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

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

While the patch looks fine to me, please keep in mind that this is yet again not exploitable, because the ScratchRegister is only overwritten inside store32(Register, BaseIndex) if (dest.offset != 0), and it is always zero. We very rarely use a BaseIndex with a constant offset.

If you really wanted, you could lessen the scope by just acquiring the ScratchRegisterScope within the specific branch, and duplicating the call to ma_str(). That may look nicer than Maybe<> and emplace().

The bug could possibly be S-S, but the test case given is not an example of something exploitable.

Comment 9

[Sean Stangl [:sstangl]]

Comment on attachment 8661230 [details] [diff] [review]
part 0 - Annotate the assertion with a recommended security rating.

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

Aha, now it is my review! I agree with Jeff.

Comment 10

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?

The patch highlights an issue with store32, looking at the chain of callers makes it obvious that we reuse the scratch register which is updated in this patch.

> 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?

All.

> If not all supported branches, which bug introduced the flaw?

Bug 976446

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I will attach the backports in a few minutes.  The backports do not rely on the ScratchRegisterScope which is used here to assert the correctness.

> How likely is this patch to cause regressions; how much testing does it need?

store32 is used by different caller, I should have covered this by looking carefully at the code, and running the test suite locally.  The test suite on treehereder is likely to catch remaining issues if any.

Comment 11

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

Not requesting aurora uplift yet :/

Comment 12

[Al Billings [:abillings - ex-MoCo]]

sec-approval for checkin on October 6, two weeks into the next cycle.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

This can be landed now.

Comment 14

[Nicolas B. Pierron [:nbp — off until 29-09]]

Attachment: (beta) ARM: Use a different scratch register for store32.

This patch should do the same as the previous patches, but for some reasons
that I do not understand I cannot compile & test ARM on beta right now.

Should I hold the landing on inbound?

Comment 15

[Nicolas B. Pierron [:nbp — off until 29-09]]

(ni? for comment 14)

Comment 16

[Al Billings [:abillings - ex-MoCo]]

I'd go ahead and land and watch it closely afterwards.

Comment 17

[Nicolas B. Pierron [:nbp — off until 29-09]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/464e82856a55

Comment 18

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

This landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/464e82856a55

:Tomcat, not sure if you're aware, but some bugs have not been marked fixed when the patch has moved on to m-c. This is the second one I've noticed. Maybe it's because it is a security bug?

Comment 19

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8661287 [details] [diff] [review]
part 1 - ARM: Use a different scratch register for store32.

Approval Request Comment
[Feature/regressing bug #]: Bug 976446
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]: inbound / m-c
[Risks and why]: see comment 2 and comment 10.
[String/UUID change made/needed]: N/A

Comment 20

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Approval Request Comment
(same as previous comment)

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Fix a sec high, taking it.
Should be in 42 beta 7.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b14beb29be00

gary: yeah seems something got wrong when marking this is on central, but fixed this

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ebafbc7c1a87

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/099fe0777201

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/099fe0777201

Comment 26

[Al Billings [:abillings - ex-MoCo]]

This needs an ESR38 patch as a sec-high. Nicolas is out (not accepting needinfo? so I assume he's out).

Comment 27

[Nicolas B. Pierron [:nbp — off until 29-09]]

(In reply to Al Billings [:abillings] from comment #26)
> This needs an ESR38 patch as a sec-high. Nicolas is out (not accepting
> needinfo? so I assume he's out).

The beta patch should work for esr38.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Do we want to take this on ESR38?

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Ritu is in PTO too.

NBP, could you request the uplift to esr too? Merci

Comment 30

[Nicolas B. Pierron [:nbp — off until 29-09]]

Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: None.
Fix Landed on Version: All except ESR.
Risk to taking this patch (and alternatives if risky): See comment 2 and comment 10
String or UUID changes made by this patch: None

Comment 31

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

Comment on attachment 8671496 [details] [diff] [review]
(beta) ARM: Use a different scratch register for store32.

Approved for uplift to esr38 since this fixes a sec issue we're releasing in all other branches.

Comment 32

[Al Billings [:abillings - ex-MoCo]]

This didn't land before build 1.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/794da8ad9e7e

Sorry for letting this slip until today.