Closed Bug 1201793 Opened 9 years ago Closed 9 years ago

"Assertion failure: !has(reg), at ../../../gecko/js/src/jit/RegisterSets.h:860" in B2G debug build on Z3C

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox40 --- wontfix
firefox41 + fixed
firefox42 + fixed
firefox43 + fixed
firefox-esr38 41+ fixed
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- wontfix
b2g-v2.1S --- wontfix
b2g-v2.2 --- fixed
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: heycam, Assigned: lth)

References

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage][adv-main41+][adv-esr38.3+])

Attachments

(3 files)

I am trying to produce a debug build for my Z3C.  When I do, the homescreen app hits this assertion a couple of seconds after starting:

Assertion failure: !has(reg), at ../../../gecko/js/src/jit/RegisterSets.h:860

I followed these steps from my B2G checkout to produce the build:

$ git pull
$ rm -rf objdir-gecko out
$ repo forall -c 'git reset --hard'
$ repo forall -c 'git clean -f -d'
$ repo sync -d
$ ./config.sh aries
$ ./build.sh
$ ./flash.sh

and my .userconfig is:

export B2G_DEBUG=1
export NOFTU=1
export DEVICE_DEBUG=1
This is likely related to one of the recent addition from Bug 986680.  We are trying to armor our Jit to prevent the generation of code which might make us generate exploitable code.

Hopefully, this assertion should be fixed by a patch that I will land in a few hours (on Bug 1199171).
In this case this is a false-positive, but if you see any more like these, I suggest you to open a bug and flag it as security-sensitive.

Thanks.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Blocks: 1199171
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
https://hg.mozilla.org/mozilla-central/rev/de9571163bbb
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Mozilla-inbound from this evening (Sept 7), 3 instances of this:

Assertion failure: !has(reg), at /mnt/ssd/moz/mozilla-inbound/js/src/jit/RegisterSets.h:860

FAILURES:
    /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/ion/bug1000605.js
    --ion-eager /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/ion/divmodself.js
    /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/v8-v5/check-deltablue.js

Command line is straightforward, no frills debug build + run:
   lth@mosquito$ ./jit_test.py ../build-debug/dist/bin/js

The hardware is a quad-core Cortex-A15 NVIDIA Jetson TK1 development board, Ubuntu 14.04.
Flags: needinfo?(nicolas.b.pierron)
(In reply to Lars T Hansen [:lth] from comment #5)
> FAILURES:
>     /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/ion/bug1000605.js
>     --ion-eager
> /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/ion/divmodself.js
>    
> /mnt/ssd/moz/mozilla-inbound/js/src/jit-test/tests/v8-v5/check-deltablue.js
> 
> Command line is straightforward, no frills debug build + run:
>    lth@mosquito$ ./jit_test.py ../build-debug/dist/bin/js
> 
> The hardware is a quad-core Cortex-A15 NVIDIA Jetson TK1 development board,
> Ubuntu 14.04.

I am sorry, I am unable to reproduce it with the simulator at the moment.  This might be caused by the fact that compilations are ""relatively"" faster than the executed code, when we are emulating the architecture.

If you are able to reproduce these issues within gdb, then feel free to debug it.  I can help you on IRC if you need.  Otherwise, let's hope decoder would find multiple occurrences of these issues soon.
Flags: needinfo?(nicolas.b.pierron)
This is js/src/jit-test/tests/ion/bug1000605.js.

ma_b() claims the ScratchRegister but DivI has already claimed it.

#0  0x005f1002 in js::jit::SpecializedRegSet<>::add () at jit/RegisterSets.h:860
#1  0x0065fc0a in js::jit::AutoGenericRegisterScope<>::AutoGenericRegisterScope () at jit/MacroAssembler.cpp:2898
#2  0x0018880e in js::jit::ScratchRegisterScope::ScratchRegisterScope () at jit/arm/Assembler-arm.h:54
#3  0x006ed4f2 in js::jit::MacroAssemblerARM::ma_b () at jit/arm/MacroAssembler-arm.cpp:1464
#4  0x006b360c in js::jit::CodeGeneratorARM::bailoutIf () at jit/arm/CodeGenerator-arm.cpp:141
#5  0x006b4d4c in js::jit::CodeGeneratorARM::visitDivI () at jit/arm/CodeGenerator-arm.cpp:554

I might be inclined to suggest that bailoutIf or its caller needs to relinquish the scratch register scope on bailout, but I've not tried to understand the code in detail.  visitDivPowTwoI appears to have code for just such a fix, with a comment suggesting this was indeed the problem.

I'd also want to worry about visitOutOfLineBailout, for the same reason.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The other two failing runs have the same stack: DivI and then bailoutIf.
Attached patch Patch for m-i — — Splinter Review
This just rearranges the registers so that the scope can be used properly.  Fixes the three test cases, but I need to run a full test suite.
Attachment #8658139 - Flags: feedback?(nicolas.b.pierron)
Comment on attachment 8658139 [details] [diff] [review]
Patch for m-i

Apart from separately filed bug 1202643 this passes testing so far, so upgrading to r?.
Attachment #8658139 - Flags: feedback?(nicolas.b.pierron) → review?(nicolas.b.pierron)
Comment on attachment 8658139 [details] [diff] [review]
Patch for m-i

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

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +556,2 @@
>          bailoutIf(Assembler::NotEqual, ins->snapshot());
> +        masm.ma_mov(temp, output);

Could we change the scoping inside ma_mov, or we have to swap the registers?
Group: javascript-core-security
Comment on attachment 8658139 [details] [diff] [review]
Patch for m-i

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

How did this ever worked before?!

This patch fix a register miss-use which cause the leak of a pointer to a some generated stub code which is used for reconstructing the stack.
Leaking this pointer is a HUGE security issue as this memory area used to be writable (a-part from flushing the instruction cache).

This patch will not apply directly on previous branches (due to the lack of ScratchRegisterScope), but we should definitely backport it to all ARM releases.

Side-Note: Before landing this patch, we should double check all uses of bailoutIf on ARM, to ensure that we do not accidentally erase the scratch register with the bailout address.
Attachment #8658139 - Flags: review?(nicolas.b.pierron) → review+
Keywords: sec-high
This issue got introduced by Bug 872824 (gecko 25).

It might be used to create an exploit, due to the leak of a pointer (result of an integer division) to writable & executable page.

Only ARM targets are concerned.
(In reply to Nicolas B. Pierron [:nbp] from comment #12)
> Comment on attachment 8658139 [details] [diff] [review]
> Tentative fix
> 
> Side-Note: Before landing this patch, we should double check all uses of
> bailoutIf on ARM, to ensure that we do not accidentally erase the scratch
> register with the bailout address.

I think that if we do proper testing (ahem) in debug builds then the ScratchRegisterScope will catch that, as it did here.  The problem is to do proper testing.  In this case I was just lucky because I had a fast machine.

Anyway I went over the ARM files and looked at all uses of bailoutIf, and I did not find any more problems with it being called inside a ScratchRegisterScope.  I'll look into preparing a patch for older branches before landing on m-i (or, let me know if something else is appropriate).

I'll also look into the problem I mentioned earlier with visitOutOfLineBailout, which should assert in the same way given a suitable test case.  Again, the level of testing is probably not what we'd like it to be.
So far as I can tell, the offending function visitOutOfLineBailout() is dead code.  (Were it not it would be easy to fix the problem but it might be better to remove the function?)
Last word on visitOutOfLineBailout: the code is not a problem because it uses a non-problematic variant of ma_b(), not the one that needs a scratch register.
Attached patch Patch for Aurora, Beta, etc. — — Splinter Review
Attachment #8658293 - Flags: review?(nicolas.b.pierron)
Attachment #8658139 - Attachment description: Tentative fix → Patch for m-i
Attachment #8658293 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

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

Anyone who truly understands the patch will see that it is about avoiding a bailout overwriting a result, and will start to investigate whether the bailout code can be subverted.  That's probably tricky (comment 12 has some suggestions) but we must assume that a valid memory address inside the engine can be extracted.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No test cases, and the check-in comment can be made bland.

Which older supported branches are affected by this flaw?

Lots.  See annotations by :nbp and :ryanvm on this bug.  There are two patches, one for central/inbound and one for the others (the present one).

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

According to :nbp, Bug 872824 (see comment 13 on the present bug).

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

I believe the present patch is the appropriate backport for the affected patches; the nightly patch is different and can land later.

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

It is not likely to cause regressions and needs only normal testing.
Attachment #8658293 - Flags: sec-approval?
sec-approval+ for trunk. Please nominate the appropriate patch for Aurora, Beta, and ESR38 (ideally today) so we can get it in to those before Friday's last beta.
Attachment #8658293 - Flags: sec-approval? → sec-approval+
(In reply to Al Billings [:abillings] from comment #19)
> sec-approval+ for trunk. Please nominate the appropriate patch for Aurora,
> Beta, and ESR38 (ideally today) so we can get it in to those before Friday's
> last beta.

The appropriate patch for Aurora, Beta, and ESR38 is the one labeled "Patch for Aurora, Beta, etc", the one that was approved by your reply.  Do I need to do anything more in this bug than telling you this?  (First time I do this, bear with me...)
Flags: needinfo?(abillings)
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Security issue
[Describe test coverage new/current, TreeHerder]: Found by testing on ARM HW
[Risks and why]: Exposes memory address to JS
[String/UUID change made/needed]: N/A
Attachment #8658293 - Flags: approval-mozilla-aurora?
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Security issue
[Describe test coverage new/current, TreeHerder]: Found by testing on ARM HW
[Risks and why]: Information leakage
[String/UUID change made/needed]: N/A
Attachment #8658293 - Flags: approval-mozilla-beta?
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

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

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8658293 - Flags: approval-mozilla-esr38?
Flags: needinfo?(abillings)
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

Sec fix, taking it for Aurora42 and Beta41.
Attachment #8658293 - Flags: approval-mozilla-beta?
Attachment #8658293 - Flags: approval-mozilla-beta+
Attachment #8658293 - Flags: approval-mozilla-aurora?
Attachment #8658293 - Flags: approval-mozilla-aurora+
Comment on attachment 8658293 [details] [diff] [review]
Patch for Aurora, Beta, etc.

Sec-high fix, taking this for esr 38.
Attachment #8658293 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
https://hg.mozilla.org/mozilla-central/rev/a6163cb891f8
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
I'm pretty sure this isn't actually exploitable.

If you look at ma_b(), which requests use of a scratch register, you'll find that the scratch register is only used on two of three paths in a switch statement, for B_MOVWT and B_LDR_BX.

The switch is done on b_type(), which is hardcoded to B_LDR. So the two paths on which the scratch register would be overwritten are actually just dead code, and the B_LDR path just calls as_Imm32Pool(), which does not overwrite the scratch register.

It's bad code, but it's not exploitable.
Also I'm pretty sure the patch is just totally wrong. Via the code:

> scratch = lhs / rhs;
> temp = scratch * rhs;

so, (with a check that the math works out):

> temp = (lhs / rhs) * rhs = lhs

so by returning temp instead of scratch, it's always just returning lhs, instead of the result of the idiv. You really do want to return the scratch register.

The patch should be reverted everywhere, and it is not a security problem per Comment 29.
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(lhansen)
Oh. I just realized that the variable names were switched, so the patch doesn't have any effect. Never mind, then! All is well.
Flags: needinfo?(nicolas.b.pierron)
Flags: needinfo?(lhansen)
Hey Lars, this cause conflicts on aurora like: merging js/src/jit/arm/CodeGenerator-arm.cpp failed!

could you take a look, thanks!
Flags: needinfo?(lhansen)
(In reply to Carsten Book [:Tomcat] from comment #33)
> Hey Lars, this cause conflicts on aurora like: merging
> js/src/jit/arm/CodeGenerator-arm.cpp failed!

Can you backout the backported patch and do the merge?
Group: javascript-core-security → core-security-release
(In reply to Carsten Book [:Tomcat] from comment #33)
> Hey Lars, this cause conflicts on aurora like: merging
> js/src/jit/arm/CodeGenerator-arm.cpp failed!
> 
> could you take a look, thanks!

I don't know what to think about that (and I don't know where to look).  I haven't landed anything for this bug, everything that's been landed has been landed for me by somebody else, presumably various sheriffs and branch owners.  I prepared two patches, one for m-i and one for Aurora etc.  Those patches can't merge as they touch the same code, so a conflict is inevitable if m-c is merged to Aurora or Aurora is merged to m-c.
Flags: needinfo?(lhansen)
The "merge" in question would have been hg describing a patch that failed to apply as a merge conflict. Dunno why it didn't apply for him, worked fine for me.

https://hg.mozilla.org/releases/mozilla-aurora/rev/6fa940a15b93
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+][adv-esr38.3+]
nbp: on 2.2 i get grafting 261658:a6163cb891f8 "Bug 1201793 - correct scratch register scope r=nbp CLOSED TREE"
merging js/src/jit/arm/CodeGenerator-arm.cpp
warning: conflicts during merge.


do you know what i do wrong here ? :)
Flags: needinfo?(nicolas.b.pierron)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: